Bug 1736003 - tweak security patch landing guidance, r=dveditz DONTBUILD

Differential Revision: https://phabricator.services.mozilla.com/D128585

docs/bug-mgmt/processes/fixing-security-bugs.rst

@@ -27,8 +27,8 @@ usually made public after 6 months and a couple of releases.
 
 From the moment a security bug has been privately reported to the moment
 a fix is shipped and the bug is set public, all information about that
-bug need to be handled carefully in order to avoid an unmitigated
-vulnerability to be known and exploited out there before we release a
+bug needs to be handled carefully in order to avoid an unmitigated
+vulnerability becoming known and exploited before we release a
 fix (0-day).
 
 During a normal process, information about the nature of bug can be
@@ -57,7 +57,9 @@ easily before we shipped the fix to our users. This includes:
    - In commit messages, code comments and test cases.
 -  The fact that a bug / commit is security-related:
 
-   -  **Trigger words** in the commit message or code comments such as "security", "exploitable" or the nature of a security vulnerability (overflow, use-after-free…)
+   -  **Trigger words** in the commit message or code comments such as
+      "security", "exploitable", or the nature of a security vulnerability
+      (overflow, use-after-free…)
    -  **Security approver’s name** in a commit message.
 -  The Firefox versions and components affected by the vulnerability.
 -  Patches with an obvious fix.
@@ -68,9 +70,9 @@ In Bugzilla and other public channels
 In addition to commits, you’ll need to be mindful of not disclosing
 sensitive information about the bug in public places, such as Bugzilla:
 
--  **Do not add public bugs in the “duplicate”, “depends on”, “blocks”
-   or “see also” section if these bugs could give hints about the nature
-   of the security issue.**
+-  **Do not add public bugs in the “duplicate”, “depends on”, “blocks”,
+   “regression”, “regressed by”, or “see also” section if these bugs 
+   could give hints about the nature of the security issue.**
 
    -  Mention the bugs in comment of the private bug instead.
 -  Do not comment sensitive information in public related bugs.
@@ -84,8 +86,8 @@ password or with proper right access management)
 During Development
 ------------------
 
-Testing sec-high and sec-critical bugs
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Testing security bugs
+~~~~~~~~~~~~~~~~~~~~~
 
 Pushing to Try servers requires Level 1 Commit access but **content
 viewing is publicly accessible**.
@@ -94,6 +96,13 @@ As much as possible, **do not push to Try servers**. Testing should be
 done locally before checkin in order to prevent public disclosing of the
 bug.
 
+Because of the public visibility, pushing to Try has all the same concerns
+as committing the patch. Please heed the concerns in the
+:ref:`landing-your-patch` section before thinking about it, and check with
+the security team for an informal "sec-approval" before doing so.
+
+**Do not push the bug's own vulnerability testcase to Try.**
+
 If you need to push to Try servers, make sure your tests don’t disclose
 what the vulnerability is about or how to trigger it. Do not mention
 anywhere it is security related.
@@ -120,11 +129,60 @@ Requesting sec-approval
 See :ref:`Security Bug Approval Process`
 for more details
 
+.. _landing-your-patch:
+
+Landing your patch (with or without sec-approval)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Before asking for sec-approval or landing, ensure your patch does not disclose
+information about the security vulnerability unnecessarily. Specifically:
+
+#. The patch commit message and its contents should not mention security,
+   security bugs, or sec-approvers.
+   Note that you can alter the commit message directly in phabricator,
+   if that's the only thing you need to do - you don't need to amend your
+   local commit and re-push it.
+   While comprehensive commit messages are generally encouraged; they
+   should be omitted for security bugs and instead be posted in the bug
+   (which will eventually become public.)
+#. Separate out tests into a separate commit.
+   **Do not land tests when landing the patch. Remember we don’t want
+   to 0-day ourselves!** This includes when pushing to try.
+
+   -  Tests should only be checked in later, after an official Firefox
+      release that contains the fix has been live for at least
+      four weeks. For example, if Firefox 53
+      contains a security issue that affects the world and that issue is
+      fixed in 54, tests for this fix should not be checked in
+      until four weeks after 54 goes live.
+
+      The exception to this is if there is a security issue that doesn't
+      affect any release branches, only mozilla-central and/or other
+      development branches.  Since the security problem was never
+      released to the world, once the bug is fixed in all affected
+      places, tests can be checked in to the various branches.
+   -  There are two main techniques for remembering to check in the
+      tests later:
+
+     a.  clone the sec bug into a separate "task" bug **that is also
+         in a security-sensitive group to ensure it's not publicly visible**
+         called something like "land tests for bug xxxxx" and assign to
+         yourself. It should get a "sec-other" keyword rating.
+
+         Tip: In phabricator, you can change the bug linked to
+         a commit with tests if the tests were already separate, while keeping
+         the previously granted review, meaning you can just land the patch
+         when ready, rather than having your reviewer and you have to remember
+         what this was about a month or two down the line.
+     b.  Or, set the "in-testsuite" flag to "?", and later set it to "+"
+         when the tests get checked in.
+
+
 Landing tests
 ~~~~~~~~~~~~~
 
