Last Comment Bug 476603 - [SECURITY] Editing attachments doesn't have any CSRF protection
: [SECURITY] Editing attachments doesn't have any CSRF protection
Status: RESOLVED FIXED
[sg:want][wanted-bmo]security
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 3.2.1
: All All
: -- major (vote)
: Bugzilla 3.2
Assigned To: Reed Loden [:reed] (use needinfo?)
: default-qa
Mentors:
Depends on:
Blocks: 480875 488173 CVE-2013-1734
  Show dependency treegraph
 
Reported: 2009-02-02 23:10 PST by Dave Miller [:justdave] (justdave@bugzilla.org)
Modified: 2013-09-08 09:19 PDT (History)
8 users (show)
LpSolit: approval+
LpSolit: blocking3.4+
LpSolit: approval3.2+
mkanat: blocking3.2.3+
mkanat: blocking3.2.2-
LpSolit: testcase?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (5.51 KB, patch)
2009-02-03 00:38 PST, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Review
patch - v1.1 (4.83 KB, patch)
2009-02-03 00:43 PST, Reed Loden [:reed] (use needinfo?)
LpSolit: review-
Details | Diff | Review
patch - v2 (3.41 KB, patch)
2009-02-07 15:03 PST, Reed Loden [:reed] (use needinfo?)
LpSolit: review+
Details | Diff | Review
patch for HEAD - v3 (5.08 KB, patch)
2009-03-29 18:52 PDT, Reed Loden [:reed] (use needinfo?)
LpSolit: review+
Details | Diff | Review
patch for HEAD - v3.1 (5.22 KB, patch)
2009-03-30 11:18 PDT, Reed Loden [:reed] (use needinfo?)
reed: review+
Details | Diff | Review
patch for 3.2 - v1 (5.22 KB, patch)
2009-03-30 11:21 PDT, Reed Loden [:reed] (use needinfo?)
LpSolit: review+
Details | Diff | Review

Description Dave Miller [:justdave] (justdave@bugzilla.org) 2009-02-02 23:10:36 PST
I loaded an attachment.cgi action=view page while logged in with my justdave@mozilla.com account.  I then logged out and back in with my justdave@bugzilla.org account, then submitted my changes on the form without reloading it, and it let me.  The CSRF check should have failed on that because I was using a different User ID.

LpSolit says that's the only form left that wasn't done yet, and he didn't have a bug on file for it, so here's the bug.
Comment 1 Reed Loden [:reed] (use needinfo?) 2009-02-02 23:14:20 PST
(In reply to comment #0)
> LpSolit says that's the only form left that wasn't done yet, and he didn't have
> a bug on file for it, so here's the bug.

Technically, there's also bug 471801, too.
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-02-02 23:17:32 PST
We're already doing a version 3.2.2 sometime in the next several hours because of bug 476594, so if we can get a patch on this and reviewed and tested in that timeframe we might be able to include this in 3.2.2.  Since our previous advisory indicated that we now had application-wide CSRF protection it'd probably be a good idea to include this if we can.  LpSolit's about to head out to work though, so it won't be him working on it.
Comment 3 Max Kanat-Alexander 2009-02-02 23:58:53 PST
This is an emergency release, I need to get it out ASAP, so there will be no fixes other than the emergency fix. We can fix this for 3.2.3.
Comment 4 Reed Loden [:reed] (use needinfo?) 2009-02-03 00:38:14 PST
Created attachment 360262 [details] [diff] [review]
patch - v1

Add CSRF protection to edit action and rename the token used by delete action to "deleteattachment".
Comment 5 Reed Loden [:reed] (use needinfo?) 2009-02-03 00:43:47 PST
Created attachment 360265 [details] [diff] [review]
patch - v1.1

Don't need the delete_token() call since it's a hash token.
Comment 6 Max Kanat-Alexander 2009-02-03 00:50:55 PST
Yes, definitely a blocker. We already have a patch and this would be good to fix.
Comment 7 Frédéric Buclin 2009-02-03 04:28:08 PST
None of our recent security bugs was marked as more severe than "major", and this one was left intentionally behind -> major as well. I will review it later today when I'm back home.
Comment 8 Frédéric Buclin 2009-02-07 09:20:36 PST
Comment on attachment 360265 [details] [diff] [review]
patch - v1.1

>Index: attachment.cgi

>-                  && ($event eq 'attachment' . $attachment->id))
>+                  && ($event eq 'deleteattachment' . $attachment->id))

This change is unrelated to this bug. It's a session token, unrelated to hash tokens. If we change this, all old tokens with "attachment" in them will be seen as invalid. I would only accept this change in 3.4, not on branches (as there is no reason to do this change). Also, it should be named "delete_attachment".



>Index: template/en/default/attachment/edit.html.tmpl

>+  <input type="hidden" name="token" value="[% issue_hash_token([attachment.id, attachment.modification_time]) FILTER html %]">

