1190036 - clipboardData.getFilesAndDirectories() crash

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: value, at BindingUtils.h:937

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8650989 [details] [diff] [review]
crash.patch

We need this on aurora too, right?


>     if (!mFiles) {
>+      aRv.Throw(NS_ERROR_FAILURE);
>       return nullptr;
>     }

This is wrong, I mean the returning null (and now also throwing the exception). Please file a followup bug to fix.
or fix in this bug.
It is ok mFiles to be null and the spec says 
"if no files or directories were chosen, returns a Promise resolved with an empty sequence."

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8650989 [details] [diff] [review]
crash.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1164310
[User impact if declined]: a crash.
[Describe test coverage new/current, TreeHerder]: No test.
[Risks and why]: The patch just adds an exception. No risks at all.
[String/UUID change made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Too easy. 1 line. Test included.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes. The test shows the error clearly.

Which older supported branches are affected by this flaw?

aurora.

If not all supported branches, which bug introduced the flaw?

bug 1164310
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 5

[Daniel Veditz [:dveditz]]

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> 
> Too easy. 1 line. Test included.

The crashes I see seem to be a null dereference. How would this be exploited?

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> 
> Yes. The test shows the error clearly.

Are you checking in the test? I don't see it in the patch.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: crash.patch

crashtest included, and sorry for the previous comment: I meant that it's easy to reproduce the crash, not to exploit it.

Comment 7

[Daniel Veditz [:dveditz]]

If this bug is guaranteed to be a null deref (as it appeared in my -very-minimal- testing) then we don't need to hide this bug or request sec-approval. If it _is_ potentially exploitable we wouldn't want to include the test until after we released the fix.

I assume those asserts are just regular debug-only asserts and don't actually protect us at runtime, but if we've forced asserts() on in release let me know if that's an important part of the patch.

Comment 8

[Andrea Marchesini [:baku]]

Yes, you are right. It's a null deref.

Comment 9

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f90e2d3e63660fa660b641e7c8150071d6984797
Bug 1190036 - clipboardData.getFilesAndDirectories() should throw an exception when returning null, r=smaug

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f90e2d3e6366

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651427 [details] [diff] [review]
crash.patch

The patch looks safe and has an automated test. It has been in Nightly for a few days. Let's uplift to Aurora42 as it's also a crash fix.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c47645f91f51