522722 - Add test for new Function() to add-on validation

Status: RESOLVED DUPLICATE of bug 518439

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

Description

[:Gijs (he/him)]

Copied from my mail to amo-editors:

Especially authors not
very aware of the security risks use it as a shortcut to manipulate webpage
content more easily - and then end up with gaping security holes in their
code. It's possible it would be nice to have a similar check for unwrapping
such wrappers and using the unwrapped object, which tends to have the same
problem (IIRC it's "wrappedJSObject", although I'm not 100% sure if that's
correct, and more importantly, there are a lot of other wrappers where one
needs to access that property, and just using it does not necessarily mean
there is unsafe access going on, depends where the object in question is
coming from etc.).

So, to be clear, we can test all their files for wrappedJSObject and/or similar, and test chrome.manifest specifically for "xpcnativewrappers=no" instructions (where you may want to be generous about whitespace).

Comment 1

[Dave Garrett]

Looks like we already are:

http://viewvc.svn.mozilla.org/vc/addons/trunk/site/app/controllers/components/validation.php?view=markup

$unsafePatterns = array('/nsIProcess/',
                    '/\.launch\s*\(/',
                    '/\beval\s*\(/',
                    '/\bsetInterval\s*\(/',
                    '/\bsetTimeout\s*\(/',
                    '/<browser\s*(?![^<>]*type=["\'])[^<>]*>/i',
                    '/<iframe\s*(?![^<>]*type=["\'])[^<>]*>/i',
                    '/xpcnativewrappers=/',
                    '/evalInSandbox/',
                    '/mozIJSSubscriptLoader/',
                    '/wrappedJSObject/');

Is something else needed here?

Comment 2

[Justin Dolske [:Dolske]]

Looks good to me, CCing mrbkap to see if anything else comes to mind.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Only thing I can think of is the Function constructor (called either as |new Function()| or simply |Function()|)?

Comment 4

[:Gijs (he/him)]

(In reply to comment #1)
> Looks like we already are:
> 
> http://viewvc.svn.mozilla.org/vc/addons/trunk/site/app/controllers/components/validation.php?view=markup

Uhm, https://addons.mozilla.org/en-US/developers/versions/validate/81168#test-results-67077 shows passes only, but https://addons.mozilla.org/en-US/firefox/files/browse/67077/1 has an xpcnativewrappers=no line in its chrome.manifest...

Maybe chrome.manifest isn't actually checked for this regex?

Comment 5

[:Gijs (he/him)]

(In reply to comment #4)
> (In reply to comment #1)
> > Looks like we already are:
> > 
> > http://viewvc.svn.mozilla.org/vc/addons/trunk/site/app/controllers/components/validation.php?view=markup
> 
> Uhm,
> https://addons.mozilla.org/en-US/developers/versions/validate/81168#test-results-67077
> shows passes only, but
> https://addons.mozilla.org/en-US/firefox/files/browse/67077/1 has an
> xpcnativewrappers=no line in its chrome.manifest...
> 
> Maybe chrome.manifest isn't actually checked for this regex?

Ugh, except apparently those results were old - they should state if they were done using a different version of the add-on! :-\

Comment 6

[:Gijs (he/him)]

Updating the summary per comment #3.

Comment 7

[Wil Clouser [:clouserw]]

Thanks Gijs.  RJ is going to be in that file anyway this week, so I'll assign to him.  RJ - if you could add this check to the tests that would be awesome.  P5 though, so don't stress about it.

Also, to address comment 5, test results are deleted daily so old results shouldn't be a problem anymore.

Comment 8

[:Gijs (he/him)]

(In reply to comment #7)
> Also, to address comment 5, test results are deleted daily so old results
> shouldn't be a problem anymore.

This is really a bit of a sideshow, but just out of interest - any reason they can't be auto-regenerated once a new XPI is uploaded? That seems like the most "natural" way to get up-to-date info for any nomination/update.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

The comments in bug 523888 can shed some light into why the current test result caching works this way.

Comment 10

[RJ Walsh [:rjwalsh]]

Unless I'm totally missing something here, this appears to be resolved in bug 518439 ?

Comment 11

[Wil Clouser [:clouserw]]

RJ brings up a good point