503675 - Every user can remove any tags from any add-ons

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[fcp]

Everyone can remove any tags from any add-ons by faking a request as if it were generated by XMLHttpRequest.  My assumption is that this is not intended.

I flag this bug as security sensitive just in case.  Please remove the flag when it is fixed or it is no longer sensitive.

For example, I managed to remove a developer tag “one” and a user tag “90” from clouserw’s extension ThreadBuggle.  Neither of the tags had been added by me.  To do so, I only had to make a request which was crafted from a legitimate request with the referer header field and the addonid and the tagid in the request body modified.

Comment 1

[fcp]

Silly me.  Of course I meant ThreadBubble.... :(
https://preview.addons.mozilla.org/en-US/thunderbird/addon/5326

Comment 2

[fcp]

Here are steps to exploit this bug.  In the steps below, I use the Live HTTP Headers (LHH) extension:
https://addons.mozilla.org/addon/3829
because it is very useful when you capture a legitimate request and make a modified request.  Of course, LHH has nothing to do with this bug.

Steps to reproduce:
Suppose you want to remove a tag with tagid=TTTTT from an add-on with addonid=AAAAA.  The tagid can be found in the XHTML source of the add-on detail page.
1. Install LHH.
2. Log in to preview.addons.mozilla.org and open the details page of the add-on AAAAA.
3. Add a dummy tag.
4. Open Tools > Live HTTP Headers.
5. Remove the tag which you added in step 3.
6. In the LHH window, select the line
https://preview.addons.mozilla.org/en-US/thunderbird/tags/remove
and press the Replay... button.
7. Modify the value of “tagid=” in the request body to TTTTT.
8. Press the Replay button.
9. Go back to the add-on details page (and optionally reload), and look at the Tags list.

Actual results:
The tag which you tried to remove is really gone.

Expected results:
The request in step 8 is rejected by the server.

Comment 3

[Wil Clouser [:clouserw]]

Attachment: *sigh*

I was trying to change as little as possible since we're almost to a launch.  Since we already had the TagList I was just checking to see if the ownerOrDeveloper flag was set and if it was I was going to let them delete the tag.

However, I think I found a bug in the makeTagList() function.  To check if they are an owner or dev they are looking at a single element (hardcoded to [0]) when it can actually return many elements in the array.  I changed the association to only bind for the add-on id we want which I think should only return 1 element and fix their code.  We should definitely be checking this though as it's pretty core.

Also, since bug 500457 is stuck in a rut I added a few unit tests for these specific cases.

Comment 4

[Wil Clouser [:clouserw]]

Yeah, no need for this to be a security bug since it's not live yet.  All that would do is hide our shame and that's not how we roll.

Comment 5

[Jeff Balogh (:jbalogh)]

Comment on attachment 388224 [details] [diff] [review]
*sigh*

Everything works, but when I delete a tag it's not removed from the user-visible tag list.

Comment 6

[Wil Clouser [:clouserw]]

Attachment: disables cake's cache

Comment 7

[Jeff Balogh (:jbalogh)]

Comment on attachment 388285 [details] [diff] [review]
disables cake's cache

Works for me.  There's a lot of whitespace issues (tabs vs. spaces, trailing), but I think I'm the only one anal-retentive enough to be bothered.


>diff --git a/site/app/models/tag.php b/site/app/models/tag.php
>index c63da77..3c6d90f 100644
>--- a/site/app/models/tag.php
>+++ b/site/app/models/tag.php
>@@ -109,30 +111,63 @@ class Tag extends AppModel
>         } 
> 
>         if (!empty($_related_tag_ids)) {
>+            $this->bindModel(array('hasMany' => array('UserTagAddon' =>
>+                array(
>+                    'className' => 'UserTagAddon', 
>+                    'conditions' => array('addon_id' => $addon_data['Addon']['id'])
>+                )))
>+            );
>             $related_tags = $this->findAll("Tag.id IN (".implode(',', $_related_tag_ids).") and blacklisted=0",null,"Tag.tag_text asc");      	
>+            $this->__resetAssociations();

In theory, resetAssociations should not need to be called from our code.  Was there a particular reason for adding it here?

Comment 8

[Wil Clouser [:clouserw]]

(In reply to comment #7)
> (From update of attachment 388285 [details] [diff] [review])
> Works for me.  There's a lot of whitespace issues (tabs vs. spaces, trailing),
> but I think I'm the only one anal-retentive enough to be bothered.

Anything I add should be well indented with spaces.  I don't like to change tabs->spaces in other code because it bloats my patches.  I've committed several patches separately that just change whitespace though (and you are welcome to also as long as you make sure it doesn't break things. ;)  If I see trailing whitespace I'll get rid of it in my code but it doesn't worry me as much.

> >diff --git a/site/app/models/tag.php b/site/app/models/tag.php
> >index c63da77..3c6d90f 100644
> >--- a/site/app/models/tag.php
> >+++ b/site/app/models/tag.php
> >@@ -109,30 +111,63 @@ class Tag extends AppModel
> >         } 
> > 
> >         if (!empty($_related_tag_ids)) {
> >+            $this->bindModel(array('hasMany' => array('UserTagAddon' =>
> >+                array(
> >+                    'className' => 'UserTagAddon', 
> >+                    'conditions' => array('addon_id' => $addon_data['Addon']['id'])
> >+                )))
> >+            );
> >             $related_tags = $this->findAll("Tag.id IN (".implode(',', $_related_tag_ids).") and blacklisted=0",null,"Tag.tag_text asc");      	
> >+            $this->__resetAssociations();
> 
> In theory, resetAssociations should not need to be called from our code.  Was
> there a particular reason for adding it here?

No, I was just changing the association and wanted to make sure it went back to what it was.  Are associations reset after every find() call?

Comment 9

[Wil Clouser [:clouserw]]

Also, r29684.  thanks.

Comment 10

[Jeff Balogh (:jbalogh)]

(In reply to comment #8)
> (In reply to comment #7)    	
> > >+            $this->__resetAssociations();
> > 
> > In theory, resetAssociations should not need to be called from our code.  Was
> > there a particular reason for adding it here?
> 
> No, I was just changing the association and wanted to make sure it went back to
> what it was.  Are associations reset after every find() call?

Yes, it's at the bottom of findAll in cake/libs/model/model_php5.php:1186.

Comment 11

[fcp]

I verified on preview.a.m.o that everything works as intended: I can no longer remove a tag which had been added by another user to another user’s add-on, whereas I can remove a tag if I had added the tag, I am the author of the add-on, or both.  Thanks!

A minor comment: the response to a disallowed request is currently 404 Not Found, but 403 Forbidden sounds more suitable.  But this is more of an aesthetic issue rather than a real issue.

And thanks for making the bug public, Wil.  I was 90% sure that it was not necessary to mark this bug as security-sensitive but not 100% sure.

Comment 12

[Jeff Balogh (:jbalogh)]

fcp: feel free to mark the status as Verified Fixed next time.  Thanks!

Comment 13

[fcp]

(In reply to comment #12)
> fcp: feel free to mark the status as Verified Fixed next time.  Thanks!
I did not know that I could do that, because the help explained that Verified meant “QA has looked at the bug ...”.  But I will next time.  Thanks for letting me know, Jeff!

Comment 14

[Fred Wenzel [:wenzel]]

(In reply to comment #13)
> the help explained that Verified meant “QA has looked at the bug ...”.

Yup, QA is a community effort at Mozilla as well :) Thanks for your help!

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

fcp: you can _always_ verify your own bugs; in fact, we encourage you to do so :-)

Thanks!

Comment 16

[fcp]

Wow.  I somehow thought that the “QA” in the help referred to a team of people specially chosen for QA.  Thanks for correcting me, Frédéric and Stephen!