661937 - Flag executable code in themes

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect, P2)

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110513 Firefox/6.0a1
Build Identifier: 

The old validator had tests for executable code in themes. Since themes are generally not allowed to contain executable code and are often reviewed as if they have none, this is incredibly important.

Among things that need to be flagged are:

XBL:
• xbl:constructor/xbl:destructor elements
• xbl:field elements with any contents
• xbl:property elements with onget/onset attributes
• xbl:getter/xbl:setter elements
• Any script, xul:script, or on* attributes under xbl:contents elements

Filetypes:
• *.js
• *.xul with any script contents
• *.html with any script contents

Anything I might be missing. When we're clear on everything that needs to be flagged, I'll create a test add-on if need be.

Reproducible: Always

Comment 1

[Kris Maglione [:kmag]]

Looking at the Remora validator, the following are also flagged:

• xbl:implementation, xul:browser, iframe, embed, object elements
• javascript: and data: anywhere. I'm not entirely sure where these should be flagged, but certainly the latter should be in -moz-binding values.
• *.jsm files
• Any declaration other than skin or style in chrome.manifest

Comment 2

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

The remora patch is at bug 539064. If you need access to this bug, just let me know.

Comment 3

[Matt Basta [:basta]]

For XUL/HTML files, would it be enough to ban any element with the XBL xml namespace?

Also, we currently throw a warning if -moz-binding is used in any CSS (no matter what file it's found in). Is this sufficient?

Comment 4

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

(In reply to comment #3)
> For XUL/HTML files, would it be enough to ban any element with the XBL xml
> namespace?

XBL declarations are normally done in .xbl or .xml files. There's no hint of a binding being used by inspecting the XUL or HTML files where the binding is included.

> Also, we currently throw a warning if -moz-binding is used in any CSS (no
> matter what file it's found in). Is this sufficient?

Do you mean this warning already exists for themes?  I think that should be good enough to detect custom binding usage.

Comment 5

[Matt Basta [:basta]]

As for bug 539064, I don't have access. I can apply whatever they've got as soon as I can get access.

> Do you mean this warning already exists for themes?  I think that should be good enough to detect
> custom binding usage.

It already exists for everything.

Comment 6

[Kris Maglione [:kmag]]

Matt: It looks like the current test flags https?, ftp, or data: URLs in -moz-binding. That's probably sufficient, although the test should probably flag any URL that starts with any protocol other than chrome: or resource:. We shouldn't need anything special for themes.

Comment 7

[Matt Basta [:basta]]

https://github.com/mozilla/amo-validator/pull/53

Comment 8

[Kris Maglione [:kmag]]

Attachment: Testcase

There are still some problems with this. Testcase attached.

• It doesn't take into account namespace when testing for XBL elements, thus failing for the common case in XBL files where XBL is the default namespace.

• It doesn't catch on* event listeners, which leaves a trivial code path to exploit.

I also noticed, accidentally, that the lack of locale lines is flagged in chrome.manifest, but I'm not bothered enough by it to file a bug.

Comment 9

[Matt Basta [:basta]]

I'll work on getting the default namespace passed down, didn't think of that.

As for on* even listeners, I'm not sure what you mean. You can't addEventListener, since that's JS and all JS is flagged. on* attributes should be flagged because they pass their values to test_js_snippet, which will then throw an error for containing JS.

I just patched the message about locale lines in chrome.manifest.

Comment 10

[Kris Maglione [:kmag]]

Re on* attributes, hm, I'm not sure what's going on, then, because the test XPI contains an iframe with an onload attribute that isn't flagged. I'll have to test with regular XUL and HTML files.

Also, are the XBL tags above run through text_js_snippet? If so, they should be automatically flagged. If not, that's another critical bug that needs to be filed.

Comment 11

[Matt Basta [:basta]]

Check this out, it should address your concerns:

https://github.com/mattbasta/amo-validator/commit/dab3d4a8bf108dd93822995cc0ac3e9de5cf956c

Comment 12

[Kris Maglione [:kmag]]

Seems to be mostly readme updates, which are quite nice granted, but don't really bear on the bug.

Comment 13

[Matt Basta [:basta]]

My bad, I suck at multitasking:

https://github.com/mattbasta/amo-validator/commit/4e8efb0a44971b96e22c8f33e42defff57e311e8

Comment 14

[Matt Basta [:basta]]

Merged:

https://github.com/mozilla/amo-validator/commit/3c3062cc3ea51f28abe70bd34031d99bf706a965

Comment 15

[krupa raj[:krupa]]

(In reply to comment #8)
 
> There are still some problems with this. Testcase attached.
> 
> • It doesn't take into account namespace when testing for XBL elements, thus
> failing for the common case in XBL files where XBL is the default namespace.

This is now a warning. verified at https://addons.allizom.org/en-US/developers/upload/dcee9ec607fd4dd2ac5f49d7c4171ec0 . Also, see post-fix screenshot.

> • It doesn't catch on* event listeners, which leaves a trivial code path to
> exploit.

I am not sure what was decided regarding this. Is this is still an issue, feel free to reopen.

> 
> I also noticed, accidentally, that the lack of locale lines is flagged in
> chrome.manifest, but I'm not bothered enough by it to file a bug.

I don't see any locale-related errors anymore.

Kris, thanks for attaching the test file. That helped a lot.

Comment 16

[krupa raj[:krupa]]

Attachment: post-fix screenshot