This must be enclosed in [% IF user.id %] [% END %], as tokens generated for logged out users are invalid. Also, if the user is logged out before reaching this page, clicking "Submit" will first display the login page, then display the "suspicious action" page. We should try to find a way to avoid this problem.
Comment 9 Reed Loden [:reed] (use needinfo?) 2009-02-07 14:41:04 PST
(In reply to comment #8)
> (From update of attachment 360265 [details] [diff] [review])
> >Index: attachment.cgi
> 
> >-                  && ($event eq 'attachment' . $attachment->id))
> >+                  && ($event eq 'deleteattachment' . $attachment->id))
> 
> This change is unrelated to this bug. It's a session token, unrelated to hash
> tokens. If we change this, all old tokens with "attachment" in them will be
> seen as invalid. I would only accept this change in 3.4, not on branches (as
> there is no reason to do this change). Also, it should be named
> "delete_attachment".

I'm ok with making that change only on tip, but I disagree with your last comment, especially since I used the "createattachment" session token as the example when renaming it:
  $vars->{'token'} = issue_session_token('createattachment:');

So, do you want delete_attachment or deleteattachment, and if delete_attachment, should createattachment be renamed to create_attachment?

> >Index: template/en/default/attachment/edit.html.tmpl
> 
> >+  <input type="hidden" name="token" value="[% issue_hash_token([attachment.id, attachment.modification_time]) FILTER html %]">
> 
> This must be enclosed in [% IF user.id %] [% END %], as tokens generated for
> logged out users are invalid. Also, if the user is logged out before reaching
> this page, clicking "Submit" will first display the login page, then display
> the "suspicious action" page. We should try to find a way to avoid this
> problem.

