Bug 1628252 - add 'security bug fixing' and 'sec approval' documents to soruce docs r=dveditz
authorFrederik Braun <fbraun@mozilla.com>
Tue, 28 Apr 2020 15:07:08 +0000
changeset 526627 262c8adb52655e2028595d9838903b8b5a0a87da
parent 526626 7187db5fae0aee2a78cab2f0dba906e8a2ecee57
child 526628 964f475cc66e38351ef94a47a5eec4b160a96789
push id114400
push userfbraun@mozilla.com
push dateWed, 29 Apr 2020 07:15:49 +0000
treeherderautoland@262c8adb5265 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdveditz
bugs1628252
milestone77.0a1
first release with
nightly linux32
262c8adb5265 / 77.0a1 / 20200429095105 / files
nightly linux64
262c8adb5265 / 77.0a1 / 20200429095105 / files
nightly mac
262c8adb5265 / 77.0a1 / 20200429095105 / files
nightly win32
262c8adb5265 / 77.0a1 / 20200429095105 / files
nightly win64
262c8adb5265 / 77.0a1 / 20200429095105 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1628252 - add 'security bug fixing' and 'sec approval' documents to soruce docs r=dveditz Differential Revision: https://phabricator.services.mozilla.com/D70176
docs/bug-mgmt/processes/fixing-security-bugs.rst
docs/bug-mgmt/processes/security-approval.rst
new file mode 100644
--- /dev/null
+++ b/docs/bug-mgmt/processes/fixing-security-bugs.rst
@@ -0,0 +1,159 @@
+Fixing Security Bugs
+====================
+
+A bug has been reported as security-sensitive in Bugzilla and received a
+security rating.
+
+If this bug is private - which is most likely for a reported security
+bug - **the process for patching is slightly different than the usual
+process for fixing a bug**.
+
+Here are security guidelines to follow if you’re involved in reviewing,
+testing and landing a security patch. See
+:ref:`Security Bug Approval Process`
+for more details about how to request sec-approval and land the patch.
+
+Keeping private information private
+-----------------------------------
+
+A security-sensitive bug in Bugzilla means that all information about
+the bug except its ID number are hidden. This includes the title,
+comments, reporter, assignee and CC’d people.
+
+A security-sensitive bug usually remains private until a fix is shipped
+in a new release, **and after a certain amount of time to ensure that a
+maximum number of users updated their version of Firefox**. Bugs are
+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
+fix (0-day).
+
+During a normal process, information about the nature of bug can be
+accessed through:
+
+-  Bug comments (Bugzilla, GitHub issue)
+-  Commit message (visible on Bugzilla, tree check-ins and test servers)
+-  Code comments
+-  Test cases
+-  Bug content can potentially be discussed on public IRC/Slack channels
+   and mailing list emails.
+
+When patching for a security bug, you’ll need to be mindful about what
+type of information you share and where.
+
+In commit messages
+~~~~~~~~~~~~~~~~~~
+
+People are watching code check-ins, so we want to avoid sharing
+information which would disclose or help finding a vulnerability too
+easily before we shipped the fix to our users. This includes:
+
+-  The **nature of the vulnerability** (overflow, use-after-free, XSS,
+   CSP bypass...)
+-  **Ways to trigger and exploit that vulnerability**
+   - 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…)
+   -  **Security approver’s name** in a commit message.
+-  The Firefox versions and components affected by the vulnerability.
+-  Patches with an obvious fix.
+
+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.**
+
+   -  Mention the bugs in comment of the private bug instead.
+-  Do not comment sensitive information in public related bugs.
+-  Also be careful about who you give bug access to: **double check
+   before CC’ing the wrong person or alias**.
+
+On IRC, Slack channels, GitHub issues, mailing lists: If you need to
+discuss about a security bug, use a private channel (protected with a
+password or with proper right access management)
+
+During Development
+------------------
+
+Testing sec-high and sec-critical bugs
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Pushing to Try servers requires Level 1 Commit access but **content
+viewing is publicly accessible**.
+
+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.
+
+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.
+
+Obfuscating a security patch
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If your security patch looks obvious because of the code it contains
+(e.g. a one-line fix), or if you really need to push to Try servers,
+**consider integrating your security-related patch to non-security work
+in the same area**. And/or pretend it is related to something else, like
+some performance improvement or a correctness fix. **Definitely don't
+include the bug number in the commit message.** This will help making
+the security issue less easily identifiable. (The absolute ban against
+"Security through Obscurity" is in relation to cryptographic systems. In
+other situations you still can't *rely* on obscurity but it can
+sometimes buy you a little time. In this context we need to get the
+fixes into the hands of our users faster than attackers can weaponize
+and deploy attacks and a little extra time can help.)
+
+Requesting sec-approval
+~~~~~~~~~~~~~~~~~~~~~~~
+
+See :ref:`Security Bug Approval Process`
+for more details
+
+Landing tests
+~~~~~~~~~~~~~
+
+Tests can be landed **after the release has gone live** and **not before
+at least 4 weeks following that release**.
+
+The exception is if a security issue has never been shipped in a release
+build and has been fixed in all development branches.
+
+Making a security bug public
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This is the responsibility of the security management team.
+
+Essentials
+----------
+
+-  **Do not disclose any information about the vulnerability before a
+   release with a fix has gone live for enough time for users to update
+   their software**.
+
+   -  This includes code comments, commit messages, tests, public
+      communication channels.
+
+-  If any doubt: '''request sec-approval? '''
+-  If any doubt: **needinfo security folks**.
+-  **If there’s no rating, assume the worst and treat the bug as
+   sec-critical**.
+
+Documentation & Contacts
+------------------------
+
+- `Normal process for submitting a patch <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch>`__
+- `How to file a security bug <https://wiki.mozilla.org/Security/Fileabug>`__
+- `Handling Mozilla security bugs (policy) <https://www.mozilla.org/en-US/about/governance/policies/security-group/bugs/>`__
+- `Security Bug Approval Process <security-approval>`__
+- `Contacting the Security team(s) at Mozilla: <https://wiki.mozilla.org/Security>`__
new file mode 100644
--- /dev/null
+++ b/docs/bug-mgmt/processes/security-approval.rst
@@ -0,0 +1,219 @@
+Security Bug Approval Process
+=============================
+
+How to fix a core-security bug in Firefox - developer guidelines
+----------------------------------------------------------------
+
+Follow these security guidelines if you’re involved in reviewing,
+testing and landing a security patch:
+:ref:`Fixing Security Bugs`.
+
+Purpose: don't 0-day ourselves
+------------------------------
+
+People watch our check-ins. They may be able to start exploiting our
+users before we can get an update out to them if
+
+-  the patch is an obvious security fix (bounds check, kungFuDeathGrip,
+   etc.)
+-  the check-in comment says "security fix" or includes trigger words
+   like "exploitable", "vulnerable", "overflow", "injection", "use after
+   free", etc.
+-  comments in the code mention those types of things or how someone
+   could abuse the bug
+-  the check-in contains testcases that show exactly how to trigger the
+   vulnerability
+
+Principle: assume the worst
+---------------------------
+
+-  If there's no rating we assume it could be critical
+-  If we don't know the regression range we assume it needs porting to
+   all supported branches
+
+Process for Security Bugs (Developer Perspective)
+-------------------------------------------------
+
+Filing / Managing Bugs
+~~~~~~~~~~~~~~~~~~~~~~
+
+-  Try whenever possible to file security bugs marked as such when
+   filing, instead of filing them as open bugs and then closing later.
+   This is not always possible, but attention to this, especially when
+   filing from crash-stats, is helpful.
+-  Avoid linking it to non-security bugs with Blocks, Depends,
+   Regressions, or See Also, especially if those bugs may give a hint to
+   the sort of security issue involved. Mention the bug in a comment on
+   the security bug instead. We can always fill in the links later after
+   the fix has shipped.
+
+Developing the Patch
+~~~~~~~~~~~~~~~~~~~~
+
+-  Comments in the code should not mention a security issue is being
+   fixed. Don’t paint a picture or an arrow pointing to security issues
+   any more than the code changes already do.
+-  Avoid linking it to non-security bugs with Blocks, Depends, or See
+   Also, especially if those bugs may give a hint to the sort of
+   security issue involved. Mention the bug in a comment on the security
+   bug instead. We can always fill in the links later after the fix has
+   shipped.
+-  Do not push to Try servers if possible: this exposes the security
+   issues for these critical and high rated bugs to public viewing. In
+   an ideal case, testing of patches is done locally before final
+   check-in to mozilla-central.
+-  If pushing to Try servers is necessary, **do not include the bug
+   number in the patch**. Ideally, do not include tests in the push as
+   the tests can illustrate the exact nature of the security problem
+   frequently.
+-  If you must push to Try servers, with or without tests, try to
+   obfuscate what this patch is for. Try to push it with other,
+   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
+:ref:`Fixing Security Bugs`
+for more examples/details of these points.
+
+On Requesting sec-approval
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+For security bugs with no sec- severity rating assume the worst and
+follow the rules for sec-critical. During the sec-approval process we
+will notice it has not been rated and rate it during the process.
+
+Core-security bug fixes can be landed by a developer without any
+explicit approval if:
+
+| **A)** The bug has a sec-low, sec-moderate, sec-other, or sec-want
+  rating.
+|    **or**
+| **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"
+-  We have not shipped this vulnerability in anything other than a
+   nightly build
+
+If it meets the above criteria, check that patch in.
+
+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).
+
+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!
+
+An automatic nomination comment will be added to bugzilla when
+sec-approval is set to '?'. The questions in this need to be filled out
+as best as possible when sec-approval is requested for the patch.
+
+It is as follows (courtesy of Dan Veditz)::
+
+   [Security approval request comment]
+   How easily can the security issue be deduced from the patch?
+   Do comments in the patch, the check-in comment, or tests included in
+   the patch paint a bulls-eye on the security problem?
+   Which older supported branches are affected by this flaw?
+   If not all supported branches, which bug introduced the flaw?
+   Do you have backports for the affected branches? If not, how
+   different, hard to create, and risky will they be?
+   How likely is this patch to cause regressions; how much testing does
+   it need?
+
+This is similar to the ESR approval nomination form and is meant to help
+us evaluate the risks around approving the patch for checkin.
+
+When the bug is approved for landing, the sec-approval flag will be set
+to '+' with a comment from the approver to land the patch. At that
+point, land it according to instructions provided..
+
+This will allow us to control when we can land security bugs without
+exposing them too early and to make sure they get landed on the various
+branches.
+
+If you have any questions or are unsure about anything in this document
+contact us on Slack in the #security channel or the current
+sec-approvers Dan Veditz and Tom Ritter.
+
+Process for Security Bugs (sec-approver Perspective)
+----------------------------------------------------
+
+The security assurance team and release management will have their own
+process for approving bugs:
+
+#. The Security assurance team goes through sec-approval ? bugs daily
+   and approves low risk fixes for central (if early in cycle).
+   Developers can also ping the Security Assurance Team (specifically
+   Tom Ritter & Dan Veditz) in #security on Slack when important.
+
+   #. If a bug lacks a security-rating one should be assigned - possibly
+      in coordination with the (other member of) the Security Assurance
+      Team
+
+#. Security team marks tracking flags to ? for all affected versions
+   when approved for central. (This allows release management to decide
+   whether to uplift to branches just like always.)
+#. Weekly security/release management triage meeting goes through
+   sec-approval + and ? bugs where beta and ESR is affected, ? bugs with
+   higher risk (sec-high and sec-critical), or ? bugs near end of cycle.
+
+Options for sec-approval including a logical combination of the
+following:
+
+-  Separate out the test and comments in the code into a followup commit
+   we will commit later.
+-  Remove the commit message and place it in the bug or comments in a
+   followup commit.
+-  Please land it bundled in with another commit
+-  Land today
+-  Land today, land the tests after
+-  Land closer to the release date
+-  Land in Nightly to assess stability
+-  Land today and request uplift to all branches
+-  Request uplift to all branches and we'll land as close to shipping as
+   permitted
+-  Chemspill time
+
+The decision process for which of these to choose is perceived risk on
+multiple axes:
+
+-  ease of exploitation
+-  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"
\ No newline at end of file