Last Comment Bug 293159 - [SECURITY] Anyone can change flags and access bug summaries due to a bad check in Flag::validate() and Flag::modify()
: [SECURITY] Anyone can change flags and access bug summaries due to a bad chec...
Status: RESOLVED FIXED
[does not affect 2.16.x] [ready for 2...
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 2.17
: All All
: -- critical (vote)
: Bugzilla 2.18
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks: 266159 294021 297749
  Show dependency treegraph
 
Reported: 2005-05-06 09:52 PDT by Frédéric Buclin
Modified: 2005-07-08 00:10 PDT (History)
1 user (show)
justdave: approval+
justdave: blocking2.20+
justdave: approval2.18+
justdave: blocking2.18.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (13.51 KB, patch)
2005-05-26 16:01 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2 (14.26 KB, patch)
2005-05-26 18:21 PDT, Frédéric Buclin
myk: review+
Details | Diff | Splinter Review
patch, v2.1 (14.26 KB, patch)
2005-06-11 09:23 PDT, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review
patch for 2.18.1, v1 (13.01 KB, patch)
2005-06-11 10:42 PDT, Frédéric Buclin
myk: review+
Details | Diff | Splinter Review
patch for trunk, v2.1.1 (14.21 KB, patch)
2005-07-07 20:22 PDT, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review
patch for 2.18.1, v1.0.1 (13.08 KB, patch)
2005-07-07 20:23 PDT, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review

Description Frédéric Buclin 2005-05-06 09:52:42 PDT
Flag::validate() and Flag::modify() do not check that the flag ID one tries to
change is consistent with the bug ID and/or attachment ID passed to the
function. This way, we are able to access the summary of (almost) any bug which
has flags set on it.

It's very difficult to reproduce if you have no idea of flag IDs set on a given
bug. The idea is the following:

1) hack the URL in order to have something like:

process_bug.cgi?id=196&product=Bugzilla&component=...&flag-481=%3F&requestee-481=guest%40foo.com

Here, the flag with ID = 481 is not related to the public bug 196 but is related
to another (confidential) bug. But as the URL contains the bug ID of a public
bug, neither process_bug.cgi nor Flag::validate() or Flag::modify() will
complain as they all check if the user has access to the given bug or attachment
based on the bug/attachement ID passed in the URL.


2) use the same URL as above, but change flag-481=%3F to flag-481=X.

This way, you cancel the request and the requester (in our case: a bad guy) will
receive an email that this request has been cancelled. But the email contains
the summary of the bug!! <-- Oops!!

Note that you have to use an existing flag which is either set to + or -, else
this trick won't work (if the flag is not specifically requestable!).

Note also that this does not work if you try to create a new flag using
flag_type in the URL instead of flag, because the new flag would be created in
the public bug (the one given in the URL). About this last comment, see also bug
266159.
Comment 1 Frédéric Buclin 2005-05-26 16:01:09 PDT
Created attachment 184629 [details] [diff] [review]
patch, v1
Comment 2 Frédéric Buclin 2005-05-26 16:27:58 PDT
Comment on attachment 184629 [details] [diff] [review]
patch, v1

>+  [% ELSIF error == "invalid_flag_association" %]
>+    [% title = "Invalid Flag Association" %]
>+    Some flags do not belong to
>+    [% IF attach_id %]
>+      attachment [% attach_id FILTER html %].
>+    [% ELSE %]
>+      [% terms.bug %] [%+ bug_id FILTER html %].
>+    [% END %]

Note to self: it should be [%+ terms.bug %].
Comment 3 Frédéric Buclin 2005-05-26 18:21:45 PDT
Created attachment 184644 [details] [diff] [review]
patch, v2

Now catch inactive flag types + fix some minor nits (mainly typos).
Comment 4 Myk Melez [:myk] [@mykmelez] 2005-06-10 15:28:35 PDT
Comment on attachment 184644 [details] [diff] [review]
patch, v2

Looks good. r=myk
Comment 5 Frédéric Buclin 2005-06-10 15:45:17 PDT
Already requesting approval for the trunk. Give me a few hours to backport it to
2.18.1.
Comment 6 Frédéric Buclin 2005-06-11 09:23:29 PDT
Created attachment 185962 [details] [diff] [review]
patch, v2.1

FlagType.pm uses requestee_type, not requestee (stupid copy paste). Carrying
forward myk's r+ (use interdiff between v2 and v2.1 if you don't trust me).
Comment 7 Frédéric Buclin 2005-06-11 10:42:43 PDT
Created attachment 185974 [details] [diff] [review]
patch for 2.18.1, v1

backport for the 2.18 branch. The code is a bit easier as 2.18 does not let you
add flags when creating a new attachment.
Comment 8 Myk Melez [:myk] [@mykmelez] 2005-06-15 16:47:32 PDT
Comment on attachment 185974 [details] [diff] [review]
patch for 2.18.1, v1

Looks good, r=myk
Comment 9 Myk Melez [:myk] [@mykmelez] 2005-06-15 16:53:33 PDT
I missed this in my initial review, and it's nit, but if it's not too much
trouble could you change "Edition of Flags" to "Flag Editing" upon check-in and
then attach the patches with that update?  The current syntax is technically
valid but uncommon and considered clunky, and making this change in new patches
on this bug would save having to deal with that minor issue in a separate bug.
Comment 10 Frédéric Buclin 2005-06-15 16:59:04 PDT
ok, I will fix this nit on checkin. Patches ready for approval! :)
Comment 11 Frédéric Buclin 2005-07-04 14:21:26 PDT
For further information about this bug, see my comment in bug 297749 comment 4.
Comment 12 Frédéric Buclin 2005-07-07 20:22:40 PDT
Created attachment 188625 [details] [diff] [review]
patch for trunk, v2.1.1

myk's nit fixed + unbitrot
Comment 13 Frédéric Buclin 2005-07-07 20:23:51 PDT
Created attachment 188626 [details] [diff] [review]
patch for 2.18.1, v1.0.1

myk's nit fixed.
Comment 14 Max Kanat-Alexander 2005-07-07 22:31:18 PDT
Tip:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.89; previous revision: 1.88
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.263; previous revision: 1.262
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.45; previous revision: 1.44
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.52; previous revision: 1.51
done

2.18:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.58.2.6; previous revision: 1.58.2.5
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.22; previous revision: 1.205.2.21
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.18.2.7; previous revision: 1.18.2.6
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.7.2.2; previous revision: 1.7.2.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.39.2.6; previous revision: 1.39.2.5
done
Comment 15 Max Kanat-Alexander 2005-07-08 00:10:40 PDT
Removing from webtools-security because the advisory has been posted and the
release is complete.

Note You need to log in before you can comment on or make changes to this bug.