Why do we show the full edit UI on the Edit page for attachments when the user is logged out? We don't do that for show_bug anymore, so not sure why we're doing it for edit attachment... I was very surprised to see that logged-out users can go to https://landfill.bugzilla.org/bugzilla-tip/attachment.cgi?id=847&action=edit and see the full edit UI. That seems like a problem in itself.
Comment 10 Frédéric Buclin 2009-02-07 14:50:48 PST
(In reply to comment #9)
> So, do you want delete_attachment or deleteattachment, and if
> delete_attachment, should createattachment be renamed to create_attachment?

Yeah, both with an underscore, please. But not as part of this security bug.


> Why do we show the full edit UI on the Edit page for attachments when the user
> is logged out?

Because it always behaved this way since this page exists. We only fixed show_bug so far, and attachment.cgi still needs to be fixed to only look editable by logged in users. This should be fixed separately (and I will file a bug if there isn't one yet).
Comment 11 Reed Loden [:reed] (use needinfo?) 2009-02-07 15:03:47 PST
Created attachment 361094 [details] [diff] [review]
patch - v2

Address review comments. I'll move the rename to a new bug.
Comment 12 Frédéric Buclin 2009-02-08 03:13:48 PST
Comment on attachment 361094 [details] [diff] [review]
patch - v2

>Index: template/en/default/attachment/edit.html.tmpl

>+[% IF user.id %]
>+  <input type="hidden" name="token" ...
>+[% END %]

Please fix the indentation. [% IF ... %] but be aligned with the surrounding HTML elements. Otherwise looks good and works fine (I tested on tip only! It still needs testing on branches, despite I don't suspect any reason to break there). r=LpSolit


Note that due to the problem I mentioned in the second part of my comment 8 about logged out users, bug 365267 should be fixed first, which unfortunately won't happen on branches. I don't want the "invalid token" message to be displayed to all logged out users. It's scary, inaccurate, and would break their workflow. We have to discuss and decide what's the best approach for them.
Comment 13 Frédéric Buclin 2009-02-08 03:15:34 PST
I'm not yet requesting approval due to comment 12. Dave, Max, what's your opinion on this?
Comment 14 Dave Miller [:justdave] (justdave@bugzilla.org) 2009-02-08 09:51:27 PST
couldn't we check if they authenticated as part of the request?  Is there a way to do that?  if so you could check either a token or a login if they bounced off the login page during the submit.

We also advertised the conversion to needing to be logged in to edit as a feature on 3.2, so backporting this to 3.2 could still be considered a bugfix, since we intended to have it and forgot, essentially.

Traditionally in the past, the current stable release still gets normal bugfixes until a new stable release replaces it, and then it drops to security-only.  So if we keep with tradition bug 365267 could still be backported to 3.2.
Comment 15 Frédéric Buclin 2009-02-08 09:55:52 PST
(In reply to comment #14)
> We also advertised the conversion to needing to be logged in to edit as a
> feature on 3.2, so backporting this to 3.2 could still be considered a bugfix

We said and do so for show_bug.cgi. But we definitely didn't do it for attachment.cgi. So it doesn't really look like a bugfix to me.
Comment 16 Max Kanat-Alexander 2009-02-08 11:29:58 PST
Hmmm. Well, I'd rather have people be secure than have things be convenient for the slightly-less-common case where somebody is logging in at the same time they're editing the page. We're eventually going to make that impossible anyway, so I think it's OK to make it slightly inconvenient now, no?
Comment 17 Frédéric Buclin 2009-02-26 14:11:09 PST
Tested successfully with 3.4 and 3.2, but the patch doesn't apply cleanly on the 3.0 branch. It needs to be backported.
Comment 18 Frédéric Buclin 2009-03-01 12:04:41 PST
OK, reed just noticed that attachments.modification_time doesn't exist on the 3.0 branch, and editing an attachment doesn't update bugs.delta_ts, so we cannot use it for hash tokens either. And I'm not going to use session tokens here because everyone viewing an attachment would trigger a new token, even if they are not editing it. So we are not going to take this security fix on this branch (it would require to change the DB schema to add the modification_time column to the attachments table, which, I suppose, we don't want).
Comment 19 Max Kanat-Alexander 2009-03-29 18:10:06 PDT
I've CC'ed a few client writers on this security bug here, before we release. Is this fix going to affect your clients? It adds the same sort of token protection for updating attachments that we added for updating bugs, and it doesn't add a token in the XML anywhere for the attachments.
Comment 20 Max Kanat-Alexander 2009-03-29 18:10:43 PDT
reed: You know, just to be safe, we should probably add a <token> element to every <attachment> element in a bug's XML.
Comment 21 Reed Loden [:reed] (use needinfo?) 2009-03-29 18:52:50 PDT
Created attachment 369948 [details] [diff] [review]
patch for HEAD - v3

Address comment #12 nit and comment #20 suggestion.
Comment 22 Max Kanat-Alexander 2009-03-29 19:37:43 PDT
Comment on attachment 369948 [details] [diff] [review]
patch for HEAD - v3

I'll let LpSolit r+ or r-, but this looks good to me. For the comment, though, it's not process_bug.cgi, it's attachment.cgi.
Comment 23 Robert Elves 2009-03-30 09:29:43 PDT
Thanks for cc'ing us Max.  Yes, we will need the token in the xml. When is this slated for release?
Comment 24 Frédéric Buclin 2009-03-30 11:00:03 PDT
Comment on attachment 369948 [details] [diff] [review]
patch for HEAD - v3

>Index: template/en/default/bug/show.xml.tmpl

>             <attacher>[% a.attacher.email FILTER email FILTER xml %]</attacher>

|FILTER email| doesn't exist in 3.2 and makes this patch to not apply cleanly on the 3.2 branch. You need to backport this patch due to this change.


>+        [%# This is here so automated clients can still use process_bug.cgi %]

mkanat is right. It's about attachment.cgi, not process_bug.cgi.


>+        [% IF displayfields.token && user.id %]
>+            <token>[% issue_hash_token([a.id, a.modification_time]) FILTER xml %]</token>
>+        [% END %]
>         [% IF displayfields.attachmentdata %]
>             <data encoding="base64">[% a.data FILTER base64 %]</data>
>-        [% END %]        
>+        [% END %]

These lines are not indented correctly.


r=LpSolit, but it would be nice to fix these comments in a new patch.
Comment 25 Reed Loden [:reed] (use needinfo?) 2009-03-30 11:18:34 PDT
Created attachment 370036 [details] [diff] [review]
patch for HEAD - v3.1
Comment 26 Reed Loden [:reed] (use needinfo?) 2009-03-30 11:21:23 PDT
Created attachment 370038 [details] [diff] [review]
patch for 3.2 - v1
Comment 27 Frédéric Buclin 2009-03-30 12:02:54 PDT
Comment on attachment 370038 [details] [diff] [review]
patch for 3.2 - v1

r=LpSolit
Comment 28 Max Kanat-Alexander 2009-03-30 12:23:13 PDT
(In reply to comment #23)
> Thanks for cc'ing us Max.  Yes, we will need the token in the xml. When is this
> slated for release?

  Probably some time this week.
Comment 29 Frédéric Buclin 2009-03-30 13:57:40 PDT
ok for checkin.
Comment 30 Reed Loden [:reed] (use needinfo?) 2009-03-30 14:04:04 PDT
CVS tip:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.153; previous revision: 1.152
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.58; previous revision: 1.57
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl
new revision: 1.29; previous revision: 1.28
done

BUGZILLA-3_2-BRANCH:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.144.2.5; previous revision: 1.144.2.4
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.51.2.3; previous revision: 1.51.2.2
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--  show.xml.tmpl
new revision: 1.23.2.4; previous revision: 1.23.2.3
done
Comment 31 Max Kanat-Alexander 2009-03-30 22:19:19 PDT
Security advisory sent, unlocking this bug.

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