Closed Bug 1351608 Opened 3 years ago Closed 2 years ago

Add eslint-plugin-no-unsanitized to eslint-plugin-mozilla

Categories

(Firefox Build System :: Lint and Formatting, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: freddyb, Assigned: freddyb)

References

Details

(Keywords: sec-audit, sec-want, Whiteboard: [adv-main56-][post-critsmash-triage])

Attachments

(2 files, 9 obsolete files)

52.19 KB, patch
Details | Diff | Splinter Review
48.86 KB, patch
Details | Diff | Splinter Review
I suggest we add the eslint-plugin-no-unsafe-innerhtml to eslint-plugin-mozilla.

This currently has about ~44 issues, that will need some exceptions (and future work) and probably a blanket exception for tests.
Blocks: 1351610
I can take this for moving adding the npm module. I'll check versions of the other modules at the same time.

It'll likely be late next week before I get to it.
Assignee: nobody → standard8
What class of issues is this going to prevent in Firefox? What's your assessment on how many of those 44 issues are actually issues in Firefox vs. things we'd add exceptions for?
(In reply to Dão Gottwald [::dao] from comment #2)
> What class of issues is this going to prevent in Firefox? 

XSS in chrome or content privileged code. Bug 1346653 and bug 873966 come to mind.

> What's your assessment on how many of those 44 issues are actually issues in Firefox vs.
> things we'd add exceptions for?

I have not audited most of those issues and they are generally not that easy to audit. 
The idea is to move them from "not sure whether this stuff is properly escaped and where" into "yup, definitely not dangerous".
(In reply to Frederik Braun [:freddyb] from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > What class of issues is this going to prevent in Firefox? 
> 
> XSS in chrome or content privileged code.

Well, yes, sure. I'm trying to get an idea of whether there's a pattern in Firefox, how common this problem is for the cases where Firefox uses innerHTML.

> Bug 1346653 and bug 873966 come to
> mind.
> 
> > What's your assessment on how many of those 44 issues are actually issues in Firefox vs.
> > things we'd add exceptions for?
> 
> I have not audited most of those issues and they are generally not that easy
> to audit. 
> The idea is to move them from "not sure whether this stuff is properly
> escaped and where" into "yup, definitely not dangerous".

I find writing code and then adding eslint exceptions rather cumbersome. Can we be smarter and somehow treat innerHTML in a special way when accessed from chrome? We have a long history with stuff like this, e.g. chrome code isn't vulnerable to content overwriting its own DOM methods (chrome code had to use wrappedJSObject to see those.)
(In reply to Dão Gottwald [::dao] from comment #4)
> I find writing code and then adding eslint exceptions rather cumbersome. Can
> we be smarter and somehow treat innerHTML in a special way when accessed
> from chrome? We have a long history with stuff like this, e.g. chrome code
> isn't vulnerable to content overwriting its own DOM methods (chrome code had
> to use wrappedJSObject to see those.)

Well, ideally you'd write code that doesn't cause an eslint error.
In a slightly more inconvenient case you'd write code that causes it and then rewrite some tiny bits to make the error go away, instead of adding an exception.

But I totally get your point. Do you have something in mind?
Rewriting how assignments to innerHTML in the platform work seems a bit drastic, I'm not sure if you're thinking about that.

We do have decent sanitizers like https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIParserUtils#sanitize btw.
(In reply to Frederik Braun [:freddyb] from comment #5)
> (In reply to Dão Gottwald [::dao] from comment #4)
> > I find writing code and then adding eslint exceptions rather cumbersome. Can
> > we be smarter and somehow treat innerHTML in a special way when accessed
> > from chrome? We have a long history with stuff like this, e.g. chrome code
> > isn't vulnerable to content overwriting its own DOM methods (chrome code had
> > to use wrappedJSObject to see those.)
> 
> Well, ideally you'd write code that doesn't cause an eslint error.
> In a slightly more inconvenient case you'd write code that causes it and
> then rewrite some tiny bits to make the error go away, instead of adding an
> exception.

At that point this is a circular argument, as I'm trying to figure out if there are alternatives to having this error in the first place ;)

> But I totally get your point. Do you have something in mind?
> Rewriting how assignments to innerHTML in the platform work seems a bit
> drastic, I'm not sure if you're thinking about that.
> 
> We do have decent sanitizers like
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIParserUtils#sanitize btw.

Could we automatically sanitize and make chrome code jump through hoops to get the unsanitized version, much like wrappedJSObject? I'm just thinking out loud here. Ultimately this is probably a question for DOM peers.
This furthers the discussion of Bug 1347455, so marking it as sensitive until we know otherwise.

Also I wanted to highlight a discussion on moving this rule to be more configurable and potentially under a Mozilla repo: https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/issues/45

I'm happy to enable this rule but perhaps there is a config that we can choose that minimises the exceptions required in setting this  up.
Group: core-security-release
Keywords: sec-audit
Dropping from my direct todo list, until we've decided exactly what we want to do.
Assignee: standard8 → nobody
Summary: Add eslint-plugin-no-unsafe-innerhtml to eslint-plugin-mozilla → Add eslint-plugin-no-unsanitized to eslint-plugin-mozilla
Can't use mozreview for security sensitive bugs.
No need to review anytime soon, Monday is a holiday in Germany :-)
Assignee: nobody → fbraun
Status: NEW → ASSIGNED
Attachment #8873922 - Flags: review?(standard8)
This patch makes the existing violations go away. I intend to file good first bugs for those.

I auto-fixed those with a tiny script [1] and the files are now error free according to another run of ./mach eslint. But we'll see if this works on try (comment above)


[1] https://gist.github.com/mozfreddyb/5e276f2832aa2e487fad8ce3b01de7dd
Attachment #8873924 - Flags: review?(standard8)
Comment on attachment 8873924 [details] [diff] [review]
0002-Bug-1351608-comment-out-existing-violations-to-no-un.patch

Review of attachment 8873924 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to do, however are we sure none of these are currently security issues? I'm a bit concerned that if we land it as-is, then it'll potentially give people targeted places to look for issues.
Attachment #8873924 - Flags: review?(standard8)
Comment on attachment 8873922 [details] [diff] [review]
0001-Bug-1351608-Add-eslint-plugin-no-unsanitized-to-esli.patch

Review of attachment 8873922 [details] [diff] [review]:
-----------------------------------------------------------------

This will need a little extra - as the shrinkwap needs updating, but also there's a special package we put on the server.

I can generate those once we've worked out about the second patch.
Attachment #8873922 - Flags: review?(standard8)
On the try build, are you sure the devtools failures are actually intermittent? It looks like something needs regenerating according to the output (and given it is across all builds/platforms, that suggests something more wrong).
Thanks for the useful comments. I will work through them one-by-one.
I'll start by looking more closely at the violations that are not in tests.
The evaluation will be documented in <https://docs.google.com/spreadsheets/d/1PtTT_9Iz_yHFxN_oRMpkL0ZN1tisdBIn7L_YhtWvbOw/edit#gid=0>
(In reply to Mark Banner (:standard8) from comment #12)
> This looks reasonable to do, however are we sure none of these are currently
> security issues? I'm a bit concerned that if we land it as-is, then it'll
> potentially give people targeted places to look for issues.

My initial idea was to have the linter and the fixes run in parallel, so we don't get more violations that might cause us to start all over again.
However, I found two critical vulnerabilities, that I'd prefer having fixed before landing this (I'll mark them as blocking).


(In reply to Mark Banner (:standard8) from comment #13)
> Comment on attachment 8873922 [details] [diff] [review]
> 0001-Bug-1351608-Add-eslint-plugin-no-unsanitized-to-esli.patch
> 
> Review of attachment 8873922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This will need a little extra - as the shrinkwap needs updating, but also
> there's a special package we put on the server.
> 
> I can generate those once we've worked out about the second patch.

Not sure I understand, but your help is always appreciated.


(In reply to Mark Banner (:standard8) from comment #14)
> On the try build, are you sure the devtools failures are actually
> intermittent? It looks like something needs regenerating according to the
> output (and given it is across all builds/platforms, that suggests something
> more wrong).

True, I fixed that in the next commit, being uploaded nowish.
You mentioned the shrinkwrap needs updating. This is actually too colloquial for me to understand what you're talking about. Please advise :-)
Attachment #8873922 - Attachment is obsolete: true
Attachment #8877132 - Flags: feedback?(standard8)
Attachment #8873924 - Attachment is obsolete: true
Attachment #8877133 - Flags: review?(standard8)
FYI, the try-push was successful[1]. and I'd like to land this as soon as the blockers are addressed, which have land approval for June 26th.
I suspect that the linter rule might find new violations by then so I plan to retry proactively in the next week.



[1] There was an intermittent failure that ran successful in the end. However, the test is now disabled according to https://bugzilla.mozilla.org/show_bug.cgi?id=1294025#a26611814_448747
This is an updated patch. The updates were to run our update script that uploads a fresh package to the releng servers for ESLint to download, and then updates the appropriate files.

This means it should run correctly on the build servers.
Attachment #8877132 - Attachment is obsolete: true
Attachment #8877132 - Flags: feedback?(standard8)
Attachment #8878074 - Flags: review+
Attachment #8878074 - Attachment is patch: true
Comment on attachment 8877133 [details] [diff] [review]
0002-Bug-1351608-comment-out-existing-violations-to-no-un.patch

Review of attachment 8877133 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8
Attachment #8877133 - Flags: review?(standard8) → review+
rebased, taking over r+
Attachment #8878074 - Attachment is obsolete: true
Attachment #8880307 - Flags: review+
rebased, taking over r+
Attachment #8880308 - Flags: review+
rebased, carrying over r+
Attachment #8880307 - Attachment is obsolete: true
Attachment #8881924 - Flags: review+
rebased, carrying over r+
Attachment #8877133 - Attachment is obsolete: true
Attachment #8880308 - Attachment is obsolete: true
Attachment #8881925 - Flags: review+
Keywords: checkin-needed
looking into it.
Flags: needinfo?(fbraun)
rebased and pushed to try some time ago, but the error does not make a lot of sense to me.
https://treeherder.mozilla.org/logviewer.html#?job_id=110774474&repo=try&lineNumber=4625
> [task 2017-06-29T18:37:36.436930Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
> [task 2017-06-29T18:37:36.437900Z] ln: failed to create symbolic link 'node_modules/eslint-plugin-mozilla': File exists

Standard8 said he will look into it.
Flags: needinfo?(standard8)
Sorry for the delay here, I didn't get time in the end. I've just had a look now and there's something weird going on with the dependencies and npm 5 at the moment. I'll have to park it for now and take a look most likely on Tuesday morning.
I found a bit of time today to sort this out.

The issue was that the eslint.tar.gz file that contains the npm files & gets uploaded for the builders was also including our in-tree eslint-plugin-mozilla & spidermonkey-js. This is a side effect of my recent move to npm 5.

I had to adjust the update script for removing those directories before packaging.

Additionally for tools/lint/eslint/eslint-plugin-mozilla/package.json, I changed the eslint-plugin-no-unsanitized to be a peer dependency rather than a direct one. I think this works better for npm 5 at the moment. I'll look into it later if we should be have it as a real dependency or not.

I'll get these pushed to inbound in a few mins.
Attachment #8881924 - Attachment is obsolete: true
Attachment #8881925 - Attachment is obsolete: true
Flags: needinfo?(standard8)
https://hg.mozilla.org/mozilla-central/rev/ab2440cec0db
https://hg.mozilla.org/mozilla-central/rev/003e74a2a953
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I can't figure out what this line is doing here and removing it doesn't seem to break lint:
http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/modules/ExtensionsUI.jsm#442

Can it be removed?  (also, fwiw another similar line in the same file was also added with indentation that doesn't match the surrounding code...)
Flags: needinfo?(standard8)
(In reply to Andrew Swan [:aswan] from comment #36)
> I can't figure out what this line is doing here and removing it doesn't seem
> to break lint:
> http://searchfox.org/mozilla-central/rev/
> cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/modules/ExtensionsUI.jsm#442
> 
> Can it be removed?  (also, fwiw another similar line in the same file was
> also added with indentation that doesn't match the surrounding code...)

I think it can be removed, it looks like it might have been left in when rebasing after https://hg.mozilla.org/mozilla-central/rev/2abb397e917c landed.
Flags: needinfo?(standard8)
Whiteboard: [adv-main56-]
Whiteboard: [adv-main56-] → [adv-main56-][post-critsmash-triage]
Group: core-security-release
Product: Testing → Firefox Build System
Keywords: sec-audit, sec-want
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.