Closed
Bug 1275065
Opened 8 years ago
Closed 8 years ago
Unsafe innerHTML usage in b2gInstaller
Categories
(Firefox OS Graveyard :: B2gInstaller, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jovan.gerodetti, Assigned: marsf)
References
Details
Attachments
(3 files)
We didn't pass the AMO review because the installer contains unsafe innerHTML usages. This add-on is creating DOM nodes from HTML strings containing potentially unsanitized data, by assigning to innerHTML, jQuery.html, or through similar means. Aside from being inefficient, this is a major security risk. For more information, see https://developer.mozilla.org/en/XUL_School/DOM_Building_and_HTML_Insertion . Here are some examples that were discovered: * about.js line 1438, 1441: the builds are being downloaded from somewhere and the template in buildRow is being built from that downloaded data without escaping. Similar in other locations. Please consider not using innerHTML, or at least escape all strings inserted into the templates.
Comment 1•8 years ago
|
||
You are welcome to fix this :)
Assignee | ||
Comment 2•8 years ago
|
||
https://github.com/mozilla-b2g/b2g-installer/pull/43 ;)
Assignee | ||
Comment 3•8 years ago
|
||
Assignee: nobody → chimantaea_mirabilis
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8761518 -
Flags: review?(lissyx+mozillians)
Comment 4•8 years ago
|
||
Nice, can you show test results to make sure nothing regresses ?
Comment 5•8 years ago
|
||
Also, it would make it easier for reading/verifying to split the patch in two: the one fixing the innerHTML issue and the one fixing coding style :)
Flags: needinfo?(chimantaea_mirabilis)
Comment 6•8 years ago
|
||
Thanks for the PR !
Assignee | ||
Comment 7•8 years ago
|
||
It works well for my code to fix innerHTML issue, at the "Select" section of screenshot.
However, This tested code has a workaround to avoid OS check, because I'm working on Windows.
about.js:line 1584
> if (!isLinuxOk && !isDarwinOk) {
> console.debug("Only Linux and (Darwin x86-64) are supported");
> //return reject(new Error("GECKO_UNSUPPORTED_OS_ARCH"));
> }
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #5) > Also, it would make it easier for reading/verifying to split the patch in > two: the one fixing the innerHTML issue and the one fixing coding style :) Just reverted the first commit and separeted it. Thank you for reviewing.
Reporter | ||
Comment 9•8 years ago
|
||
Could we use template elements for this, instead of creating the DOM tree in JS?
Flags: needinfo?(lissyx+mozillians)
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Jovan Gerodetti from comment #9) > Could we use template elements for this, instead of creating the DOM tree in > JS? That's fine. I have naver used html template element, but this codes works. Add template to about.xhtml: > <section class="select"> > <h2>Select</h2> > ... > <template id="buildItemTemplate"> > <li><label class="build"> > <input type="radio" value="" name="build" /><!-- value: build.url --> > <div class="description"> > <h4></h4><!-- build.name --> > <span></span><!-- build.description --> > </div> > </label></li> > </template> > </section> Replace drawBuild(): > function drawBuild(build, isSupported) { > var tmpl = document.querySelector("#buildItemTemplate"); > if (isSupported && build.url) { > tmpl.content.querySelector("input").value = encodeURI(build.url); > tmpl.content.querySelector("input").style.display = "inherit"; > } else { > tmpl.content.querySelector("input").style.display = "none"; > } > tmpl.content.querySelector("h4").textContent = build.name; > tmpl.content.querySelector("span").textContent = build.description; > > return document.importNode(tmpl.content, true); > } Which is better?
Flags: needinfo?(chimantaea_mirabilis)
Assignee | ||
Comment 12•8 years ago
|
||
OK, commited to github as addtional change.
Flags: needinfo?(lissyx+mozillians)
Comment 14•8 years ago
|
||
Comment on attachment 8761518 [details] [review] b2g-installer PR #43 Keeping needinfo myself to verify the unit tests before merging
Flags: needinfo?(lissyx+mozillians)
Attachment #8761518 -
Flags: review?(lissyx+mozillians) → review+
Comment 15•8 years ago
|
||
Jovan, would you mind doing the checking ? The job is mostly to follow the steps at https://developer.mozilla.org/en-US/docs/Mozilla/B2G_OS/Building_and_installing_B2G_OS/B2G_installer_add-on#Hacking and make sure all the addon's mochitest are passing: > ./mach mochitest browser/extensions/b2g-installer/ This should be done on either linux or osx
Flags: needinfo?(lissyx+mozillians) → needinfo?(titannanomail)
Reporter | ||
Comment 16•8 years ago
|
||
The following tests failed: 1235 INFO TEST-UNEXPECTED-FAIL | browser/extensions/b2g-installer/test/mochitest/test_devices.html | two builds available - got 6, expected 2 SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:268:5 test_assert_builds/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices.html:66:7 test_assert_builds@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices.html:63:12 runTest/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices.html:127:19 promise callback*runTest@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices.html:122:5 setTimeout handler*delayAgain@chrome://specialpowers/content/specialpowersAPI.js:702:12 delayedCallback@chrome://specialpowers/content/specialpowersAPI.js:704:19 setTimeout handler*SpecialPowersAPI.prototype._setTimeout@chrome://specialpowers/content/specialpowersAPI.js:688:7 prefObs@chrome://specialpowers/content/specialpowersAPI.js:1126:7 SpecialPowersObserverAPI.prototype._receiveMessageAPI@chrome://specialpowers/content/SpecialPowersObserverAPI.js:287:22 SpecialPowersObserver.prototype._receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:107:10 SpecialPowersObserver.prototype.receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:302:14 SpecialPowers.prototype._sendSyncMessage@chrome://specialpowers/content/specialpowers.js:83:10 SpecialPowersAPI.prototype._setPref@chrome://specialpowers/content/specialpowersAPI.js:1304:12 SpecialPowersAPI.prototype._applyPrefs@chrome://specialpowers/content/specialpowersAPI.js:1137:9 SpecialPowersAPI.prototype.pushPrefEnv@chrome://specialpowers/content/specialpowersAPI.js:1079:7 setPrefsAndRunTests@http://mochi.test:8888/chrome/browser/extensions/b2g-installer/test/mochitest/common.js:92:3 @chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices.html:135:3 1236 INFO TEST-UNEXPECTED-FAIL | browser/extensions/b2g-installer/test/mochitest/test_devices_unkown.html | one element for no device - got 3, expected 1 SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:268:5 test_assert_device/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices_unkown.html:57:7 test_assert_device@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices_unkown.html:47:12 runTest/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices_unkown.html:79:19 promise callback*runTest@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices_unkown.html:76:5 setTimeout handler*delayAgain@chrome://specialpowers/content/specialpowersAPI.js:702:12 delayedCallback@chrome://specialpowers/content/specialpowersAPI.js:704:19 setTimeout handler*SpecialPowersAPI.prototype._setTimeout@chrome://specialpowers/content/specialpowersAPI.js:688:7 prefObs@chrome://specialpowers/content/specialpowersAPI.js:1126:7 SpecialPowersObserverAPI.prototype._receiveMessageAPI@chrome://specialpowers/content/SpecialPowersObserverAPI.js:287:22 SpecialPowersObserver.prototype._receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:107:10 SpecialPowersObserver.prototype.receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:302:14 SpecialPowers.prototype._sendSyncMessage@chrome://specialpowers/content/specialpowers.js:83:10 SpecialPowersAPI.prototype._setPref@chrome://specialpowers/content/specialpowersAPI.js:1304:12 SpecialPowersAPI.prototype._applyPrefs@chrome://specialpowers/content/specialpowersAPI.js:1137:9 SpecialPowersAPI.prototype.pushPrefEnv@chrome://specialpowers/content/specialpowersAPI.js:1079:7 setPrefsAndRunTests@http://mochi.test:8888/chrome/browser/extensions/b2g-installer/test/mochitest/common.js:92:3 @chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_devices_unkown.html:84:3 1237 INFO TEST-UNEXPECTED-FAIL | browser/extensions/b2g-installer/test/mochitest/test_select_build.html | two builds available - got 6, expected 2 SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:268:5 assert_select_build@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:43:5 test_select_userdebug/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:77:7 test_select_userdebug@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:75:12 test_plug_assert_unplug/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:125:19 promise callback*test_plug_assert_unplug@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:124:12 promise callback*runTest@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:130:5 setTimeout handler*delayAgain@chrome://specialpowers/content/specialpowersAPI.js:702:12 delayedCallback@chrome://specialpowers/content/specialpowersAPI.js:704:19 setTimeout handler*SpecialPowersAPI.prototype._setTimeout@chrome://specialpowers/content/specialpowersAPI.js:688:7 prefObs@chrome://specialpowers/content/specialpowersAPI.js:1126:7 SpecialPowersObserverAPI.prototype._receiveMessageAPI@chrome://specialpowers/content/SpecialPowersObserverAPI.js:287:22 SpecialPowersObserver.prototype._receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:107:10 SpecialPowersObserver.prototype.receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:302:14 SpecialPowers.prototype._sendSyncMessage@chrome://specialpowers/content/specialpowers.js:83:10 SpecialPowersAPI.prototype._setPref@chrome://specialpowers/content/specialpowersAPI.js:1304:12 SpecialPowersAPI.prototype._applyPrefs@chrome://specialpowers/content/specialpowersAPI.js:1137:9 SpecialPowersAPI.prototype.pushPrefEnv@chrome://specialpowers/content/specialpowersAPI.js:1079:7 setPrefsAndRunTests@http://mochi.test:8888/chrome/browser/extensions/b2g-installer/test/mochitest/common.js:92:3 @chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:138:3 1238 INFO TEST-UNEXPECTED-FAIL | browser/extensions/b2g-installer/test/mochitest/test_select_build.html | two builds available - got 6, expected 2 SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:268:5 assert_select_build@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:43:5 test_select_eng/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:87:7 test_select_eng@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:85:12 test_plug_assert_unplug/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:125:19 promise callback*test_plug_assert_unplug@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:124:12 promise callback*runTest@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:130:5 setTimeout handler*delayAgain@chrome://specialpowers/content/specialpowersAPI.js:702:12 delayedCallback@chrome://specialpowers/content/specialpowersAPI.js:704:19 setTimeout handler*SpecialPowersAPI.prototype._setTimeout@chrome://specialpowers/content/specialpowersAPI.js:688:7 prefObs@chrome://specialpowers/content/specialpowersAPI.js:1126:7 SpecialPowersObserverAPI.prototype._receiveMessageAPI@chrome://specialpowers/content/SpecialPowersObserverAPI.js:287:22 SpecialPowersObserver.prototype._receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:107:10 SpecialPowersObserver.prototype.receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:302:14 SpecialPowers.prototype._sendSyncMessage@chrome://specialpowers/content/specialpowers.js:83:10 SpecialPowersAPI.prototype._setPref@chrome://specialpowers/content/specialpowersAPI.js:1304:12 SpecialPowersAPI.prototype._applyPrefs@chrome://specialpowers/content/specialpowersAPI.js:1137:9 SpecialPowersAPI.prototype.pushPrefEnv@chrome://specialpowers/content/specialpowersAPI.js:1079:7 setPrefsAndRunTests@http://mochi.test:8888/chrome/browser/extensions/b2g-installer/test/mochitest/common.js:92:3 @chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:138:3 1239 INFO TEST-UNEXPECTED-FAIL | browser/extensions/b2g-installer/test/mochitest/test_select_build.html | two builds available - got 9, expected 3 SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:268:5 assert_select_build@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:43:5 test_select_custom/</</<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:108:11 setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:622:12 test_select_custom/</<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:107:9 test_select_custom/<@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:96:14 promise callback*test_select_custom@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:95:12 promise callback*runTest@chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:130:5 setTimeout handler*delayAgain@chrome://specialpowers/content/specialpowersAPI.js:702:12 delayedCallback@chrome://specialpowers/content/specialpowersAPI.js:704:19 setTimeout handler*SpecialPowersAPI.prototype._setTimeout@chrome://specialpowers/content/specialpowersAPI.js:688:7 prefObs@chrome://specialpowers/content/specialpowersAPI.js:1126:7 SpecialPowersObserverAPI.prototype._receiveMessageAPI@chrome://specialpowers/content/SpecialPowersObserverAPI.js:287:22 SpecialPowersObserver.prototype._receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:107:10 SpecialPowersObserver.prototype.receiveMessage@chrome://specialpowers/content/SpecialPowersObserver.jsm:302:14 SpecialPowers.prototype._sendSyncMessage@chrome://specialpowers/content/specialpowers.js:83:10 SpecialPowersAPI.prototype._setPref@chrome://specialpowers/content/specialpowersAPI.js:1304:12 SpecialPowersAPI.prototype._applyPrefs@chrome://specialpowers/content/specialpowersAPI.js:1137:9 SpecialPowersAPI.prototype.pushPrefEnv@chrome://specialpowers/content/specialpowersAPI.js:1079:7 setPrefsAndRunTests@http://mochi.test:8888/chrome/browser/extensions/b2g-installer/test/mochitest/common.js:92:3 @chrome://mochitests/content/chrome/browser/extensions/b2g-installer/test/mochitest/test_select_build.html:138:3
Flags: needinfo?(titannanomail) → needinfo?(lissyx+mozillians)
Comment 17•8 years ago
|
||
Looks like something is broken :)
Flags: needinfo?(lissyx+mozillians) → needinfo?(chimantaea_mirabilis)
Assignee | ||
Comment 18•8 years ago
|
||
I've confirmed. These fails are same reason. > 1235 INFO TEST-UNEXPECTED-FAIL | browser/extensions/b2g-installer/test/mochitest/test_devices.html | two builds available - got 6, expected 2 This means the childNodes gets child nodes and 2 text nodes of spaces. test_devices.html:66 > is(devices.childNodes.length, 2, "two builds available"); test_devices_unknown.html:53, 57 > is(devices.childNodes.length, 0, "no build available"); > is(noDevice.childNodes.length, 1, "one element for no device"); test_select_build.html:43 > is(devices.childNodes.length, nbExpected, "two builds available"); To fix this, it should be used ".childElementCount" instead of using "childNodes.length".
Flags: needinfo?(chimantaea_mirabilis)
Assignee | ||
Comment 19•8 years ago
|
||
Commited the fix to github. Could you please check again?
Flags: needinfo?(titannanomail)
Reporter | ||
Comment 20•8 years ago
|
||
> 0 INFO TEST-START | Shutdown
> 1 INFO Passed: 1151
> 2 INFO Failed: 0
> 3 INFO Todo: 0
> 4 INFO Mode: e10s
> 5 INFO SimpleTest FINISHED
Masahiko, can you squash your commits?
Flags: needinfo?(titannanomail) → needinfo?(chimantaea_mirabilis)
Assignee | ||
Comment 21•8 years ago
|
||
(In reply to Jovan Gerodetti from comment #20) > Masahiko, can you squash your commits? Rebase and squashed. It will be OK. Thank you very much :)
Flags: needinfo?(chimantaea_mirabilis)
Comment 23•8 years ago
|
||
https://github.com/mozilla-b2g/b2g-installer/commit/fa85ecf2285dc30dd5c1a3e86ca6cd88ceb03b6e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(lissyx+mozillians)
Resolution: --- → FIXED
Comment 24•8 years ago
|
||
Thanks both of you!
You need to log in
before you can comment on or make changes to this bug.
Description
•