Closed Bug 1338328 Opened 3 years ago Closed 3 years ago

Bug 860857 added some untested codepaths that trigger assertions and memory-corruption crashes when exercised

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox51 + wontfix
firefox52 + wontfix
firefox-esr52 --- wontfix
firefox53 + wontfix
firefox54 + wontfix
firefox55 --- fixed

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)
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)
> 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....
Can you take this, Neil?
Flags: needinfo?(enndeakin)
Attached patch bug1338328.patch (obsolete) — Splinter Review
I was thinking I could help with a simple patch here but now I realize I was probably wrong :)
Tracking since this is sec-critical.
MozReview-Commit-ID: 9qwwXxWgWrO
Attachment #8841044 - Flags: review?(amarchesini)
Attachment #8837315 - Attachment is obsolete: true
Assignee: nobody → michael
Flags: needinfo?(enndeakin)
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+
Attachment #8841044 - Attachment is obsolete: true
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?
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-criticalsec-other
Attachment #8841787 - Flags: sec-approval?
So feel free to go ahead and land it whenever. Thanks for fixing this.
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.
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: 3 years ago
Flags: needinfo?(michael)
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
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)
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)
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?
Group: dom-core-security → core-security-release
Whiteboard: [adv-main55-]
Flags: qe-verify-
Whiteboard: [adv-main55-] → [adv-main55-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.