q1 Reporter
	Description • 7 years ago

DataTransfer::FillInExternalCustomTypes() (dom\events\DataTransfer.cpp) doesn't check status when reading from an input stream. If the stream is defective, such that insufficient or no data is read from it, FillInExternalCustomTypes() uses the resulting partially- or un-initialized stack variables to create various objects, which are then used by other code.

The stream can be derived from untrusted data.

In several tests, this bug causes memory corruption, resulting in access-violation crashes from attempts to read and/or write memory to which the executing code has no valid access.



The bugs are in all the calls to |stream -> Read*()| and also on the call to SetInputStream()}:

1491: void
1492: DataTransfer::FillInExternalCustomTypes(nsIVariant* aData, uint32_t aIndex,
1493:                                         nsIPrincipal* aPrincipal)
1494: {
1495:   char* chrs;
1496:   uint32_t len = 0;
1497:   nsresult rv = aData->GetAsStringWithSize(&len, &chrs);
1498:   if (NS_FAILED(rv)) {
1499:     return;
1500:   }
1501: 
1502:   nsAutoCString str;
1503:   str.Adopt(chrs, len);
1504: 
1505:   nsCOMPtr<nsIInputStream> stringStream;
1506:   NS_NewCStringInputStream(getter_AddRefs(stringStream), str);
1507: 
1508:   nsCOMPtr<nsIBinaryInputStream> stream =
1509:     do_CreateInstance("@mozilla.org/binaryinputstream;1");
1510:   if (!stream) {
1511:     return;
1512:   }
1513: 
1514:   stream->SetInputStream(stringStream);
1515: 
1516:   uint32_t type;
1517:   do {
1518:     stream->Read32(&type);
1519:     if (type == eCustomClipboardTypeId_String) {
1520:       uint32_t formatLength;
1521:       stream->Read32(&formatLength);
1522:       char* formatBytes;
1523:       stream->ReadBytes(formatLength, &formatBytes);
1524:       nsAutoString format;
1525:       format.Adopt(reinterpret_cast<char16_t*>(formatBytes),
1526:                    formatLength / sizeof(char16_t));
1527: 
1528:       uint32_t dataLength;
1529:       stream->Read32(&dataLength);
1530:       char* dataBytes;
1531:       stream->ReadBytes(dataLength, &dataBytes);
1532:       nsAutoString data;
1533:       data.Adopt(reinterpret_cast<char16_t*>(dataBytes),
1534:                  dataLength / sizeof(char16_t));
1535: 
1536:       RefPtr<nsVariantCC> variant = new nsVariantCC();
1537:       variant->SetAsAString(data);
1538: 
1539:       SetDataWithPrincipal(format, variant, aIndex, aPrincipal);
1540:     }
1541:   } while (type != eCustomClipboardTypeId_None);
1542: }

Attached is a POC, derived from the POC for https://bugzilla.mozilla.org/show_bug.cgi?id=1347748 , that creates a defective (in this case, truncated) input stream.

The POC uses lots of memory, so try it on a system with >= 16 GB.

