293159 - [SECURITY] Anyone can change flags and access bug summaries due to a bad check in Flag::validate() and Flag::modify()

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Frédéric Buclin]

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]

Attachment: patch, v1

Comment 2

[Frédéric Buclin]

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]

Attachment: patch, v2

Now catch inactive flag types + fix some minor nits (mainly typos).

Comment 4

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

Comment on attachment 184644 [details] [diff] [review]
patch, v2

Looks good. r=myk

Comment 5

[Frédéric Buclin]

Already requesting approval for the trunk. Give me a few hours to backport it to
2.18.1.

Comment 6

[Frédéric Buclin]

Attachment: 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]

Attachment: 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]]

Comment on attachment 185974 [details] [diff] [review]
patch for 2.18.1, v1

Looks good, r=myk

Comment 9

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

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]

ok, I will fix this nit on checkin. Patches ready for approval! :)

Comment 11

[Frédéric Buclin]

For further information about this bug, see my comment in bug 297749 comment 4.

Comment 12

[Frédéric Buclin]

Attachment: patch for trunk, v2.1.1

myk's nit fixed + unbitrot

Comment 13

[Frédéric Buclin]

Attachment: patch for 2.18.1, v1.0.1

myk's nit fixed.

Comment 14

[Max Kanat-Alexander]

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]

Removing from webtools-security because the advisory has been posted and the
release is complete.