Unsafe innerHTML usage in b2gInstaller

RESOLVED FIXED

Status

Firefox OS
B2gInstaller
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jovan Gerodetti, Assigned: marsf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
You are welcome to fix this :)
Blocks: 1265385
Blocks: 1166276
(Assignee)

Comment 2

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

;)
(Assignee)

Comment 3

2 years ago
Created attachment 8761518 [details] [review]
b2g-installer PR #43
Assignee: nobody → chimantaea_mirabilis
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8761518 - Flags: review?(lissyx+mozillians)
Nice, can you show test results to make sure nothing regresses ?
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)
Thanks for the PR !
(Assignee)

Comment 7

2 years ago
Created attachment 8761887 [details]
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"));
> }
(Assignee)

Comment 8

2 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

2 years ago
Could we use template elements for this, instead of creating the DOM tree in JS?
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 10

2 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)
I like the idea of using templates :)
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 12

2 years ago
OK, commited to github as addtional change.
Flags: needinfo?(lissyx+mozillians)
Looks good to me.
Flags: needinfo?(lissyx+mozillians)
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+
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

a year 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)
Looks like something is broken :)
Flags: needinfo?(lissyx+mozillians) → needinfo?(chimantaea_mirabilis)
(Assignee)

Comment 18

a year ago
Created attachment 8762817 [details]
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".
Flags: needinfo?(chimantaea_mirabilis)
(Assignee)

Comment 19

a year ago
Commited the fix to github.
Could you please check again?
Flags: needinfo?(titannanomail)
(Reporter)

Comment 20

a year 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

a year 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)
(Reporter)

Comment 22

a year ago
Alexandre, can you merge?
Flags: needinfo?(lissyx+mozillians)
https://github.com/mozilla-b2g/b2g-installer/commit/fa85ecf2285dc30dd5c1a3e86ca6cd88ceb03b6e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(lissyx+mozillians)
Resolution: --- → FIXED
Thanks both of you!
You need to log in before you can comment on or make changes to this bug.