User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/602.4.8 (KHTML, like Gecko) Version/10.0.3 Safari/602.4.8 Steps to reproduce: Using Firefox 52.0 on macOS Sierra, I added the Containers experiment via Test Pilot and created new containers: - Click on the Containers toolbar icon - Select the "+" option - Create new containers with the following names: '"><h1>Test</h1> <marquee>abc Actual results: In both cases, the HTML was accepted and stored by the Containers popout. This is presented in an unsanitised fashion upon clicking the Containers toolbar icon. Expected results: The HTML should be sanitised upon being stored in the browser. I have flagged this issue as Security-Sensitive on the basis that XSS-like attacks may be possible, although am currently unable to identify a suitable Proof of Concept.

:baku, :jkt, where does this code live? I assume on github somewhere or something like that? I guess we need to keep the issue here given there's no security flags on github? Can you take a look?

Found https://github.com/mozilla/testpilot-containers/blob/master/webextension/js/popup.js , CC'ing other committers. TBH, I'm flabbergasted this was released like this. :-\ :kmag, how much sheltering does webextensions give us here?

Well, the extension doesn't change the default CSP policy, so it won't be possible to inject inline or remote scripts this way. There may be some more subtle way to exploit it, though.

Incidentally, since you're already using template strings for this, it should be pretty easy to fix this issue with something like this: http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#94-128

Granted this wouldn't be great, I know

Whoops clicking the take button auto commits the comment I half wrote. Luckily this is only self-xss but not great I know. We were using textContent before dunno why this changed.

(In reply to Jonathan Kingston [:jkt] from comment #6) > Luckily this is only self-xss but not great I know. Nope. This issue also applies to text from tab titles.

Used "git format-patch -1" to export. "git am 0001-Fix-escaping-of-container-names-in-popups.-Bug-13466.patch" should work after checking out the file. :groovecoder we should merge this on monday before the release. This uses kmags solutions which breaks the STR.

Comment on attachment 8846457 [details] [diff] [review] 0001-Fix-escaping-of-container-names-in-popups.-Bug-13466.patch Review of attachment 8846457 [details] [diff] [review]: ----------------------------------------------------------------- This is clearly the simpler fix, but using textContent would make me happier. Please switch to that in a followup issue after this is released. ::: webextension/js/popup.js @@ +26,5 @@ > + * @return {string} The escaped string. > + */ > +function escapeXML(str) { > + const replacements = {"&": "&amp;", "\"": "&quot;", "'": "&apos;", "<": "&lt;", ">": "&gt;"}; > + return String(str).replace(/[&"''<>]/g, m => replacements[m]); Nit: looks like ' is in here twice? Also, why do you need the String() conversion?

Yeah I started down the textContent path and realised how many there were. I would have had to do setAttribute("title", `something ${string}`) also for many of the elements. I also started work on: https://github.com/jonathanKingston/eslint-plugin-unsafe-property-assignment which is still WIP (I was working on it till 2am). The intent is a basic catch of the issue here (it finds all the issues in the commit and also prevents assigning "innerHTML" to a variable so that it could be used also for var access of the property element[myKey]). ESLint is passed a hash of {prop: ['filters']} so that the developer is required to provide a tagged template function el.prop = filter`me ${var}`; whenever a string with variables are used. It isn't a sophisticated check that tracks execution however for our own test pilot developers it should likely catch any cases this would happen with again. My only issue left with the extension is the inability to run it in the wild: "ESLint couldn't find the plugin "eslint-plugin-unsafe-property-assignment". This can happen for a couple different reasons" however the test cases are all passing that use the rules so it is likely a config issue. Once I have the ESLint restriction in the build process I will file the follow up to move to use textContent and attrs. -- Copy and paste again(the only change I made was to fix our ESLint setup). I expect the String() is paranoia in-case some function ever sends a number / other.

Giving this a moderate rating based on comment 3 (that it's just HTML injection), but it would be sec-high if scripts can be injected also.

This fix went live in the 2.0.0 release: https://github.com/mozilla/testpilot-containers/commit/e3daf2e84216333338cd3a7c1ec6b565ba2f8de3 The fix was: https://github.com/mozilla/testpilot-containers/pull/368 Further sanitisation was added here: https://github.com/mozilla/testpilot-containers/pull/379 The extension was moved to: https://github.com/jonathanKingston/eslint-plugin-no-unescaped to better account for methods and assignments that would cause this issue. I'm speaking with the addons team and others if we can further get this as a requirement for all extensions etc.

Confirming the fix and sanitisation – thanks everyone!

Marking as verified as per comment#13. I also went through verification using the STR from comment#0 with the following build: * fx52.0.1, buildid: 20170316213829, changeset: 2f2b4a119565 using Containers Experiment 2.0.0 via Test Pilot * fx55.0a1, buildid: 20170322030208, changeset: 201231223cd4 using Containers Experiment 2.0.0 via Test Pilot Platforms being used: * macOS 10.12.3 x64 - PASSED * Windows 10 Pro x64 VM - PASSED * Ubuntu 16.04.2 LTS x64 VM - PASSED Test Cases Used: * creating a new container via "+" under the container toolbar * edited a container via "+" under the container toolbar * creating a new container via about:preferences#containers * editing a container via about:preferences#containers

Clearing my needinfo.