Closed Bug 640855 Opened 13 years ago Closed 13 years ago

Validator: Overzealous "Dangerous Global Object" warnings

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Q2 2011

People

(Reporter: nmaier, Assigned: basta)

References

Details

The "Dangerous Global Object" should only appear when actually attempting to modify a built-in object prototype, or when it comes to Function, initiating a new instance.

Examples for expected warnings:
- Function(foo)
eval replacement

- Array.prototype = foo
messing with built-in prototype

- Array.prototype.bar = foo
messing with built-in prototype properties


Examples for expected OK (no warning):
- (foo instanceof Function)
checking type of foo is Function

- Array.prototype.forEach(arr, func)
Calling forEach on an array-like object
Assignee: nobody → mbasta
Regarding the use of Function, all references should be flagged as dangerous. The specific scenario involving "instanceof" is possibly the only exception. Since there is no instanceof support in the validator (nor will there likely be for a very long time), I'll just have the validator ignore the right hand side of any binary expression where the right hand side is an identifier called "Function".


With regard to the various prototypes, those can likely be changed from dangerous to read-only.

Jorge: What is your opinion on this? Is there any risk in making these changes?
I'm with you on the change to the Function detection. I didn't quite understand your plan for the prototype checks. We basically want to avoid the prototype from being changed in any way. If you can mark them as read-only so that it only flags those changes, then that sounds ideal.
(In reply to comment #1)
> Regarding the use of Function, all references should be flagged as dangerous.
> The specific scenario involving "instanceof" is possibly the only exception.
> Since there is no instanceof support in the validator (nor will there likely be
> for a very long time), I'll just have the validator ignore the right hand side
> of any binary expression where the right hand side is an identifier called
> "Function".

This could be used to hide eval()s from the validator then:
var f = Function;
(new f("alert('yep')"))()
On the other hand the aim of the validator was never to catch folks trying to
game the system (an impossible task)...
In the end it should be a balance between the fewest possible false-positives
and still catching the important and nasty stuff authors occasionally and
without bad intent release to the wild.

> With regard to the various prototypes, those can likely be changed from
> dangerous to read-only.
> 
> Jorge: What is your opinion on this? Is there any risk in making these changes?

(In reply to comment #2)
> [...] I didn't quite understand
> your plan for the prototype checks. We basically want to avoid the prototype
> from being changed in any way. If you can mark them as read-only so that it
> only flags those changes, then that sounds ideal.

These prototype accesses aren't in fact "Dangerous" per-se. We're on the lookout for code modifying/extending the built-in prototypes, as this creates incompatibilities with other add-ons or even the browser itself.
The following gist demonstrates such issues:
https://gist.github.com/798599

To be more precise, we actually only want to detect prototype changes in shared scopes, e.g. the scope of browser.xul. It is actually OK to modify the built-in prototypes in non-shared/add-on owned scopes, such as all own windows, js-modules, js-components, workers and bootstrap.js.
I, however, understand that the validator right now has no knowledge about what has to be considered a "shared" scope and what is not.

As such, I would suggest to reword the warning for the prototype stuff, to something along the lines of
"Built-in prototype modification"
"Modifying the prototypes of built-in objects, such as Object, String, Array, will create incompatibilities in overlay scripts. Hence it is prohibited to make such modifications in overlays."

(new Function("..."))) is the same as eval, and hence might in fact be dangerous (as in insecure).
> This could be used to hide eval()s from the validator then:
> var f = Function;
> (new f("alert('yep')"))()
> On the other hand the aim of the validator was never to catch folks trying to
> game the system (an impossible task)...

That's the point. It would only apply for the instanceof operator. There doesn't appear to be any harm in that. 'Function' is (and will continue to be) marked as dangerous 100% of the time, unless specific exceptions prevent it from being invoked as such.

> To be more precise, we actually only want to detect prototype changes in shared
> scopes, e.g. the scope of browser.xul. It is actually OK to modify the built-in
> prototypes in non-shared/add-on owned scopes, such as all own windows,
> js-modules, js-components, workers and bootstrap.js.
> I, however, understand that the validator right now has no knowledge about what
> has to be considered a "shared" scope and what is not.

The only prototypes marked as dangerous right now are for the main types: Object.prototype, Array.prototype, Number.prototype, Date.prototype, String.prototype, RegExp.prototype, and Boolean.prototype. All other prototypes are treated as lazily-extended JS objects.

Creating a unique message for the prototypes will be a lot of work and shouldn't be expected any time soon. As it stands, all predefined entities are read-only unless an exception is added. There's currently no way to distinguish between these. The default message for overwriting or modifying global entities should be sufficient for now.
Component: Admin/Editor Tools → Developer Pages
QA Contact: admin-tools → developers
Target Milestone: --- → 6.0.7
Depends on: 648102
Target Milestone: 6.0.7 → Q2 2011
Depends on: 650728
The fix has been pushed to mozilla/amo-validator:

https://github.com/mozilla/amo-validator/commit/c85bea8a537a734c9405a62a2b190ff6a686dc85

Tests were updated to compensate for the change in functionality.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.