1275065 - Unsafe innerHTML usage in b2gInstaller

Status: RESOLVED FIXED

(Firefox OS Graveyard :: B2gInstaller, defect)

Description

[Jovan Gerodetti]

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

[:gerard-majax]

You are welcome to fix this :)

Comment 2

[Masahiko Imanaka [:marsf]]

https://github.com/mozilla-b2g/b2g-installer/pull/43

;)

Comment 3

[Masahiko Imanaka [:marsf]]

Attachment: b2g-installer PR #43

Comment 4

[:gerard-majax]

Nice, can you show test results to make sure nothing regresses ?

Comment 5

[:gerard-majax]

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 :)

Comment 6

[:gerard-majax]

Thanks for the PR !

Comment 7

[Masahiko Imanaka [:marsf]]

Attachment: Fixed innerHTML issue (Firefox nightly + Flame-KK)

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"));
> }

Comment 8

[Masahiko Imanaka [:marsf]]

(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.

Comment 9

[Jovan Gerodetti]

Could we use template elements for this, instead of creating the DOM tree in JS?

Comment 10

[Masahiko Imanaka [:marsf]]

(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?

Comment 11

[:gerard-majax]

I like the idea of using templates :)

Comment 12

[Masahiko Imanaka [:marsf]]

OK, commited to github as addtional change.

Comment 13

[:gerard-majax]

Looks good to me.

Comment 14

[:gerard-majax]

Comment on attachment 8761518 [details] [review]
b2g-installer PR #43

Keeping needinfo myself to verify the unit tests before merging

Comment 15

[:gerard-majax]

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

Comment 16

[Jovan Gerodetti]

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

Comment 17

[:gerard-majax]

Looks like something is broken :)

Comment 18

[Masahiko Imanaka [:marsf]]

Attachment: mochitest fails to get childNodes

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".

Comment 19

[Masahiko Imanaka [:marsf]]

Commited the fix to github.
Could you please check again?

Comment 20

[Jovan Gerodetti]

> 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?

Comment 21

[Masahiko Imanaka [:marsf]]

(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 :)

Comment 22

[Jovan Gerodetti]

Alexandre, can you merge?

Comment 23

[:gerard-majax]

https://github.com/mozilla-b2g/b2g-installer/commit/fa85ecf2285dc30dd5c1a3e86ca6cd88ceb03b6e

Comment 24

[:gerard-majax]

Thanks both of you!