Does the team require code reviews for all code and build scripts / configuration changes?

ID

esf_s3c_dev/code_review

Severity

high

Category

Levels

Optional

false

Tags

SSDF-PW.7.2, code-reviews, security, source-code, supply-chain

Description

Does the team require code reviews for all code and build scripts / configuration changes?

This check determines whether the project requires code review before pull requests (merge requests) are merged.

Reference: ESF - Securing the Software Supply Chain - Recommended practices for Developers - 2.2.1 Modification or Exploitation of Source Code by Insiders

Rationale

Reviews detect various unintentional problems, including vulnerabilities that can be fixed immediately before they are merged, which improves the quality of the code.

Reviews may also detect or deter an attacker trying to insert malicious code (either as a malicious contributor or as an attacker who has subverted a contributor’s account), because a reviewer might either detect the subversion, including any attempts made by the attacker to obfuscate malicious code or implant an evil dependency.

Lack of code review increase the risk of unintentional vulnerabilities or possible injection of malicious code.

Requiring review does not eliminate all risks. The other reviewers might fail to notice unintentional vulnerabilities or malicious code, be colluding with a malicious developer, or even be the same person (using a "sock puppet" account).

Verification

The check first tries to detect whether Branch-Protection is enabled on the default branch with at least one required reviewer or by requiring Approvals from Code Owners. If this fails, the check determines whether the most recent commits have an approved review or if the merger is different from the committer (implicit review). It also performs a similar check for reviews using Prow (labels "lgtm" or "approved") and Gerrit ("Reviewed-on" and "Reviewed-by").

Remediation

  • If the project has only one contributor, or does not have enough reviewers to practically require that all contributions be reviewed, try to recruit more maintainers to the project who will be willing to review others' work. Ideally at least some of these people will be from different organizations (see Contributors checkpoint). If the project has very limited utility, consider expanding its intended utility so more people will be interested in improving it, and make that larger scope clear to potential contributors.

  • Follow security best practices by performing strict code reviews for every new pull request / merge request.

  • Make "code reviews" mandatory in your repository configuration. (Instructions for GitHub.)

Small Print

Requiring reviews for all changes is infeasible for some projects, such as those with only one active participant. Even a project with multiple active contributors may not have enough active participation to be able to require review of all proposed changes. Projects with a small number of active participants instead sometimes aim for a review of a percentage of proposals (e.g., "at least half of all proposed changes are reviewed").