Use it by opening the POC in two instances of FF and attaching the debugger to FF 1. Set a BP on DataTransfer::FillInExternalCustomTypes(), drag the text "Drag this text..." from FF 2 onto the text "...and drop it here!" in FF 1. Wait a minute or three for the BP to fire. On the first iteration of the loop, the code reads |formatLength| == 0x1a, and the correct |formatBytes| "hacker/hacker". It then reads the bogus |dataLength| 0x2000, but |ReadBytes()| on line 1531 gets only 0x62 bytes from the stream, then returns failure. Line 1533 "adopts" the resulting string (which might or might not be OK, depending upon what trash was in the uninitialized pointer |dataBytes|.

On the second iteration, the stream is at EOF, so none of the Read() functions return any data. |formatLength| is, thus, unchanged (at 0x1a), but |formatBytes| points to freed memory. Lines 1525-26 "adopt" this garbage into |format|. A similar thing then happens with |data|.

You can now remove the BP and proceed, and (if your memory contents are appropriate) the POC might display an alert box with a string beginning "xxxxxxx" followed by trash read from beyond bounds. Or FF might crash if your memory contents are otherwise. To get more interesting crashes, launch a WebGL video in a different tab in FF 1 before dropping the text.

An attacker could create a more-defective input stream and easily convince a user to drop the object from which it is derived onto FF. She could then use a JS drop handler (deHandler() in the POC) to read the data, and then AJAX it to a remote server.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 1 • 7 years ago

I haven't been super successful at reproducing this on linux, but I think this should fix the issue. Both with and without this patch firefox hangs and then crashes after some period of time, on e10s this is because of the maximum IPC message size being exceeded, and without e10s it seems to be an X error complaining about a string being too large.

I imagine that this bug reproduces better on other platforms?

MozReview-Commit-ID: BP4avMImDB8

	q1 Reporter
	Comment 2 • 7 years ago

(In reply to Michael Layzell [:mystor] from comment #1)

> ...I haven't been super successful at reproducing this on linux, but I think
> this should fix the issue. Both with and without this patch firefox hangs
> and then crashes after some period of time, on e10s this is because of the
> maximum IPC message size being exceeded, and without e10s it seems to be an
> X error complaining about a string being too large.
> 
> I imagine that this bug reproduces better on other platforms?

It's completely reproducible on Win7 x64 using FF 51.0.1 x64 debug build.

	q1 Reporter
	Comment 3 • 7 years ago

It also crashes FF 50.0.1 x64 release on Win7 x64; I haven't tested later versions of FF release builds.

BTW, you must use an x64 build and platform, because the POC generates strings too large for 32 bit builds and platforms.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 4 • 7 years ago

Comment on attachment 8848576 [details] [diff] [review]
Check nsresult values more places in DataTransfer

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I imagine that someone looking at this patch closely wouldn't have too much trouble coming up with an attack similar to the POC attack in comment 1.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I don't think so, I wrote the commit message to sound like a routine cleanup where we do more checks.

Which older supported branches are affected by this flaw? I think all of them are (maybe not ESR45 if we're still supporting it)

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch probably applies cleanly on all supported branches. If it doesn't creating a patch which does should not be hard.

How likely is this patch to cause regressions; how much testing does it need? This patch should not cause any regressions.

	Al Billings [:abillings - ex-MoCo]
	Comment 5 • 7 years ago

Giving sec-approval+ for trunk.
We'll want Aurora, Beta, and ESR52 patches made and nominated.

We really do need to know if ESR45 is affected as we are still supporting it.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 6 • 7 years ago

(In reply to Al Billings [:abillings] from comment #5)
> Giving sec-approval+ for trunk.
> We'll want Aurora, Beta, and ESR52 patches made and nominated.
> 
> We really do need to know if ESR45 is affected as we are still supporting it.

ESR45 is not affected. I'll try to get this rebased onto aurora beta and 52.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 7 • 7 years ago

Comment on attachment 8848576 [details] [diff] [review]
Check nsresult values more places in DataTransfer

I was able to successfully apply this patch on all affected branches.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: potential security vulnerability
Fix Landed on Version: approved to land in central
Risk to taking this patch (and alternatives if risky): Fairly low risk - only adds checks.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: potential security vulnerability
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Approved to land, has not landed yet.
[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?]: Low risk.
[Why is the change risky/not risky?]:  Only adds checks to ensure no buffer overruns
[String changes made/needed]: None

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 8 • 7 years ago

Comment on attachment 8848576 [details] [diff] [review]
Check nsresult values more places in DataTransfer

This still needs to land on m-c, but let's try to get it onto aurora and beta for Monday's beta 7 build too.

	Carsten Book [:Tomcat]
	Comment 9 • 7 years ago
uplift

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> Comment on attachment 8848576 [details] [diff] [review]
> Check nsresult values more places in DataTransfer
> 
> This still needs to land on m-c, but let's try to get it onto aurora and
> beta for Monday's beta 7 build too.

went ahead and checked this into aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/0583275688f1f5e8e6d6c6842514c7ec183f5381

Michael, do you want to land this on m-c (or more m-i) or shall this be done in the checkin-needed cyle ?

	Carsten Book [:Tomcat]
	Comment 10 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/c21f6a37628a

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 11 • 7 years ago

(In reply to Carsten Book [:Tomcat] from comment #9)
> Michael, do you want to land this on m-c (or more m-i) or shall this be done
> in the checkin-needed cyle ?

Sorry, I've been sick over the last few days & not working. This patch is on my desktop in the office, so it will probably land faster checkin-needed style.

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcef05f7661

	Carsten Book [:Tomcat]
	Comment 13 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/4bcef05f7661

	Julien Cristau [:jcristau]
	Comment 14 • 7 years ago

Comment on attachment 8848576 [details] [diff] [review]
Check nsresult values more places in DataTransfer

add missing error checking, sec-high, esr52+

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/a4e1e04c88ee

	Andrei Vaida [:avaida]
	Comment 16 • 7 years ago

Setting qe-verify- based on Michael's assessment on manual testing needs (see Comment 7).