Reviewing a Drupal core private security issue

Expectations for the private security issue tracker

The private issue tracker for core can be found at:
https://security.drupal.org/project/issues/drupal
If you are not a member of the Security Team yourself, you will only be able to view issues that the team has granted you access to. All information on these issues, including the number of issues that exist and whether or not security releases are planned, is private information that should never be shared with anyone who does not already have access to the issue.

Issue statuses in private security issues

As in the public queue, issues marked RTBC are awaiting a committer's signoff, and each issue should have separate patch authors, reviewers, and committers (so if you mark the issue RTBC, someone else should sign off). In the private tracker, for core only, the "Ready for SA to be published" issue status basically means "I would have just committed this if it were a public issue" -- in other words, it has final committer signoff. Move RTBC issues to this status when you are ready to sign off on them.

Standards for reviewing private security issues

Follow all the standards you would normally use for reviewing a core RTBC issue. Additionally:

1. Allowed changes

   Think of security issues as being subject to an extra-strict version of the patch release allowed changes. Site owners need to be able to update to a security release immediately without worrying that it will break their site. So, wherever possible, the issue should avoid disruptive changes of any kind, including:

   • Deprecations
   • API additions (even internal API additions)
   • Internal API changes or BC breaks
   • String or UI changes
   • Behavior changes
   • Upgrade paths
   • Any out-of-scope changes, cleanup, refactoring, etc.

   If the issue has anything that could disrupt sites (even edgecase sites or sites not following best practices), push back. Ask contributors to think of ways to avoid the disruptive change (for example, hardcoding \Drupal::service() rather than adding a dependency to a constructor, or hardcoding a string instead of adding a constant, or inlining logic rather than encapsulating it).

   If avoiding disruption means doing something that's not best practice, tag the issue "Needs followup" and document in the IS what the followup for the "right" way would be. These followup issues will be created after the advisory is released (sometimes after a deliberate delay if the issue might disclose specifics not available in the advisory).

2. Handling disruptions

   If the issue requires any disruptions of any kind (if the disruption is integral to resolving the security issue), there must be a release note documented in the issue summary. This also includes anything that would have even a minor change record for a public issue (because we can't post change records until after the release is published). See the 8.5.8 release notes for an example of how to document disruptive changes in the security release.

3. Patch compatibility with supported branches

   Confirm that there are versions of the patch that work on all three current core minor branches: the development branch (e.g. 9.4.x), the production branch (e.g. 9.3.x), and the security-only support branch (e.g. 9.2.x). If the next major branch (e.g. 10.0.x) is open for development, a version for it should be provided as well. The relevant patches for each version should be documented in the issue summary. If one patch works on more than one branch (often the case), then this should be noted.

4. Patch compatibility with previous release tags

   Make sure the production and security-only support patches also work on the most recent tags of those branches! Security releases are created from the most recent tag, not from the branch tip. On rare occasions, commits since the last tag might affect the patch.

   When this happens, if the issue will be released before the next patch release window, it's best to revert the commits that introduce the discrepancy. The patch should be created against the tag; creating a separate patch against HEAD isn't helpful because it will cause a merge conflict when the release is tagged regardless.

   Don't do the revert(s) yet -- document the commit hash(es) that need to be reverted in the IS, and the release manager will do the revert(s) prior to committing the changes to the private repository. (See Creating a Drupal core security release).

5. Successful test runs

   Confirm that the most recent patch has had successful test runs on:

   • The development branch HEAD (e.g. 10.0.x, 9.4.x)
   • The production branch HEAD (e.g. 9.3.x)
   • The latest production support tag (e.g. 9.3.3)
   • The latest security-only support tag (e.g. 9.2.11)

6. Combined patches with test coverage

   Confirm that there is a version of the patch that has test coverage for the fix and any known exploits.

7. Test-only patches and their output

   Confirm that there is a test-only patch that fails and that the exact output of running the test(s) locally is documented in the issue summary (since the private testrunner does not easily expose this information on the issue the way that Drupal.org does and not everyone can access the detailed logs).

8. Manual testing

   Confirm that the final patch has been manually tested to ensure it resolves all known exploits.

9. Fix-only patches

   Confirm that there is a version of the patch that excludes any exploit tests if the test adds information beyond what's immediately obvious from the fix itself. This is the version that will be used on the security window. Tests containing direct exploits are only released in a public followup two months after the advisory, so that they can't be used by malicious actors to derive an exploit before most sites manage to update.

10. Draft security advisory (SA)

    Make sure there is a draft security advisory. Review it for accuracy and clarity (just like you would for a public issue's change record).

11. Issue credit

    Grant credit for the issue using the same issue credit guidelines we use for public issues. There are plain checkboxes at the bottom of the private issue to credit commenters. Data from this field will automatically be populated into the public security advisory (which in turn is used in contribution credit on d.o) and will also be used in the commit message for the advisory.