Check add-ons for security vulnerabilities at submission


Status Graveyard
Developer Pages
11 years ago
2 years ago


(Reporter: fligtar, Assigned: rjwalsh)


Dependency tree / graph



(2 attachments)



11 years ago
Add-ons should be scanned via regex (and other methods) for security vulnerabilities upon submission. Problem files will be flagged for admin/security review.
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.


11 years ago
Blocks: 382708


10 years ago
Assignee: fligtar → nobody


10 years ago
Duplicate of this bug: 412095

Comment 3

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

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

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.
Assignee: nobody → clouserw
Ah, I was thinking of bug 444136
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: 

Feel free to change and comment.  I plan on cleaning it up at the end of the week.
The list looks really good- should this remain one monolithic bug for implementation or should we break it out into smaller tasks?
(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.
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)
Target Milestone: Future → 5.0.4
Created attachment 371374 [details]
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.
Created attachment 371376 [details]
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.


9 years ago
Priority: -- → P1
Target Milestone: 5.0.4 → 5.0.7
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.


9 years ago
Target Milestone: 5.0.7 → 5.0.8
Blocks: 441854
Blocks: 441852
Blocks: 444136
Assignee: clouserw → rjbuild1088
Blocks: 502267
Bumping to 5.0.9.  This is coming along fine and I expect we'll resolve it in the next milestone.
Target Milestone: 5.0.8 → 5.0.9


8 years ago
Duplicate of this bug: 380107
Duplicate of this bug: 375520

Comment 17

8 years ago
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.
Last Resolved: 8 years ago
Resolution: --- → FIXED
Verified FIXED; I've seen theme and add-on-specifc tests run on preview.
Product: → Graveyard
You need to log in before you can comment on or make changes to this bug.