In particular, the "return NS_ERROR_TYPE_ERR" bit is bad. See bug 1328861 comment 12.
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.
> 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?
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.
Attachment #8841044 - Flags: review?(amarchesini)
Attachment #8837315 - Attachment is obsolete: true
Assignee: nobody → michael
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.
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.
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.
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
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
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.
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
I'm not convinced that we need this in esr52 as bug 1328861 has already landed there. What do you think bz?
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.
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 :).
Whiteboard: [adv-main55-] → [adv-main55-][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.