q1 Reporter
	Description • 7 years ago

DataTransfer::GetTransferable() (dom\events\DataTransfer.cpp) can cause an integer overflow. Because of the incidental fact that this function only ever writes an even number of bytes to its temporary output stream |stream|, the overflow occurs, but it doesn't cause a write beyond bounds, only a truncation of the transferable data. If this function were modified to write an odd number of bytes to |stream|, this bug would become exploitable.

The bug is in the unguarded addition in lines 1071-72 (see below).

Attached is a POC that illustrates the overflow. Use it by attaching a debugger to FF and setting a BP on line 1071. Drag the text and wait for the BP to trigger. On the 8th BP, observe how |totalCustomLength| overflows.

If GetTransferable() ever wrote an odd number of bytes to |stream|, the overflow would become exploitable by allowing a script to use a series of DataTransfer.setData() calls that cause |totalCustomLength| to total exactly 0xfffffffb when control reaches line 1081. Line 1081 would then increment |totalCustomLength| to 0xffffffff, and line 1088 would then overflow and allocate 0 bytes of nsStringBuffer. But lines 1093-94 would read 0xffffffff bytes of content-provided data into the 0-length nsStringBuffer. (You can simulate this overrun using the debugger. Make sure to skip the debug-only assertions in nsStringBuffer::Alloc()).


 951: already_AddRefed<nsITransferable>
 952: DataTransfer::GetTransferable(uint32_t aIndex, nsILoadContext* aLoadContext)
 953: {
 954:   if (aIndex >= MozItemCount()) {
 955:     return nullptr;
 956:   }
 957: 
 958:   const nsTArray<RefPtr<DataTransferItem>>& item = *mItems->MozItemsAt(aIndex);
 959:   uint32_t count = item.Length();
 960:   if (!count) {
 961:     return nullptr;
 962:   }
 ...
1008:   do {
1009:     for (uint32_t f = 0; f < count; f++) {
1010:       RefPtr<DataTransferItem> formatitem = item[f];
1011:       nsCOMPtr<nsIVariant> variant = formatitem->DataNoSecurityCheck();
1012:       if (!variant) { // skip empty items
1013:         continue;
1014:       }
1015: 
1016:       nsAutoString type;
1017:       formatitem->GetType(type);
1018: 
1019:       // If the data is of one of the well-known formats, use it directly.
1020:       bool isCustomFormat = true;
1021:       for (uint32_t f = 0; f < ArrayLength(knownFormats); f++) {
1022:         if (type.EqualsASCII(knownFormats[f])) {
1023:           isCustomFormat = false;
1024:           break;
1025:         }
1026:       }
1027: 
1028:       uint32_t lengthInBytes;
1029:       nsCOMPtr<nsISupports> convertedData;
1030: 
1031:       if (handlingCustomFormats) {
1032:         if (!ConvertFromVariant(variant, getter_AddRefs(convertedData),
1033:                                 &lengthInBytes)) {
1034:           continue;
1035:         }
1036: 
1037:         // When handling custom types, add the data to the stream if this is a
1038:         // custom type.
1039:         if (isCustomFormat) {
1040:           // If it isn't a string, just ignore it. The dataTransfer is cached in
1041:           // the drag sesion during drag-and-drop, so non-strings will be
1042:           // available when dragging locally.
1043:           nsCOMPtr<nsISupportsString> str(do_QueryInterface(convertedData));
1044:           if (str) {
1045:             nsAutoString data;
1046:             str->GetData(data);
1047: 
1048:             if (!stream) {
1049:               // Create a storage stream to write to.
1050:               NS_NewStorageStream(1024, UINT32_MAX, getter_AddRefs(storageStream));
1051: 
1052:               nsCOMPtr<nsIOutputStream> outputStream;
1053:               storageStream->GetOutputStream(0, getter_AddRefs(outputStream));
1054: 
1055:               stream = do_CreateInstance("@mozilla.org/binaryoutputstream;1");
1056:               stream->SetOutputStream(outputStream);
1057:             }
1058: 
1059:             int32_t formatLength = type.Length() * sizeof(nsString::char_type);
1060: 
1061:             stream->Write32(eCustomClipboardTypeId_String);
1062:             stream->Write32(formatLength);
1063:             stream->WriteBytes((const char *)type.get(),
1064:                                formatLength);
1065:             stream->Write32(lengthInBytes);
1066:             stream->WriteBytes((const char *)data.get(), lengthInBytes);
1067: 
1068:             // The total size of the stream is the format length, the data
1069:             // length, two integers to hold the lengths and one integer for the
1070:             // string flag.
1071:             totalCustomLength +=
1072:               formatLength + lengthInBytes + (sizeof(uint32_t) * 3);
1073:           }
1074:         }
1075:       } else if (isCustomFormat && stream) {
1076:         // This is the second pass of the loop (handlingCustomFormats is false).
1077:         // When encountering the first custom format, append all of the stream
1078:         // at this position.
1079: 
1080:         // Write out a terminator.
1081:         totalCustomLength += sizeof(uint32_t);
1082:         stream->Write32(eCustomClipboardTypeId_None);
1083: 
1084:         nsCOMPtr<nsIInputStream> inputStream;
1085:         storageStream->NewInputStream(0, getter_AddRefs(inputStream));
1086: 
1087:         RefPtr<nsStringBuffer> stringBuffer =
1088:           nsStringBuffer::Alloc(totalCustomLength + 1);
1089: 
1090:         // Read the data from the string and add a null-terminator as ToString
1091:         // needs it.
1092:         uint32_t amountRead;
1093:         inputStream->Read(static_cast<char*>(stringBuffer->Data()),
1094:                           totalCustomLength, &amountRead);
1095:         static_cast<char*>(stringBuffer->Data())[amountRead] = 0;
...

	Daniel Veditz [:dveditz]
	Comment 1 • 7 years ago

Neil: could you take a look at this? Sounds like we only got lucky that this isn't exploitable, we should squash the bug before something changes it into something worse.

	q1 Reporter
	Comment 2 • 7 years ago

BTW, the POC uses lots of memory. Try it on a system with >= 16 GB.

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

Comment on attachment 8849202 [details] [diff] [review]
Add some overflow and write error checks

Review of attachment 8849202 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DataTransfer.cpp
@@ +946,5 @@
>  
>    bool added = false;
>    bool handlingCustomFormats = true;
> +  // The length includes an ending terminator plus final null.
> +  uint32_t totalCustomLength = sizeof(uint32_t) + 1;

I found this comment fairly confusing and it took me a little bit to figure out what it was trying to say.

Perhaps something along the lines of the following would be better:

// When writing the custom data, we need to ensure that there is sufficient space for a (uint32_t) ending type, and the null byte character at the end of the nsCString. We claim that space upfront.

I'm not sure if this is the best comment, but I feel like it would be more clear about what ending terminator you're talking about and why we care about the final null.

@@ +1035,5 @@
> +            // Guard against large data by ignoring any that don't fit.
> +            uint32_t newSize = formatLength + lengthInBytes + (sizeof(uint32_t) * 3);
> +            if ((uint64_t)totalCustomLength + newSize < UINT32_MAX) {
> +              if (NS_FAILED(stream->Write32(eCustomClipboardTypeId_String))) {
> +                continue;

I think we should probably at least NS_WARN_IF on these.

This looks to me like it could cause an integer overflow. If we fail to perform one of these writes then totalCustomLength is never increased, and yet the stream head has advanced by some percentage of that amount.

Perhaps we want to mark the custom data writing as "failed" when we get a failure here, and just discard the entire stream? We could probably just null out the stream object and then set a flag which prevents it from being recreated.

@@ +1063,5 @@
>          // This is the second pass of the loop (handlingCustomFormats is false).
>          // When encountering the first custom format, append all of the stream
>          // at this position.
>  
> +        nsresult rv = stream->Write32(eCustomClipboardTypeId_None);

Please re-add the // Write out a terminator. comment

	q1 Reporter
	Comment 5 • 7 years ago

(In reply to Neil Deakin from comment #3)
> Created attachment 8849202 [details] [diff] [review]
> Add some overflow and write error checks

The patch would be more resilient if it used CheckedInt. In particular, it not clear to me that

   uint32_t newSize = formatLength + lengthInBytes + (sizeof(uint32_t) * 3);

can't overflow; I don't know the maximum possible size of object that ConvertFromVariant() can return, and anyway someone might increase it without realizing that doing so creates a security bug.

Also, as written, the patch truncates the stream on error; this might cause problems elsewhere, like https://bugzilla.mozilla.org/show_bug.cgi?id=1348143 . It would be best, I think, to discard an incomplete stream entirely.

Finally, is |continue;| really the right thing to do if there's an error in the |if (handlingCustomFormats)| clause? That leaves |handlingCustomFormats == true|, which seems incorrect. (The original code does this, too, if there's an error in ConvertFromVariant()).

	q1 Reporter
	Comment 7 • 7 years ago

(In reply to Neil Deakin from comment #6)
> Created attachment 8849854 [details] [diff] [review]
> Add some overflow and write error checks, v2

------
+            CheckedInt<uint32_t> formatLength = type.Length() * sizeof(nsString::char_type);

should be

+            CheckedInt<uint32_t> formatLength = CheckedInt<uint32_t> (type.Length()) * sizeof(nsString::char_type);
------

------
+            uint32_t amountRead;
+            inputStream->Read(static_cast<char*>(stringBuffer->Data()),
+                              totalCustomLength, &amountRead);

If the |Read()| fails, |amountRead| will be uninitialized...which then probably causes

+            static_cast<char*>(stringBuffer->Data())[amountRead] = 0;

to overwrite something that it doesn't own (I notice that this bug is in the original code...yuck)
------

------
It seems that this block:

+            if (newSize.isValid()) {
+...
+              totalCustomLength = newSize.value();
+            }

must be followed with

             else {
               totalCustomLength = 0;
             }

for the block that follows it to work correctly on overflow.
------

Is there a way to get the entire patch file, instead of just the section that the version-control system thinks I should examine?

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

Comment on attachment 8849854 [details] [diff] [review]
Add some overflow and write error checks, v2

Review of attachment 8849854 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DataTransfer.cpp
@@ +949,5 @@
> +
> +  // When writing the custom data, we need to ensure that there is sufficient
> +  // space for a (uint32_t) data ending type, and the null byte character at
> +  // the end of the nsCString. We claim that space upfront
> +  uint32_t totalCustomLength = sizeof(uint32_t) + 1;

Perhaps mention that `0` is reserved as a value to be used when a read fails?

@@ +1033,5 @@
>                stream = do_CreateInstance("@mozilla.org/binaryoutputstream;1");
>                stream->SetOutputStream(outputStream);
>              }
>  
> +            CheckedInt<uint32_t> formatLength = type.Length() * sizeof(nsString::char_type);

I think this would be rejected by the static analysis. Nevertheless, this doesn't actually check the multiplication - it just multiplies, then wraps the type into a CheckedInt<uint32_t>.

@@ +1040,5 @@
> +            // length, two integers to hold the lengths and one integer for
> +            // the string flag. Guard against large data by ignoring any that
> +            // don't fit.
> +            CheckedInt<uint32_t> newSize = formatLength + totalCustomLength +
> +                                           lengthInBytes + (sizeof(uint32_t) * 3);

Here too - wrap in a CheckedInt before adding.

@@ +1042,5 @@
> +            // don't fit.
> +            CheckedInt<uint32_t> newSize = formatLength + totalCustomLength +
> +                                           lengthInBytes + (sizeof(uint32_t) * 3);
> +
> +            if (newSize.isValid()) {

I assume the idea here is that if newSize overflows, we discard this particular custom type but continue running?

@@ +1045,5 @@
> +
> +            if (newSize.isValid()) {
> +              // If a write error occurs, set totalCustomLength to 0 so that
> +              // further processing gets ignored.
> +              if (NS_WARN_IF(NS_FAILED(stream->Write32(eCustomClipboardTypeId_String)))) {

style nit: It's nice to have the command on a separate line from the error checking. It feels a bit like these commands are being drowned out by the NS_WARN_IF(NS_FAILED()).

@@ +1093,2 @@
>  
> +            // Read the data from the string and add a null-terminator as ToString

Aren't we reading from the "stream" not "string"?

@@ +1093,5 @@
>  
> +            // Read the data from the string and add a null-terminator as ToString
> +            // needs it.
> +            uint32_t amountRead;
> +            inputStream->Read(static_cast<char*>(stringBuffer->Data()),

Can you check the nsresult from this?

	q1 Reporter
	Comment 9 • 7 years ago

(In reply to Michael Layzell [:mystor] from comment #8)
...
> > +            CheckedInt<uint32_t> formatLength = type.Length() * sizeof(nsString::char_type);
> 
> I think this would be rejected by the static analysis. Nevertheless, this
> doesn't actually check the multiplication - it just multiplies, then wraps
> the type into a CheckedInt<uint32_t>.
> 
> @@ +1040,5 @@
> > +            // length, two integers to hold the lengths and one integer for
> > +            // the string flag. Guard against large data by ignoring any that
> > +            // don't fit.
> > +            CheckedInt<uint32_t> newSize = formatLength + totalCustomLength +
> > +                                           lengthInBytes + (sizeof(uint32_t) * 3);
> 
> Here too - wrap in a CheckedInt before adding.

That should be OK, because |formatLength| isa |CheckedInt|, so |totalCustomLength| should get converted to a |CheckedInt| through one of the |operator +()| functions in CheckedInt. That should cause the same conversion throughout the expression.

	Neil Deakin Assignee
	Comment 11 • 7 years ago

> I assume the idea here is that if newSize overflows, we discard this
> particular custom type but continue running?
> 

Yes, so that only large items are discarded.

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

Comment on attachment 8851546 [details] [diff] [review]
Add some overflow and write error checks, v3

Review of attachment 8851546 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DataTransfer.cpp
@@ +1082,5 @@
>          // This is the second pass of the loop (handlingCustomFormats is false).
>          // When encountering the first custom format, append all of the stream
> +        // at this position. If totalCustomLength is 0 indicating a write error
> +        // occurred, or no data has been added to it, don't output anything,
> +        if (totalCustomLength > sizeof(uint32_t) + 1) {

I don't like this magic number here. Can you pull it into a constant?

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/7d726c72ff03

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

Looks like this goes back to Fx48 if my code archaeology is correct. Please request Aurora/Beta/ESR52 approval on this when you get a chance.

	Neil Deakin Assignee
	Comment 15 • 7 years ago

Comment on attachment 8851546 [details] [diff] [review]
Add some overflow and write error checks, v3

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: theoretical buffer overwrite error that can be caused by web page script adding data to drag and drop.
[Is this code covered by automated tests?]: no, drag and drop cannot be tested automatically
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: testcase is provided, but should test on 64-bit machine. I manually tested extensively
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: would only affect sites that use custom formats for drag and drop if it did, so very minimal risk.
[Why is the change risky/not risky?]:
[String changes made/needed]: no

	Marcia Knous [:marcia]
	Comment 16 • 7 years ago

Tracking+ for all branches nominated.

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

Comment on attachment 8851546 [details] [diff] [review]
Add some overflow and write error checks, v3

Too late for beta, with only a week before the RC build. Let's take it on aurora though.

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

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 7 years ago

Comment on attachment 8851546 [details] [diff] [review]
Add some overflow and write error checks, v3

54 has had this fix for a month now, it would be nice to keep ESR52.2 in sync.

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

https://hg.mozilla.org/releases/mozilla-esr52/rev/145905da25d3