1164063 - show a warning near the attachments table for sec-high/sec-crit bugs without sec-approval? on patches

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Steve Fink [:sfink] [:s:]]

I often forget to request sec-approval (from bug 786836) on security bugs before landing. It would be very helpful for me if setting sec-high or sec-crit would autocomment something like

"This bug has been marked security sensitive. Please request sec-approval? on any patches before landing, in order to avoid prematurely exposing security vulnerabilities."

(Just an example; I imagine the security folks are the ones who would be writing the text.)

Comment 1

[Al Billings [:abillings - ex-MoCo]]

We do have bug 1158178 but it hasn't gone anywhere.

Comment 2

[David Lawrence [:dkl]]

sec-high and sec-crit are keywords which we do not currently have the capability to auto-fill the comment textarea when added. We have that for normal flags such as sec-review and sec-approval. We could do either:

1. Add hook code on the backend to add the comment when a bug change is submitted containing the keywords mentioned.
2. Add functionality to auto-fill the comment text in the UI when a keyword is added similar to the flags. 
3. Migrate from using sec-* keywords to use flags and take advantage of what is already there.

1 and 3 are the easiest depending on which path you want to go. 2 would be the most work and is least likely to happen soon.

dkl

Comment 3

[:glob ✱]

i have concerns about the usefulness of adding a comment at the time the keyword is added.
in the sec bugs i've seen the security rating has been set early in the bug's life, so it's likely by the time the patches are ready to be committed the comment with the reminder may be lost/forgotten.


instead i propose adding a warning message near the attachments table for bugs with a sec-high/sec-critical keywords, patches, but do not have sec-approval requested or set.

eg. https://www.dropbox.com/s/btc1gzw55v8v4gx/Screenshot%202015-05-13%2011.28.03.png
(i would expect "sec-approval" to link to a wiki page describing the process).

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Byron Jones ‹:glob› from comment #3)
> i have concerns about the usefulness of adding a comment at the time the
> keyword is added.
> in the sec bugs i've seen the security rating has been set early in the
> bug's life, so it's likely by the time the patches are ready to be committed
> the comment with the reminder may be lost/forgotten.
> 
> instead i propose adding a warning message near the attachments table for
> bugs with a sec-high/sec-critical keywords, patches, but do not have
> sec-approval requested or set.

That's fair, though it wouldn't work for my personal pattern, since I normally do most of my patch manipulation from the command line and therefore do not see the attachments table. (I do read the bugmail, though.) Then again, I think abillings is right and bug 1158178 is the real fix for scumbags like me.

> eg.
> https://www.dropbox.com/s/btc1gzw55v8v4gx/Screenshot%202015-05-13%2011.28.03.
> png
> (i would expect "sec-approval" to link to a wiki page describing the
> process).

I like it, and think it would be independently useful. Though I'd make it a somewhat scarier color. Orange or something.

Comment 5

[:glob ✱]

