179510 - Request notifications dont take group restrictions fully into account

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Joel Peshkin]

This is a "minor" security issue.

When a security bug (or a private attachment) is flagged with a request and that
request is copied automatically to a list (such as review requests going to
reviewers), the notification of the request does not take into account the
privileges of the recipient.

To further complicate things, the requestee may not have access to the very
thing they are being requested to act upon.

How do we want this to work?

Comment 1

[Bradley Baetz (:bbaetz)]

- we shouldn't cc the request cclist unless the bug has no restrictions (or
maybe we can try to find an account, and use those)?

We should always mail the requestee, but warn when making a request that the
person can't see the bug, asking for the user to add the person tho the cclist,
or sometihng,

Comment 2

[Joel Peshkin]

Actually, we could just call CanSeeBug() for each member of CClist just as
processmail does.

Comment 3

[Bradley Baetz (:bbaetz)]

No, we can't - the cclist is a comma separated string; we don't have accounts
for that.

Comment 4

[Bradley Baetz (:bbaetz)]

This also leaks the summary, which people aren't going to want for security bugs
sent to the reviewers list.

So hows this:

- When requesting a bug from a particular person, if they can't see the
attachment (not the bug; think insiders here), then we should have an
intermediate confirmation screen, giving the option (ie checkbox) of adding to
the cclist (if cclist_accessible is true). This should be done for each person
we're requesting from.

If cclist_accessible is not on, then we warn about that, but don't give an
option to automagically set it.

- When ccing to a list, if we can't find the user, then consider them as being
in no groups. Otherwise, just don't send the mail. There should be no prompt in
this case.

Comment 5

[Joel Peshkin]

I think this can be kept simple....

Drop anyone on the flag's CC list that lacks the group permission for the bug or
the attachment.

Always send the notice to the requestee

Comment 6

[Joel Peshkin]

(to clarify comment 5)
CC list members without accounts should be treated as bbaetz suggests, no access
to any groups.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress: half a patch

Here's a patch that throws a (nonexistent) error if the requestee is
unauthorized to see the bug.  Comments, suggestions, and help are all welcome.

Comment 8

[Bradley Baetz (:bbaetz)]

You've got the 'pending' => '?' patch here, at least, but its more the cc list
which is the interesting part of this bug.

Comment 9

[Joel Peshkin]

Before the CanSeeBug() is called, we need to make sure that ConfirmGroup() gets
called.  (For all the users other than the logged-in user)

If it is likely that the same user would also be on a CC list for the bug
itself, then we should plan on keeping a recird that ConfirmGroup() has already
been called for the other users.  This will help once processmail becomes a module.

Comment 10

[Dave Miller [:justdave]]

We have a security release to the 2.17 branch coming up really shortly...  now
would be a good time to fix this so it can be included.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

I think the minimal thing to do here is take the existing patch, add to it a
permissions check for cc: list addresses, and silently remove those that don't
have an account or whose account doesn't have permissions.  I'll do that Sunday
or Monday, and that'll take care of the security bug.  Then we can leave this
bug open or file a new one on making it work well.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress #2: includes cc: checks

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress #3: cc: checks are in a better place and work

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implementation

This checks and removes flag cc:s and throws an intelligible (although crappy)
error message if the user requests a flag from someone who can't see the bug.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 119528 [details] [diff] [review]
patch v1: implementation

Saturday wait
And sunday always comes too late
But friday never hesitate...

Comment 16

[Bradley Baetz (:bbaetz)]

Comment on attachment 119528 [details] [diff] [review]
patch v1: implementation

>Index: attachment.cgi

