Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Attachments & Requests
--
major
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: justdave, Assigned: reed)

Tracking

3.2.1
Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +
blocking3.4 +
approval3.2 +
blocking3.2.3 +
blocking3.2.2 -
testcase ?

Details

(Whiteboard: [sg:want][wanted-bmo]security)

Attachments

(2 attachments, 4 obsolete attachments)

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.
(Assignee)

Updated

9 years ago
Flags: blocking3.2.2?
Whiteboard: [sg:want][wanted-bmo]
(Assignee)

Comment 1

9 years ago
(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.
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

9 years ago
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.
Flags: blocking3.2.2? → blocking3.2.2-
(Assignee)

Comment 4

9 years ago
Created attachment 360262 [details] [diff] [review]
patch - v1

Add CSRF protection to edit action and rename the token used by delete action to "deleteattachment".
Assignee: attach-and-request → reed
Status: NEW → ASSIGNED
Attachment #360262 - Flags: review?(LpSolit)
(Assignee)

Updated

9 years ago
Flags: blocking3.2.3?
Whiteboard: [sg:want][wanted-bmo] → [sg:want][wanted-bmo]security
Target Milestone: --- → Bugzilla 3.0
(Assignee)

Comment 5

9 years ago
Created attachment 360265 [details] [diff] [review]
patch - v1.1

Don't need the delete_token() call since it's a hash token.
Attachment #360262 - Attachment is obsolete: true
Attachment #360265 - Flags: review?(LpSolit)
Attachment #360262 - Flags: review?(LpSolit)

Comment 6

9 years ago
Yes, definitely a blocker. We already have a patch and this would be good to fix.
Flags: blocking3.2.3? → blocking3.2.3+

Comment 7

9 years ago
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.
Severity: blocker → major

Updated

9 years ago
Attachment #360265 - Flags: review?(LpSolit) → review-

Comment 8

9 years ago
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.
(Assignee)

Comment 9

9 years ago
(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

9 years ago
(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).
(Assignee)

Comment 11

9 years ago
Created attachment 361094 [details] [diff] [review]
patch - v2

Address review comments. I'll move the rename to a new bug.
Attachment #360265 - Attachment is obsolete: true
Attachment #361094 - Flags: review?(LpSolit)

Comment 12

9 years ago
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.
Attachment #361094 - Flags: review?(LpSolit) → review+

Comment 13

9 years ago
I'm not yet requesting approval due to comment 12. Dave, Max, what's your opinion on this?
Depends on: 365267
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

9 years ago
(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

9 years ago
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?

Updated

9 years ago
Flags: blocking3.4+

Comment 17

9 years ago
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.
Flags: approval?
Flags: approval3.2?

Updated

9 years ago
Flags: blocking3.0.9+

Comment 18

9 years ago
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).
No longer depends on: 365267
Flags: blocking3.0.9+
Summary: editing attachments doesn't have any CSRF protection → [SECURITY] Editing attachments doesn't have any CSRF protection
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Updated

9 years ago
Blocks: 480875

Comment 19

8 years ago
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

8 years ago
reed: You know, just to be safe, we should probably add a <token> element to every <attachment> element in a bug's XML.
(Assignee)

Comment 21

8 years ago
Created attachment 369948 [details] [diff] [review]
patch for HEAD - v3

Address comment #12 nit and comment #20 suggestion.
Attachment #361094 - Attachment is obsolete: true
Attachment #369948 - Flags: review?(LpSolit)

Comment 22

8 years ago
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

8 years ago
Thanks for cc'ing us Max.  Yes, we will need the token in the xml. When is this slated for release?

Comment 24

8 years ago
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.
Attachment #369948 - Flags: review?(LpSolit) → review+

Updated

8 years ago
Attachment #369948 - Attachment description: patch - v3 → patch for HEAD - v3
(Assignee)

Comment 25

8 years ago
Created attachment 370036 [details] [diff] [review]
patch for HEAD - v3.1
Attachment #369948 - Attachment is obsolete: true
Attachment #370036 - Flags: review+
(Assignee)

Comment 26

8 years ago
Created attachment 370038 [details] [diff] [review]
patch for 3.2 - v1
Attachment #370038 - Flags: review?(LpSolit)

Comment 27

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

r=LpSolit
Attachment #370038 - Flags: review?(LpSolit) → review+

Comment 28

8 years ago
(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

8 years ago
ok for checkin.
Flags: approval?
Flags: approval3.2?
Flags: approval3.2+
Flags: approval+
(Assignee)

Comment 30

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 31

8 years ago
Security advisory sent, unlocking this bug.
Group: bugzilla-security

Updated

8 years ago
Blocks: 488173

Updated

6 years ago
Flags: testcase?

Updated

4 years ago
Blocks: 913904
You need to log in before you can comment on or make changes to this bug.