(In reply to Steve Fink [:sfink, :s:] from comment #4)
> Then again, I think abillings is right and bug 1158178 is the real fix for
> scumbags like me.

+1

> I like it, and think it would be independently useful.

the framework for that is happening in bug 1135164, updating summary and marking dependency.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

I like this too. The release management team is talking about similar ideas in bug 1158178. Could we do this (warning developers) and also make it impossible to set approval flags for uplifts to + on these bugs, when they don't have sec-approval:+ set?

Comment 7

[:glob ✱]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #6)
> I like this too. The release management team is talking about similar ideas
> in bug 1158178. Could we do this (warning developers) and also make it
> impossible to set approval flags for uplifts to + on these bugs, when they
> don't have sec-approval:+ set?

can you point me to a wiki page that describe the use of the flags and when these limitations should apply?
i'd probably want to break the flag limiting into a separate bug from this, but that documentation helps both causes.

Comment 8

[:glob ✱]

sylvestre, are you able to provide the details for comment 7?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Sure, this should be this page:
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 10

[:glob ✱]

more questions ..

if a sec-critical bug has 2 non-obsolete patches, do both patches require sec-approval, or is it ok if any of the patches have approval?


the docs has:
> 1. The bug has a sec-low, sec-moderate, sec-other, or sec-want rating.
> OR
> 2. The bug is a recent regression on mozilla-central (this means that
> the specific regressing check-in has been identified on mozilla-central)

i'd like a sanity check on the logic i'd need to implement... we'd warn if

- the bug has 'sec-high' or 'sec-critical' keywords set
AND
- there aren't any status-firefox tracking flags set
  OR
  all status-firefox tracking flags except the latest are set to 'unaffected'
AND
- there is at least one patch without sec-approval set (to any value)

?

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

(In reply to Byron Jones ‹:glob› from comment #10)

> i'd like a sanity check on the logic i'd need to implement... we'd warn if

> - there aren't any status-firefox tracking flags set
>   OR
>   all status-firefox tracking flags except the latest are set to 'unaffected'


I think this should be 

OR
any of the current status-firefox tracking flags that aren't the latest are set to 'affected'

As I understand it, if we've said that all the other current branches are unaffected, and only one branch is affected, the patch doesn't need sec-approval.  If we know more than one branch is affected, or if we aren't sure if multiple branches are affected or not (ie, status is not set) then we should be determining which branches are affected before landing anywhere.     

The problem with this logic is that sometimes we set 'affected' for the current nightly, and maybe unaffected for the one before it, but no one bothers to say that every current previous branch is unaffected, because we only need to establish a demarcation point for when the regression happened. So we may need to add in another OR like so,

OR 
a branch is marked 'affected' and the one preceding it (one number lower) has no value set

Hope that makes sense, it's late and I'm tired !

Comment 12

[:glob ✱]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> any of the current status-firefox tracking flags that aren't the latest are
> set to 'affected'

ok, that sounds easier.

> The problem with this logic is that sometimes we set 'affected' for the
> current nightly, and maybe unaffected for the one before it, but no one
> bothers to say that every current previous branch is unaffected[...]

that's fine -- by your logic as long as any of the not-nightly flags are set to 'affected' then we'll warn.

it doesn't have to be perfect initially as it's just a warning; we can always refine the logic over time to match real world usage.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)

> As I understand it, if we've said that all the other current branches are
> unaffected, and only one branch is affected, the patch doesn't need
> sec-approval. 

No, this is not the rule. The rule is if ONLY trunk is affected, then it doesn't need sec-approval.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 14

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #10)
> if a sec-critical bug has 2 non-obsolete patches, do both patches require
> sec-approval, or is it ok if any of the patches have approval?

i'll go with "we're good as long as at least one patch has sec-approval"

Comment 15

[:glob ✱]

Attachment: 1164063_1.patch

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 8668807 [details] [diff] [review]
1164063_1.patch

Review of attachment 8668807 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

::: extensions/BMO/Extension.pm
@@ +851,5 @@
> +        WHERE       => {
> +            'name like ?' => 'cf_status_firefox%',
> +        },
> +    });
> +    $flags = [ grep { $_->name =~ /^cf_status_firefox/ } @$flags ];

This line seems redundant since you already have the WHERE above

Comment 17

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #16)
> ::: extensions/BMO/Extension.pm
> @@ +851,5 @@
> > +        WHERE       => {
> > +            'name like ?' => 'cf_status_firefox%',
> > +        },
> > +    });
> > +    $flags = [ grep { $_->name =~ /^cf_status_firefox/ } @$flags ];
> 
> This line seems redundant since you already have the WHERE above

it does seem that way, but set flags are added after the sql query is executed so we need that grep.
i'll add a comment.

Comment 18

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   42270ff..bbd091b  master -> master

Comment 19

[Sylvestre Ledru [:Sylvestre]]

"sec-approval required on patches before landing"
Excellent! Congrat to the bmo team! :)