Closed
Bug 1372509
(CVE-2017-7799)
Opened 8 years ago
Closed 8 years ago
Self-XSS XUL Injection in about:webrtc
Categories
(Core :: WebRTC, defect, P1)
Core
WebRTC
Tracking
()
VERIFIED
FIXED
mozilla56
People
(Reporter: freddy, Assigned: jib)
Details
(4 keywords, Whiteboard: [adv-main55+][post-critsmash-triage])
Attachments
(1 file)
1.97 KB,
patch
|
jesup
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(This would be a sec-critical, if there was a way for an attacker to inject markup to this page. It seems to me the information comes from the browser user through actual interaction and it's unlikely someone creates and uses a file called <textarea> :-))
JavaScript in about:webrtc should use textContent or escape HTML before assigning to .innerHTML
1)
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/toolkit/content/aboutwebrtc/aboutWebrtc.js#143
> this.msg.innerHTML =
> `<span class="info-label">${this._messageHeader}:</span> ${this._message}`;
In this case, I suggest replacing ><'& with their respective entities for both variables. This could be done with a tagged template string and an escape function like https://github.com/fxos-eng/sanitizer/blob/master/sanitizer.js#L46-L57
In
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/toolkit/content/aboutwebrtc/aboutWebrtc.js#308
> msg.innerHTML = `${data.error.name}: ${data.error.message}`;
I suggest using textContent instead of innerHTML
I am assigning this to you jib, because you have been modifying this file in the past. Please help me find someone else if you can not take this bug.
Assignee | ||
Updated•8 years ago
|
Rank: 14
Priority: -- → P1
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8885442 -
Flags: review?(rjesup)
Comment 2•8 years ago
|
||
Comment on attachment 8885442 [details] [diff] [review]
Fix eslint warnings in about:webrtc
Review of attachment 8885442 [details] [diff] [review]:
-----------------------------------------------------------------
Don't know a lot about eslint, but looks ok
Attachment #8885442 -
Flags: review?(rjesup) → review+
Reporter | ||
Comment 3•8 years ago
|
||
Looks great, thank you!
(NB, I haven't seen this 'Object.assign(document.createElement(), ..)' before. That's pretty neat!)
Flags: sec-review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 4•8 years ago
|
||
Thanks, yeah Object.assign() is a good feature. If someone could push this for me that'd be great.
Comment 5•8 years ago
|
||
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 7•8 years ago
|
||
Seems worth a backport request for Beta at least. Not sure if it's worth the churn for ESR52 since the attack seems mostly hypothetical?
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox-esr52:
--- → affected
Flags: needinfo?(jib)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8885442 [details] [diff] [review]
Fix eslint warnings in about:webrtc
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1100508
[User impact if declined]: sec-moderate - see comment 0
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:
Go to about:webrtc, click all three buttons (Save page requires file dialog confirm) and verify each adds one status line of text below.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]:
Trivial change. Small risk area (extremely small sub-pool of users visit about:webrtc)
[String changes made/needed]: None
Attachment #8885442 -
Flags: approval-mozilla-beta?
Comment 10•8 years ago
|
||
Comment on attachment 8885442 [details] [diff] [review]
Fix eslint warnings in about:webrtc
input sanitization in about:webrtc, beta55+
Attachment #8885442 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Comment 11•8 years ago
|
||
uplift |
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify+
Updated•7 years ago
|
Alias: CVE-2017-7799
Whiteboard: [adv-main55+]
Comment 12•7 years ago
|
||
I managed to test the issue on Firefox 55.0b12 and on Firefox 56.0a1 (2017-07-26), under Windows 10x64, Ubuntu 16.04x64 and under Mac OS X 10.11 and the status lines are correctly displayed.
I've also tried to reproduce the issue on Firefox 52.2.1, Firefox 54.0.1 and on Firefox 56.0a1 (2017-06-12), under Windows 10x64, without success. The status lines (Save page, Debug Mode and AEC Logging) are also displayed on affected builds.
Is this behavior expected?
Flags: needinfo?(rjesup) needinfo?(jib)
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
Comment 13•7 years ago
|
||
I managed to test the issue on Firefox 55.0b12 and on Firefox 56.0a1 (2017-07-26), under Windows 10x64, Ubuntu 16.04x64 and under Mac OS X 10.11 and the status lines are correctly displayed.
I've also tried to reproduce the issue on Firefox 52.2.1, Firefox 54.0.1 and on Firefox 56.0a1 (2017-06-12), under Windows 10x64, without success. The status lines (Save page, Debug Mode and AEC Logging) are also displayed on affected builds.
Is this behavior expected?
Comment 14•7 years ago
|
||
Sorry for the double Commend and ni.
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
Assignee | ||
Comment 15•7 years ago
|
||
Thanks!
Yes there should be no visible difference in how the status lines are displayed. It's just using safer web-tech to get it there.
Flags: needinfo?(jib)
Updated•7 years ago
|
Whiteboard: [adv-main55+] → [adv-main55+][post-critsmash-triage]
Comment 16•7 years ago
|
||
According to Comment 13 and Comment 15, I'm marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•7 years ago
|
Flags: needinfo?(rjesup)
Updated•7 years ago
|
Group: core-security-release
Updated•3 years ago
|
Keywords: csectype-sandbox-escape
You need to log in
before you can comment on or make changes to this bug.
Description
•