>+                  || &::ThrowUserError("flag_requestee_unauthorized",

For ThrowUserError, you can now |use Bugzilla::Error|, and drop the &::.

>+    &::SendSQL("SELECT    1, short_desc, product_id, component_id,
>+                          SUM(bug_group_map.group_id)

COUNT(), not SUM - its marginally cheaper. What we really want is an EXISTS
subselect, but....

>+    # If the target bug is restricted to one or more groups, then we need
>+    # to make sure we don't send email about it to unauthorized users
>+    # on the request type's CC: list, so we have to trawl the list for users
>+    # not on in those groups or email addresses that don't have an account.
>+    if ($flag->{'target'}->{'bug'}->{'restricted'}) {
>+        my @new_cc_list;
>+        foreach my $cc (split(/[, ]+/, $flag->{'type'}->{'cc_list'})) {
>+            my $user_id = &::DBname_to_id($cc) || next;
>+            &::ConfirmGroup($user_id);

This would all be a lot easier with Bugzilla::User. I should probably finish
that patch off...

>+            push(@new_cc_list, $cc)
>+              if &::CanSeeBug($flag->{'target'}->{'bug'}->{'id'}, $user_id);

Is there a way of testing multiple people for CanSeeBug in one go? That would
be a lot quicker. I know how to do it given subselects and MINUS, but I'm not
sure if theres an efficent way without it. I also don't know a nice way to do
it given the , separated stuff we have.

>+sub privileges {

Hmm. You know, I'd rather that you didn't do this, and waited for my user.pm
patch to go in. OTOH, you don't actually appear to use this sub anywhere, so
just drop it.

Do you need to handle private attachments here, or does somewhere else do that?

OK, now let me go and test.

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 119528 [details] [diff] [review]
patch v1: implementation


>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "flag_requestee_unauthorized" %]
>+    [% title = "Flag Requestee Not Authorized" %]
>+
>+    You asked [% requestee.identity FILTER html %] for 
>+    [% flag_type.name FILTER html %], but bug [% bug_id %] has been restricted 
>+    to users in certain groups, and that user isn't in all the groups 
>+    to which the bug has been restricted.  Please choose someone else to ask 
>+    or go back and add that user to the CC: list for the bug (making sure 
>+    users on the bug's CC: list are able to access it).

There should probably be <code></code> arround some stuff here, too

Anyway, this dies when I try to add teh patch flag to a bug, because
bug_group_map isn't locked in GetBug. I _hate_ this behaviour of mysql.

Is Bug.pm lightweight enough now? (Not that that would help the locking issue,
mind you)

Comment 18

[Bradley Baetz (:bbaetz)]

Comment on attachment 119528 [details] [diff] [review]
patch v1: implementation

Also fails if I don't put any name in the field when requesting a flag	which
is specifically requestable.

Comment 19

[Bradley Baetz (:bbaetz)]

Comment on attachment 119528 [details] [diff] [review]
patch v1: implementation

... in fact, all attachment actions fail.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes bbaetz' issues

>For ThrowUserError, you can now |use Bugzilla::Error|, and drop the &::.

Ok, done for new throws.


>COUNT(), not SUM - its marginally cheaper.

Done.


>Is there a way of testing multiple people for CanSeeBug in one go?

Not at first glance anyway, but the number of addresses on a flag type's CC:
list is likely to be small; it can always be an alias that explodes to multiple
users, after all.


>Hmm. You know, I'd rather that you didn't do this, and waited for my user.pm
>patch to go in. OTOH, you don't actually appear to use this sub anywhere, so
>just drop it.

Done.


>Do you need to handle private attachments here, or does somewhere else do
that?

No, I need to handle them.  Done.


>There should probably be <code></code> arround some stuff here, too

Done.


>Anyway, this dies when I try to add teh patch flag to a bug, because
>bug_group_map isn't locked in GetBug. I _hate_ this behaviour of mysql.

Yuck.  Fixed.


>Is Bug.pm lightweight enough now? (Not that that would help the locking issue,

>mind you)

Not sure, but let's keep this patch minimal, to reduce risk and get it done
quicker, as it's wanted for the security update.


>Also fails if I don't put any name in the field when requesting a flag which
>is specifically requestable.

>... in fact, all attachment actions fail.

All problems have been fixed as far as I can tell.


I also removed the pending fix; I'll attach that to the relevant bug instead.

Comment 21

[Bradley Baetz (:bbaetz)]

Comment on attachment 119629 [details] [diff] [review]
patch v2: fixes bbaetz' issues

You've reverted a whole set of my ThrowUserError changes in proces_bug.

trim isn't |undef| safe on semi-purpose, I believe - all operations are not
defined, baisclaly.

Whats the CanSeeBug comment all about? Isn't that to do with that other bug?

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: oops fixed

>You've reverted a whole set of my ThrowUserError changes in proces_bug.

Urk. How did I miss that?  Fixed.  Strange that CVS made it happen, however. 
Somehow when I CVS updated I didn't get your changes, but the version flag got
updated to the latest version, so it looked like I was backing your changes
out.

>trim isn't |undef| safe on semi-purpose, I believe - all operations are not
>defined, baisclaly.

The only thing my change does is prevent warnings from being printed to the web
server error log about "Use of uninitialized value in substitution".  People
rarely look at their web server's error log unless there's an actual error, at
which point lots of warning messages is just a distraction, so the current
behavior is not that great.

So, we have two alternatives to the current behavior: throw an error when trim
gets an uninitialized string, or deal with it somehow without throwing an error
or a warning.  I'm ok with either, but I think that trim is the kind of
function it would be useful to be able to call casually, f.e. in this patch
where I use "trim(<form-field>)" without checking to see if <form-field> exists
first.

>Whats the CanSeeBug comment all about? Isn't that to do with that other bug?

Yeah.  I thought it would be useful to state, but it isn't necessary; removed.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 119707 [details] [diff] [review]
patch v3: oops fixed

Review requested, but here's a potential problem:

attachment.cgi and process_bug.cgi lock a bunch of tables and then call
Flag::process, which calls ::create, ::modify, and ::clear, which call
::notify, which calls ConfirmGroup, which calls DeriveGroup, which locks a few
tables, killing the previous locks.  Another reason to fix bug 200174, although
there's probably a workaround here somewhere.

Comment 24

[Bradley Baetz (:bbaetz)]

The 'workarround' is to not lock tables, but to use transactions :) This is just
going to keep happening as we modularise and abstract stuff.

Can't we just UNLOCK specific tables, or something?

Comment 25

[Joel Peshkin]

I *think* you can issue another LOCK TABLES statement with a subset of the
tables that were originally locked.  This needs to be done with care (run-time
check?) because of the risk that a subsequent lock is not a true subset of the
original lock as this has potential to cause very intermittent deadlocks.

Comment 26

[Bradley Baetz (:bbaetz)]

Re comment 23 - does that mean that attachment.cgi holds table locks whilst it
sends mail??

Anywa, the fix would be to not unlock the tables if we've already locked stuff.
We're going to have to have a 'lock' abstraction UI anyway, for cross db
support, and that can easily hold a boolean flag...

Comment 27

[Myk Melez [:myk] [@mykmelez]]

>Re comment 23 - does that mean that attachment.cgi holds table locks whilst it
>sends mail??

In general, no, but when it comes to flags, yes.

Comment 28

[Dave Miller [:justdave]]

This isn't making the 2.17.4 train unless it gets resolved in the next 24 hours.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: fixes locking problem

Here's a fix for the locking problem.  It's a hack, but it'll work until we can
switch to transactions.  Basically I just pass a flag telling DeriveGroups that
we've already locked tables, and then DeriveGroups doesn't try to lock them
itself.  I also modified the actual lock statements to comply with the locks
DeriveGroups wants.

Comment 30

[Bradley Baetz (:bbaetz)]

Comment on attachment 121331 [details] [diff] [review]
patch v4: fixes locking problem

This looks fine (for a 'mysql sucks' defn of fine) from a _very_ quick glance
through. I'm not really able to spend time on bugzilla tonight, though, so if
we want this soon someone else should look at it. Late Thursday US time is
probably the earliest, although maybe tomorrow. Sorry.

Comment 31

[J. Paul Reed [:preed]]

Comment on attachment 121331 [details] [diff] [review]
patch v4: fixes locking problem

Ok: since we're in a hurry, I kinda got the 30,000 feet view of what this patch
is doing; it looks good, and that majority of the stuff below is (heavy)
"nits"... "serious suggestions," if you will.

Because we're trying to release, r=preed, but please consider fixing this stuff
below before it goes in.

> sub GetSelectableProducts {
>-    my $query = "SELECT name " .
>+    my ($by_id) = @_;
>+    my $query = "SELECT " . ($by_id ? "id, " : "") . "name " .

This is complicated, hard to read, and prone to errors in the quoting.

How about:

my $extraSql = ""
$extraSql = "id, " if ($by_id);

my $query = "SELECT $extraSql name " .

I don't care what the variable is named, but the inline expression evaluation
with all the quotation marks... ewww...

>+    foreach my $table (@tables) {
>+        # Why oh why can't we standardize on these names?!?

Can we get a bug filed on this if it's that important (and I agree that it is)?

Can you put a quick comment in about why we have to check if we're locked now?
>+    SendSQL("LOCK TABLES profiles WRITE, user_group_map WRITE, group_group_map READ, groups READ")
>+      unless $locked;

>+        # Make sure the requestee is authorized to access the bug.
>+        # (and attachment, if this installation is using the "insider group"
>+        # feature and the attachment is marked private).
>+        if ($status eq '?'
>+            && $flag->{type}->{is_requesteeble}
>+            && trim($data->{"requestee-$id"}))

Please pick a consistant quoting style. Above, you seem to use ->{'foo'} or
->{"bar"}, but here, you don't.

>+                # We know the requestee exists because we ran
>+                # Bugzilla::User::match_field before getting here.
>+                # ConfirmGroup makes sure their group settings
>+                # are up-to-date or calls DeriveGroups to update them.
>+                &::ConfirmGroup($requestee_id, 1);

Can you add a comment about what the 1 is about; that, or make a #define (or
the perl equivalent) called $LOCK_TABLE or $TABLE_ALREADY_LOCKED or whatever
that equals 1, so it's clear that that's all about.

>+                                     attach_id =>
>+                                       $flag->{target}->{attachment}->{id} });

Quoting again.

>+                if ($flag->{target}->{attachment}->{exists}

Quoting.

Comment 32

[Bradley Baetz (:bbaetz)]

The names are different because components is the only one to have been updated
so far to use ids.

(The others are likely to become custfields at some point, too)

Also, remove that pending/pend change which sunck in from an unrelated bug. /me
goes to test.

Comment 33

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: review fixes

>This is complicated, hard to read, and prone to errors in the quoting.
>How about: ...

Eww, but ok, I did:    my $extra_sql = $by_id ? "id, " : "";

>Can we get a bug filed on this if it's that important (and I agree that it
is)?

Comment added to bug 200837.

>Can you put a quick comment in about why we have to check if we're locked now?


Done.

>Please pick a consistant quoting style.

Ok, I pick quoteless.

>Above, you seem to use ->{'foo'} or ->{"bar"}, but here, you don't.

Well, I generally only use double quotes when variables need interpolating. 
Otherwise I choose quoteless, although I previously used single quotes, so
those are still prevalent in the code.

>Can you add a comment about what the 1 is about; that, or make a #define (or
the perl equivalent)

Ok, made a constant called TABLES_ALREADY_LOCKED.  Also, removed the argument
where it wasn't needed (i.e. in calls from validate, which is called outside of
any table locks).

>Also, remove that pending/pend change which sunck in from an unrelated bug.

Done.

Comment 34

[Bradley Baetz (:bbaetz)]

Comment on attachment 121606 [details] [diff] [review]
patch v5: review fixes

r=bbaetz, but please get a second review.

Also, please file a bug on that issue with invalid login names not generating
an error from match_user.

Comment 35

[J. Paul Reed [:preed]]

Comment on attachment 121606 [details] [diff] [review]
patch v5: review fixes

My biggest nits were clobbered... carry that r=preed forward!

Comment 36

[Myk Melez [:myk] [@mykmelez]]

Checking in Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.14; previous revision: 1.13
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.40; previous revision: 1.39
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.237; previous revision: 1.236
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.183; previous revision: 1.182
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.34; previous revision: 1.33
done

Comment 37

[Dave Miller [:justdave]]

Security Advisory has been posted, removing security group

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Resecuring so I can test application of this patch on b.m.o.

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 121606 [details] [diff] [review]
patch v5: review fixes

test of this patch on b.m.o: this is the unpatched version

Comment 40

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 121606 [details] [diff] [review]
patch v5: review fixes

test of this patch on b.m.o: this is the patched version