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

VERIFIED FIXED

Status

()

bugzilla.mozilla.org
General
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: sfink, Assigned: glob)

Tracking

Production

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.)
(Reporter)

Updated

2 years ago
Summary: Setting sec-high or sec-crit should emit boilerplate → Setting sec-high or sec-crit should emit sec-approval boilerplate
We do have bug 1158178 but it hasn't gone anywhere.
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
(Assignee)

Comment 3

2 years ago
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).
(Reporter)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
(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.
Depends on: 1135164
Summary: Setting sec-high or sec-crit should emit sec-approval boilerplate → show a warning near the attachments table for sec-high/sec-crit bugs without sec-approval? on patches
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?
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
sylvestre, are you able to provide the details for comment 7?
Flags: needinfo?(sledru)
Sure, this should be this page:
https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(sledru)
(Assignee)

Updated

2 years ago
Assignee: nobody → glob
(Assignee)

Comment 10

2 years ago
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)

?
Flags: needinfo?(sledru)

(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 !
(Assignee)

Comment 12

2 years ago
(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.
(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
(Assignee)

Comment 14

2 years ago
(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"
Flags: needinfo?(sledru)
(Assignee)

Comment 15

2 years ago
Created attachment 8668807 [details] [diff] [review]
1164063_1.patch
Attachment #8668807 - Flags: review?(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
Attachment #8668807 - Flags: review?(dkl) → review+
(Assignee)

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   42270ff..bbd091b  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
"sec-approval required on patches before landing"
Excellent! Congrat to the bmo team! :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.