security-approval.rst (7846B)
1 Security Bug Approval Process 2 ============================= 3 4 How to fix a core-security bug in Firefox - developer guidelines 5 ---------------------------------------------------------------- 6 7 Follow these security guidelines if you’re involved in reviewing, 8 testing and landing a security patch: 9 :ref:`Fixing Security Bugs`. 10 11 Purpose: don't 0-day ourselves 12 ------------------------------ 13 14 People watch our check-ins. They may be able to start exploiting our 15 users before we can get an update out to them if 16 17 - the patch is an obvious security fix (bounds check, kungFuDeathGrip, 18 etc.) 19 - the check-in comment says "security fix" or includes trigger words 20 like "exploitable", "vulnerable", "overflow", "injection", "use after 21 free", etc. 22 - comments in the code mention those types of things or how someone 23 could abuse the bug 24 - the check-in contains testcases that show exactly how to trigger the 25 vulnerability 26 27 Principle: assume the worst 28 --------------------------- 29 30 - If there's no rating we assume it could be critical 31 - If we don't know the regression range we assume it needs porting to 32 all supported branches 33 34 Process for Security Bugs (Developer Perspective) 35 ------------------------------------------------- 36 37 Filing / Managing Bugs 38 ~~~~~~~~~~~~~~~~~~~~~~ 39 40 - Try whenever possible to file security bugs marked as such when 41 filing, instead of filing them as open bugs and then closing later. 42 This is not always possible, but attention to this, especially when 43 filing from crash-stats, is helpful. 44 - It is _ok_ to link security bugs to non-security bugs with Blocks, 45 Depends, Regressions, or See Also. Users with the editbugs permission 46 will be able to see the reference, but not view a restricted bug or 47 its summary. Users without the permission will not be able to see the link. 48 For critical severity bugs where even that seems problematic, consider 49 mentioning the bug in a comment on the security bug instead. We can always 50 fill in the links later after the fix has shipped. 51 52 Developing the Patch 53 ~~~~~~~~~~~~~~~~~~~~ 54 55 - Comments in the code should not mention a security issue is being 56 fixed. Don’t paint a picture or an arrow pointing to security issues 57 any more than the code changes already do. 58 - Do not push to Try servers if possible: this exposes the security 59 issues for these critical and high rated bugs to public viewing. In 60 an ideal case, testing of patches is done locally before final 61 check-in to mozilla-central. 62 - If pushing to Try servers is necessary, **do not include the bug 63 number in the patch**. Ideally, do not include tests in the push as 64 the tests can illustrate the exact nature of the security problem 65 frequently. 66 - If you must push to Try servers, with or without tests, try to 67 obfuscate what this patch is for. Try to push it with other, 68 non-security work, in the same area. 69 70 Request review of the patch in the same process as normal. After the 71 patch has been reviewed you will request sec-approval as needed. See 72 :ref:`Fixing Security Bugs` 73 for more examples/details of these points. 74 75 Preparing the patch for landing 76 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 77 78 See :ref:`Fixing Security Bugs` 79 for more details. 80 81 On Requesting sec-approval 82 ~~~~~~~~~~~~~~~~~~~~~~~~~~ 83 84 For security bugs with no sec- severity rating assume the worst and 85 follow the rules for sec-critical. During the sec-approval process we 86 will notice it has not been rated and rate it during the process. 87 88 Core-security bug fixes can be landed by a developer without any 89 explicit approval if: 90 91 | **A)** The bug has a sec-low, sec-moderate, sec-other, or sec-want 92 rating. 93 | **or** 94 | **B)** The bug is a recent regression on mozilla-central. This means 95 96 - A specific regressing check-in has been identified 97 - The developer can (**and has**) marked the status flags for ESR and 98 Beta as "unaffected" 99 - We have not shipped this vulnerability in anything other than a 100 nightly build 101 102 If it meets the above criteria, developers do not need to ask for sec-approval. 103 104 In all other cases, developers should ask for sec-approval. When a patch is 105 ready to be landed, click on the "Details" link for it in the Bugzilla 106 attachment table (not directly on phabricator at time of writing), then set the 107 sec-approval flag to '?'. 108 109 If developers are unsure about a bug and it has a patch ready, just 110 request sec-approval anyway and move on. Don't overthink it! 111 112 An automatic nomination comment will be added to bugzilla when 113 sec-approval is set to '?'. The questions in this need to be filled out 114 as best as possible when sec-approval is requested for the patch. 115 116 It is as follows (courtesy of Dan Veditz):: 117 118 [Security approval request comment] 119 How easily can the security issue be deduced from the patch? 120 Do comments in the patch, the check-in comment, or tests included in 121 the patch paint a bulls-eye on the security problem? 122 Which older supported branches are affected by this flaw? 123 If not all supported branches, which bug introduced the flaw? 124 Do you have backports for the affected branches? If not, how 125 different, hard to create, and risky will they be? 126 How likely is this patch to cause regressions; how much testing does 127 it need? 128 129 This is similar to the ESR approval nomination form and is meant to help 130 us evaluate the risks around approving the patch for checkin. 131 132 When the bug is approved for landing, the sec-approval flag will be set 133 to '+' with a comment from the approver to land the patch. At that 134 point, land it according to instructions provided.. 135 136 This will allow us to control when we can land security bugs without 137 exposing them too early and to make sure they get landed on the various 138 branches. 139 140 If you have any questions or are unsure about anything in this document 141 contact us on Slack in the #security channel or the current 142 sec-approvers Dan Veditz and Tom Ritter. 143 144 Process for Security Bugs (sec-approver Perspective) 145 ---------------------------------------------------- 146 147 The security assurance team and release management will have their own 148 process for approving bugs: 149 150 #. The Security assurance team goes through sec-approval ? bugs daily 151 and approves low risk fixes for central (if early in cycle). 152 Developers can also ping the Security Assurance Team (specifically 153 Tom Ritter & Dan Veditz) in #security on Slack when important. 154 155 #. If a bug lacks a security-rating one should be assigned - possibly 156 in coordination with the (other member of) the Security Assurance 157 Team 158 159 #. Security team marks tracking flags to ? for all affected versions 160 when approved for central. (This allows release management to decide 161 whether to uplift to branches just like always.) 162 #. Weekly security/release management triage meeting goes through 163 sec-approval + and ? bugs where beta and ESR is affected, ? bugs with 164 higher risk (sec-high and sec-critical), or ? bugs near end of cycle. 165 166 Options for sec-approval including a logical combination of the 167 following: 168 169 - Separate out the test and comments in the code into a followup commit 170 we will commit later. 171 - Remove the commit message and place it in the bug or comments in a 172 followup commit. 173 - Please land it bundled in with another commit 174 - Land today 175 - Land today, land the tests after 176 - Land closer to the release date 177 - Land in Nightly to assess stability 178 - Land today and request uplift to all branches 179 - Request uplift to all branches and we'll land as close to shipping as 180 permitted 181 - Chemspill time 182 183 The decision process for which of these to choose is perceived risk on 184 multiple axes: 185 186 - ease of exploitation 187 - reverse engineering risk 188 - stability risk 189 190 The most common choice is: not much stability risk, not an immediate 191 reverse engineering risk, moderate to high difficulty of exploitation: 192 "land whenever".