476603 - [SECURITY] Editing attachments doesn't have any CSRF protection

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Dave Miller [:justdave]]

I loaded an attachment.cgi action=view page while logged in with my [e-mail] account.  I then logged out and back in with my [e-mail] 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]]

(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]]

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]

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

Attachment: patch - v1

Add CSRF protection to edit action and rename the token used by delete action to "deleteattachment".

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v1.1

Don't need the delete_token() call since it's a hash token.

Comment 6

[Max Kanat-Alexander]

Yes, definitely a blocker. We already have a patch and this would be good to fix.

Comment 7

[Frédéric Buclin]

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]

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

(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]

(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]]

Attachment: patch - v2

Address review comments. I'll move the rename to a new bug.

Comment 12

[Frédéric Buclin]

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]

I'm not yet requesting approval due to comment 12. Dave, Max, what's your opinion on this?

Comment 14

[Dave Miller [:justdave]]

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]

(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]

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]

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]

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]

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]

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

Attachment: patch for HEAD - v3

Address comment #12 nit and comment #20 suggestion.

Comment 22

[Max Kanat-Alexander]

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]

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]

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

Attachment: patch for HEAD - v3.1

Comment 26

[Reed Loden [:reed]]

Attachment: patch for 3.2 - v1

Comment 27

[Frédéric Buclin]

Comment on attachment 370038 [details] [diff] [review]
patch for 3.2 - v1

r=LpSolit

Comment 28

[Max Kanat-Alexander]

(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]

ok for checkin.

Comment 30

[Reed Loden [:reed]]

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]

Security advisory sent, unlocking this bug.