Closed
Bug 1351608
Opened 7 years ago
Closed 7 years ago
Add eslint-plugin-no-unsanitized to eslint-plugin-mozilla
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement)
Tracking
(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)
RESOLVED
FIXED
mozilla56
People
(Reporter: freddy, Assigned: freddy)
References
Details
(Keywords: sec-audit, sec-want, Whiteboard: [adv-main56-][post-critsmash-triage])
Attachments
(2 files, 9 obsolete files)
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.
Comment 1•7 years ago
|
||
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
Comment 2•7 years ago
|
||
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?
Assignee | ||
Comment 3•7 years ago
|
||
(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".
Comment 4•7 years ago
|
||
(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.)
Assignee | ||
Comment 5•7 years ago
|
||
(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.
Comment 6•7 years ago
|
||
(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.
Comment 7•7 years ago
|
||
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
Comment 8•7 years ago
|
||
Dropping from my direct todo list, until we've decided exactly what we want to do.
Assignee: standard8 → nobody
Assignee | ||
Updated•7 years ago
|
Summary: Add eslint-plugin-no-unsafe-innerhtml to eslint-plugin-mozilla → Add eslint-plugin-no-unsanitized to eslint-plugin-mozilla
Assignee | ||
Comment 9•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c252626a63cc3f00a431686085cfa128ff645b2b
Assignee | ||
Comment 10•7 years ago
|
||
Can't use mozreview for security sensitive bugs. No need to review anytime soon, Monday is a holiday in Germany :-)
Assignee | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
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).
Assignee | ||
Comment 15•7 years ago
|
||
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>
Assignee | ||
Comment 16•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Depends on: CVE-2017-7795, CVE-2017-7798
Assignee | ||
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8873924 -
Attachment is obsolete: true
Attachment #8877133 -
Flags: review?(standard8)
Assignee | ||
Comment 19•7 years ago
|
||
next try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e36de9aed39df720cbb932f09c99f4bb7245029d
Assignee | ||
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8878074 -
Attachment is patch: true
Comment 22•7 years ago
|
||
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+
Assignee | ||
Comment 23•7 years ago
|
||
rebased, taking over r+
Attachment #8878074 -
Attachment is obsolete: true
Attachment #8880307 -
Flags: review+
Assignee | ||
Comment 25•7 years ago
|
||
rebased, carrying over r+
Attachment #8880307 -
Attachment is obsolete: true
Attachment #8881924 -
Flags: review+
Assignee | ||
Comment 26•7 years ago
|
||
rebased, carrying over r+
Attachment #8877133 -
Attachment is obsolete: true
Attachment #8880308 -
Attachment is obsolete: true
Attachment #8881925 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d95016c5fc991c075d10b4591491d50650a7c487 https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd9e27f0fa16bd65edc431ca37bbcd5ad72b3bd
Keywords: checkin-needed
Comment 28•7 years ago
|
||
Looks like Aryx backed this out yesterday because eslint can't find the new plugin "eslint-plugin-no-unsanitized" https://hg.mozilla.org/integration/mozilla-inbound/rev/36ea00c880db https://hg.mozilla.org/integration/mozilla-inbound/rev/ed5cc84d317f https://treeherder.mozilla.org/logviewer.html#?job_id=110498091&repo=mozilla-inbound
Flags: needinfo?(fbraun)
Assignee | ||
Comment 30•7 years ago
|
||
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)
Comment 31•7 years ago
|
||
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.
Comment 32•7 years ago
|
||
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)
Comment 33•7 years ago
|
||
Comment 34•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2440cec0dbdfefb63729601239b18f4c824d63 https://hg.mozilla.org/integration/mozilla-inbound/rev/003e74a2a953449614ea1355f7f5b9dfdcf1654c
Comment 35•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ab2440cec0db https://hg.mozilla.org/mozilla-central/rev/003e74a2a953
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Comment 36•7 years ago
|
||
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)
Comment 37•7 years ago
|
||
(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)
Updated•7 years ago
|
Whiteboard: [adv-main56-]
Updated•7 years ago
|
Whiteboard: [adv-main56-] → [adv-main56-][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
Updated•6 years ago
|
Product: Testing → Firefox Build System
Updated•5 years ago
|
Updated•5 years ago
|
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•