-Tests can be landed **after the release has gone live** and **not before
-at least 4 weeks following that release**.
+Tests can be landed **once the release containing fixes has been live
+at least 4 weeks**.
 
 The exception is if a security issue has never been shipped in a release
 build and has been fixed in all development branches.

docs/bug-mgmt/processes/security-approval.rst

@@ -71,10 +71,16 @@ Developing the Patch
    non-security work, in the same area.
 
 Request review of the patch in the same process as normal. After the
-patch has received an r+ you will request sec-approval. See
+patch has been reviewed you will request sec-approval as needed. See
 :ref:`Fixing Security Bugs`
 for more examples/details of these points.
 
+Preparing the patch for landing
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+See :ref:`Fixing Security Bugs`
+for more details.
+
 On Requesting sec-approval
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -91,48 +97,17 @@ explicit approval if:
 | **B)** The bug is a recent regression on mozilla-central. This means
 
 -  A specific regressing check-in has been identified
--  The developer can (**and has**) marked the status flags for ESR,
-   Beta, and Aurora as "unaffected"
+-  The developer can (**and has**) marked the status flags for ESR and
+   Beta as "unaffected"
 -  We have not shipped this vulnerability in anything other than a
    nightly build
 
-If it meets the above criteria, check that patch in.
+If it meets the above criteria, developers do not need to ask for sec-approval.
 
-Otherwise, if the bug has a patch \*and\* is sec-high or sec-critical,
-the developer should prepare the patch for sec-approval. This entails:
-
--  Commit should occur without specific mention of security, security
-   bugs, or sec-approvers if possible. While comprehensive commit
-   messages are generally encouraged; they should be omitted for
-   security bugs and instead be posted in the bug (which will eventually
-   become public.)
--  Separate out tests into a separate commit. **Do not commit tests when
-   checking in** when the security bug fix is initially checked-in.
-   **Remember we don’t want to 0-day ourselves!**
-
-   -  Tests should only be checked in later, after an official Firefox
-      release that contains the fix has gone live and not for at least
-      four weeks following that release. For example, if Firefox 53
-      contains a fix for a security issue that affects the world and is
-      then fixed in 54, tests for this fix should not be checked in
-      until four weeks after 54 goes live. The exception to this is if
-      there is a security issue that hasn’t shipped in a release build
-      and it is being fixed on multiple development branches (such as
-      mozilla-central and beta). Since the security problem was never
-      released to the world, once the bug is fixed in all affected
-      places, tests can be checked in to the various branches.
-   -  There are two main techniques for remembering to check in the
-      tests later:
-
-      -  clone the sec bug into a hidden "task" bug "land tests for bug
-         xxxxx" and assign to yourself. It should get a "sec-other"
-         keyword rating.
-      -  Or, set the "in-testsuite" flag to "?", and later set it to "+"
-         when the tests get checked in.
-
-Following that, set the sec-approval flag to '?' on the patch when it is
-ready to be checked into mozilla-central (or elsewhere if it is branch
-only).
+In all other cases, developers should ask for sec-approval. 
+Set the sec-approval flag to '?' on the patch when it is ready to be landed.
+You will find these flags in Bugzilla using the "Details" links in the
+Bugzilla attachment table (not directly on phabricator at time of writing).
 
 If developers are unsure about a bug and it has a patch ready, just
 request sec-approval anyway and move on. Don't overthink it!
@@ -215,5 +190,7 @@ multiple axes:
 -  reverse engineering risk
 -  stability risk
 
-The most common choice is: not much stability risk, not an immediate RE
-risk, moderate to high difficulty of exploitation: "land whenever"
+The most common choice is: not much stability risk, not an immediate
+reverse engineering risk, moderate to high difficulty of exploitation:
+"land whenever".
+