349435 - Security bug: anyone can update any add-on

Status: VERIFIED FIXED

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

Description

[Justin Scott [:fligtar]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

Because AMO does not check where it's getting its post values from and passes information through hidden form fields, and because the check to see if you're the author of the addon is done with step2, not the last step, the following can be done:

Reproducible: Always

Steps to Reproduce:
1. Create a new AMO developer account. This only requires a valid email.
2. Make a naughty .xpi file and login to AMO. Go to submit a new extension, upload the xpi, and get to step 2 where the name/homepage/categories/etc page is. View the source, copy to a new file, change hidden fields to text, change the form action to "https://addons.mozilla.org/developers/additem.php?function=additem3" and save to your website or wherever. 
3. Go to your newly made page, change the values in the textboxes. Change mode from new to update, change the GUID to the guid of an existing addon, change the item id to an existing ID, and change the version. Hit submit.
Actual Results:  
Your new account will be the author of the existing addon, it will submit the xpi you uploaded as a new version to that addon, and any changes you made to description/etc will be made.


I did a test of this on my chameleon account so as to not mess up anything in production. To test yourself:
1) Create a new account or login with your existing account at https://update-staging.mozilla.org/~fligtar/v1/developers/
2) Go to http://www.fligtar.com/dfjslkjflsdkjflsecretplacelsjdfds/test.html and change the author to your account email and give it a new version number and hit submit.
3) It will submit the new version. If you had submitted a .xpi when you logged in, it would have done that part as well.

Comment 1

[Mike Shaver (:shaver emeritus)]

Raising severity, because the bugmail for dbaron's removal of the security-sensitive flag likely went out to everyone watching this component's synthetic QA as well as everyone watching morgamic.  That means we'll probably need to fix this before Remora, hopefully it's not too hard!

Comment 2

[Michael Morgan [:morgamic]]

I am working on a patch to check the author's authenticity through his session uid.

Comment 3

[Daniel Veditz [:dveditz]]

This submission would go into the review queues, right? Is there something we should tell the reviewers to watch out for (like the author name changing) until this gets fixed?  (ignoring the possibility that an evil reviewer is in on the scheme.)

Comment 4

[Justin Scott [:fligtar]]

Morgamic is working on a fix right for it right now. This hole has apparently been there for a long time, but as far as I know we've never had any complaints from an author about someone hijacking their extension. I don't think anyone knows about it, unless they happened to see this bug during the 2-3 minutes it was public yesterday.

Comment 5

[Michael Morgan [:morgamic]]

Attachment: Patch to clean up other permissions check, copy it, and place it in front of step3 INSERTs as a stop-gap.

Not pretty, but I've tested it and it should prevent someone from hijacking an add-on by posting from an arbitrary account.  Any time a numeric ID is passed, it will check ownership against the uid in the SESSION.

Comment 6

[Justin Scott [:fligtar]]

Comment on attachment 234928 [details] [diff] [review]
Patch to clean up other permissions check, copy it, and place it in front of step3 INSERTs as a stop-gap.

Editors should not be able to submit updates to add-ons. Only admins.

Comment 7

[Michael Morgan [:morgamic]]

Attachment: Retry -- this time without granting editors god mode.

Comment 8

[Justin Scott [:fligtar]]

Comment on attachment 234930 [details] [diff] [review]
Retry -- this time without granting editors god mode.

Looks good.

Comment 9

[Michael Morgan [:morgamic]]

This has been checked-in and deployed.  Took a little longer than expected because I had to get things up to date in my dev copy and we also had to wait a bit for a DB update to push the latest v1 batch.