Closed
Bug 1338328
Opened 7 years ago
Closed 7 years ago
Bug 860857 added some untested codepaths that trigger assertions and memory-corruption crashes when exercised
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bzbarsky, Assigned: nika)
References
Details
(Keywords: csectype-wildptr, sec-other, Whiteboard: [adv-main55-][post-critsmash-triage])
Attachments
(1 file, 2 obsolete files)
In particular, the "return NS_ERROR_TYPE_ERR" bit is bad. See bug 1328861 comment 12.
Flags: needinfo?(enndeakin)
Comment 1•7 years ago
|
||
There is a test for this at dom/tests/mochitest/general/test_clipboard_events.html But it would be sufficient to just silently fail instead if the error code is an issue.
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 2•7 years ago
|
||
> There is a test for this at dom/tests/mochitest/general/test_clipboard_events.html Yes, but it doesn't exercise this codepath. It's taking the "return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR" early return, because it's not a copy, cut, or dragstart event. The test doesn't check _why_ the call threw, unfortunately. > But it would be sufficient to just silently fail instead if the error code is an issue. Throwing an error is not a problem. Throwing NS_ERROR_TYPE_ERR is a problem: that's an error code with _very_ specific meaning in the DOM and shouldn't really be used by anyone other than DOM bindings machinery. If you actually reach this line in a debug build, it will nicely assert at you. NS_ERROR_DOM_INVALID_STATE_ERR or NS_ERROR_DOM_INVALID_MODIFICATION_ERR or NS_ERROR_DOM_NOT_SUPPORTED_ERR are probably reasonable options here....
Updated•7 years ago
|
Keywords: csectype-wildptr,
sec-critical
Comment 4•7 years ago
|
||
I was thinking I could help with a simple patch here but now I realize I was probably wrong :)
Updated•7 years ago
|
Blocks: 860857
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → unaffected
Comment 5•7 years ago
|
||
Tracking since this is sec-critical.
tracking-firefox51:
--- → +
tracking-firefox52:
--- → +
tracking-firefox53:
--- → +
tracking-firefox54:
--- → +
Assignee | ||
Comment 6•7 years ago
|
||
MozReview-Commit-ID: 9qwwXxWgWrO
Attachment #8841044 -
Flags: review?(amarchesini)
Assignee | ||
Updated•7 years ago
|
Attachment #8837315 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael
Flags: needinfo?(enndeakin)
Comment 7•7 years ago
|
||
Comment on attachment 8841044 [details] [diff] [review] Change error type for custom-clipdata mime type, and add test for correct exception type Review of attachment 8841044 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/general/test_clipboard_events.html @@ +54,5 @@ > test_input_paste_abort_dataTransfer, > test_input_copypaste_dataTransfer_multiple, > test_input_copy_button_dataTransfer, > + test_eventspref_disabled, > + test_input_cut_disallowed_types_dataTransfer add an extra ',' at the end of this line.
Attachment #8841044 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 8•7 years ago
|
||
MozReview-Commit-ID: 9qwwXxWgWrO
Assignee | ||
Updated•7 years ago
|
Attachment #8841044 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8841787 [details] [diff] [review] Change error type for custom-clipdata mime type, and add test for correct exception type [Security approval request comment] How easily could an exploit be constructed based on the patch? The security hole was already fixed in bug 1328861, so a successful attack on a patched version would not be possible. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, it is fairly difficult to see what the problem is which is being fixed without context. Which older supported branches are affected by this flaw? Since Fx48 If not all supported branches, which bug introduced the flaw? bug 860857 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This should easily apply on old branches, however I believe that bug 1328861 has been backported, which also closes this security hole. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions. Shouldn't need any special testing.
Attachment #8841787 -
Flags: sec-approval?
Comment 10•7 years ago
|
||
It sounds like this isn't actually a security bug per se any more, and we're just keeping it hidden because the other bug is hidden. As such, it doesn't need to be sec-critical, and it doesn't need sec-approval given that the other bug has been fixed.
Keywords: sec-critical → sec-other
Updated•7 years ago
|
Attachment #8841787 -
Flags: sec-approval?
Comment 11•7 years ago
|
||
So feel free to go ahead and land it whenever. Thanks for fixing this.
Comment 12•7 years ago
|
||
Marking wontfix for 51 since we're about to release 52. I don't think this will make it into 52 either. But we can keep tracking it for 52 in case it seems useful to take as a ridealong in a dot release.
tracking-firefox-esr52:
--- → ?
Assignee | ||
Comment 13•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc13d436dd486b917fb707f9648651efe5d35fb
Backed this out (and your other bug) for being a likely cause of bug https://treeherder.mozilla.org/logviewer.html#?job_id=80965408&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/413a21e421db
Flags: needinfo?(michael)
Assignee | ||
Comment 15•7 years ago
|
||
relanded it because it looks like it wasn't my fault (https://bugzilla.mozilla.org/show_bug.cgi?id=1337772) https://hg.mozilla.org/integration/mozilla-inbound/rev/49cf9bfdfa74181dd7b176652bb9ea87fcd31927
Flags: needinfo?(michael)
Comment 16•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/49cf9bfdfa74 Seems highly unlikely we'd include this in an Fx52 dot release since it's sec-other, but it looks like we'll need Aurora/Beta/ESR52 approval requests on it at least. Feel free to nominate it for release as well if you feel strongly that it should be considered as a ride-along.
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
status-firefox-esr52:
--- → affected
Flags: needinfo?(michael)
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8841787 [details] [diff] [review] Change error type for custom-clipdata mime type, and add test for correct exception type Approval Request Comment [Feature/Bug causing the regression]: bug 860857 [User impact if declined]: Incorrect error type produced by DataTransfer.setData (the security issue has been fixed in another bug) [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Makes small change to error type returned by function. [String changes made/needed]: None
Flags: needinfo?(michael)
Attachment #8841787 -
Flags: approval-mozilla-beta?
Attachment #8841787 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•7 years ago
|
||
I'm not convinced that we need this in esr52 as bug 1328861 has already landed there. What do you think bz?
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 19•7 years ago
|
||
I don't think we need this in ESR52, or indeed on any branches, really. The assertions and memory corruption were fixed in bug 1328861; at this point this is only hidden to prevent disclosing information about how to trigger that bug.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8841787 [details] [diff] [review] Change error type for custom-clipdata mime type, and add test for correct exception type Let's not uplift then :).
Attachment #8841787 -
Flags: approval-mozilla-beta?
Attachment #8841787 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
Updated•7 years ago
|
tracking-firefox-esr52:
? → ---
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Updated•7 years ago
|
Whiteboard: [adv-main55-]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main55-] → [adv-main55-][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•