1370070 - Assertion failure: CheckCapacity(aLength) (String is too large.), at dist/include/nsTSubstring.h:1125

Status: RESOLVED FIXED

(Core :: DOM: File, defect)

Description

[Christian Holler (:decoder)]

The following xpcshell testcase crashes on mozilla-central revision 96e18bec9fc8 (build with --enable-optimize --disable-debug --enable-address-sanitizer):

var { classes: Cc, interfaces: Ci, utils: Cu } = Components;
Cu.importGlobalProperties(['Blob', 'FileReader']);
var arrb4 = new ArrayBuffer(2147483639);
var bmxa5 = new Array();
bmxa5.push(arrb4);
var blob9 = new Blob(bmxa5);
var flrd4 = new FileReader();
flrd4.readAsText(blob9);
print("Waiting for reader to finish...");
var gThreadManager = Cc["@mozilla.org/thread-manager;1"]
    .getService(Ci.nsIThreadManager);
var mainThread = gThreadManager.currentThread;
/* Wait for reader to finish */
while (flrd4.readyState == 1)
  mainThread.processNextEvent(true);
print("Done");



Backtrace:

==12495==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f1ba1306733 bp 0x7ffe507b0fb0 sp 0x7ffe507b0c40 T0)
==12495==The signal is caused by a WRITE memory access.
==12495==Hint: address points to the zero page.
    #0 0x7f1ba1306732 in nsACString::nsACString(char*, unsigned int, unsigned int) objdir-ff-afl64opt/dist/include/nsTSubstring.h:1125:5
    #1 0x7f1ba1306732 in nsDependentCSubstring::nsDependentCSubstring(char const*, unsigned int) objdir-ff-afl64opt/dist/include/nsTDependentSubstring.h:42
    #2 0x7f1ba1306732 in mozilla::dom::FileReader::GetAsText(mozilla::dom::Blob*, nsACString const&, char const*, unsigned int, nsAString&) dom/file/FileReader.cpp:483
    #3 0x7f1ba1308d4a in mozilla::dom::FileReader::OnLoadEnd(nsresult) dom/file/FileReader.cpp
    #4 0x7f1ba13081bc in mozilla::dom::FileReader::OnInputStreamReady(nsIAsyncInputStream*) dom/file/FileReader.cpp:664:12
    #5 0x7f1b9ab75b0a in nsInputStreamReadyEvent::Run() xpcom/io/nsStreamUtils.cpp:96:20
    #6 0x7f1b9abe40ca in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1302:14
    #7 0x7f1b9ac052c1 in NS_InvokeByIndex xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129
    #8 0x7f1b9cdc2d85 in CallMethodHelper::Invoke() js/xpconnect/src/XPCWrappedNative.cpp:1996:12
    #9 0x7f1b9cdc2d85 in CallMethodHelper::Call() js/xpconnect/src/XPCWrappedNative.cpp:1315
    #10 0x7f1b9cdc2d85 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:1282
    #11 0x7f1b9cdca4b8 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:982:12
    #12 0x7f1b49cdaad5  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV objdir-ff-afl64opt/dist/include/nsTSubstring.h:1125:5 in nsACString::nsACString(char*, unsigned int, unsigned int)
==12495==ABORTING


The assertion fired here is a release assertion, so I assume this is not a problem. However, I don't know if the fact that we can get such a large blob into the FileReader in the first place violates any other constrains, so I'll mark this s-s until a developer has confirmed this to be harmless.

Comment 1

[Andrew McCreight [:mccr8]]

