371210 - Check add-ons for security vulnerabilities at submission

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, enhancement, P1)

Description

[Justin Scott [:fligtar]]

Add-ons should be scanned via regex (and other methods) for security vulnerabilities upon submission. Problem files will be flagged for admin/security review.

Comment 1

[Wladimir Palant]

Keywords: wrappedJSObject, eval, setTimeout (with a string as first parameter). I wonder how you intend to take care of the noise however - while these constructs are rarely necessary they are very overused. And of course there are cases of "vulnerable by concept" like Firefoxit but I guess this cannot be caught automatically.

Comment 3

[David A. Wheeler]

Some add-ons should have almost NO capabilities, certainly no scripting at all.  Language packs are particularly important, because for less-common languages they can't be fully analyzed by most people.  We now have a virus that slipped through because of this lack of early scanning: bug 432406.
Granted, it was found by later scanning, but it'd be better to require language packs to follow a specific format, and reject anything else (e.g., limited list of tags, etc.).

Comment 4

[David A. Wheeler]

For more discussion about scanning language packs, see bug 390657.

Comment 5

[Wil Clouser [:clouserw]]

I'm assigning this to myself to come up with a list of things to check for (which I'll ask for help on once I write some stuff down).  I thought fligtar was already working on this, but I can't find anything regarding that.

I'll assign to someone else for implementation.

Comment 6

[Wil Clouser [:clouserw]]

Ah, I was thinking of bug 444136

Comment 7

[Wil Clouser [:clouserw]]

I don't know of a resource that explains exactly what makes an add-on an add-on.  If someone has one please add a link.  That said, here is what I've got so far:

http://docs.google.com/Doc?id=dcfr9qrp_1c2pgcsfh&invite=gbvkhp5 

Feel free to change and comment.  I plan on cleaning it up at the end of the week.

Comment 8

[Nick Nguyen [:osunick]]

The list looks really good- should this remain one monolithic bug for implementation or should we break it out into smaller tasks?

Comment 9

[Wil Clouser [:clouserw]]

(In reply to comment #8)
> The list looks really good- should this remain one monolithic bug for
> implementation or should we break it out into smaller tasks?

One bug is fine for now.  It's all tightly related.

Comment 10

[Nick Nguyen [:osunick]]

changing to 5.0.4 for now to make sure we discuss in the planning meeting (can move back to future if that's where it belongs)

Comment 11

[Wil Clouser [:clouserw]]

Attachment: current work flow

Just so we're all on the same page, this is the current work flow when uploading a new add-on or updating an old one.

Comment 12

[Wil Clouser [:clouserw]]

Attachment: mockups v1

The first round of mockups.  I'm suggesting replacing the red box in the "failed" section of the previous attachment with the red boxes here and if all tests pass turning it to green and leaving it visible when we switch to the "success" page.

As noted in the mockup, all tests after the first one are run via AJAX in parallel.  This should discourage running out of memory and encourage using multiple servers to speed things up.  There are only three test sections on most of these mockups but it's easy to add more as we expand our checks.  There will also be UI for developers/editors/admins to run these tests against their add-ons at anytime.

One thing I'm wondering is do we want to alert add-on authors that we are flagging their add-ons when they first upload?  There are a lot of "flag" cases in the spec but they are flags for a reason - they might not be a problem.  I was thinking of putting a warning on the "success" mockup if something triggered a flag but thought it might cause more harm than good:
- It would have to be very carefully worded to avoid confusion
- It still looks like an error and there might not be anything wrong with it
- What can an author do at that point?  The system has already created their add-on.  If they want to fix it they'd have to upload a new version?  ugh.  I guess we could prompt them "Do you want to continue despite the warning?" but that leads back to confusion.  Anyway, open to ideas.

Requesting feedback on all of it.  Thanks.

Comment 13

[u278084]

I've been thinking about whether it is a good idea to alert authors at upload time, or to make it a separate process that they run whenever they want. The nice thing about showing problems at upload time is that people will see the results and are given a chance to correct them. As opposed to optional, which people tend to ignore (many nominated extensions still get denied because of lack of user reviews, even though we mention it. And it's next to a bright yellow asterisk). However, I'm hopeful that people who want a quick review will go through the added effort.

I guess this really depends on implementation. For doing simple regex, you can end up with tons of false-positives, and this could just confuse people into fixing flags that really aren't problems at all. But the spec looks good at reducing the number of these.

I'm for showing flags as a separate process. Regardless of whether you show flags at upload time or not, the entire tool needs to be carefully worded.

Comment 14

[Wil Clouser [:clouserw]]

Bumping to 5.0.9.  This is coming along fine and I expect we'll resolve it in the next milestone.

Comment 17

[RJ Walsh [:rjwalsh]]

Based on the completion of bug 498679, this bug is now resolved.  Automatic testing on upload will land in 5.0.9 next week.  If anyone sees a reason to re-open this, feel free to do so.

Comment 18

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

Verified FIXED; I've seen theme and add-on-specifc tests run on preview.