(In reply to Christian Holler (:decoder) from comment #0)
> The assertion fired here is a release assertion, so I assume this is not a
> problem. However, I don't know if the fact that we can get such a large blob
> into the FileReader in the first place violates any other constrains, so
> I'll mark this s-s until a developer has confirmed this to be harmless.

Any thoughts on this, Andrea?

Comment 2

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

I think baku is on vacation.

This reminds a bit bug 1349719.
Should we make CheckCapacity public and use it in FileReader::GetAsText or what? Or perhaps just 
make dependent string empty if too large capacity is requested?

Comment 3

[Eric Rahm [:erahm]]

I was going to suggest a fix on the string side but that would be pretty involved. Adding a version of |nsContentUtils::ConvertStringFromEncoding| that takes a raw string -- it just unpacks the string and passes it to TextDecoder::Decode anyhow -- would do the job and pass back an error code (NS_ERROR_OUT_OF_MEMORY) and we could handle that gracefully instead of asserting. 

What do you think Olli?

Comment 4

[Eric Rahm [:erahm]]

Well on the plus side I found a real sec issue so yay, patch forthcoming.

Comment 5

[Eric Rahm [:erahm]]

Attachment: Avoid unnecessary string creation

This takes care of two issues:
  #1 - We no longer create a nsDependentSubstring and just assert, instead we
       count on ConvertStringFromEncoding to return a reasonable error code.
  #2 - ConvertStringFromEncoding was doing an unchecked uint32_t -> int32_t
       conversion which, as we've seen in the past, can lead to sec issues when
       buffers and their sizes are involved.

MozReview-Commit-ID: LLQQrCpK630

Comment 6

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

Comment on attachment 8876334 [details] [diff] [review]
Avoid unnecessary string creation

I guess we can do it this way.

+  return nsContentUtils::ConvertStringFromEncoding(
+      encoding, aFileData, aDataLen, aResult);
is tiny bit unusual indentation. I can't see 2 space indentation anywhere there.
I would possibly write it
  return
    nsContentUtils::ConvertStringFromEncoding(encoding, aFileData, aDataLen, aResult);
or
  return nsContentUtils::ConvertStringFromEncoding(encoding, aFileData,
                                                   aDataLen, aResult);

Comment 7

[Eric Rahm [:erahm]]

FWIW I think we do the right thing re: large array sizes in FileReader, I think the main area of concern would be |DoReadData| [1] but that has various size checks.

[1] http://searchfox.org/mozilla-central/rev/12afa8cc9cea4a141494f767541b411845eef7a0/dom/file/FileReader.cpp#284-343

Comment 8

[Eric Rahm [:erahm]]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8876334 [details] [diff] [review]
> Avoid unnecessary string creation
> 
> I guess we can do it this way.

It's the best option for now unfortunately. Having |nsDependentCString| default to empty wouldn't be expected by most users (we generally assume our constructors are infallible). Also I imagine having an empty blob is legit so it would be hard to differentiate failure. |Rebind| doesn't work either since it doesn't have a return value and |Assign| would do a full copy (it's possible we could add an override to |Assign| or a fallible |Rebind| but that was what I was referring to as being a bit involved). If you think it would be useful we can file a follow up of course.

> +  return nsContentUtils::ConvertStringFromEncoding(
> +      encoding, aFileData, aDataLen, aResult);
> is tiny bit unusual indentation. I can't see 2 space indentation anywhere
> there.
> I would possibly write it
>   return
>     nsContentUtils::ConvertStringFromEncoding(encoding, aFileData, aDataLen,
> aResult);
> or
>   return nsContentUtils::ConvertStringFromEncoding(encoding, aFileData,
>                                                    aDataLen, aResult);

I'll update to the latter.

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8876334 [details] [diff] [review]
Avoid unnecessary string creation

This is currently unrated, my guess is it should be sec-moderate. The original issue is somewhat of a non-issue (probably sec-audit), but the fix does expose a possible int conversion issue (that is patched).

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

Not easy, the previous code didn't have an issue (it just safely crashed).

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

No.

Which older supported branches are affected by this flaw?

I would guess all.

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?

Low risk, but probably not worth backporting. We have save behavior, worst case scenario would be DoS.

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

Low, small change that checks int conversion.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Call it a sec-moderate and check it in.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b18762d91f3fe1d6c828e6657189811b67b7548a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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