Closed Bug 1346419 Opened 3 years ago Closed 3 years ago

Assertion failure: mLength > 0 (|First()| called on an empty string)

Categories

(Core :: Networking, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + verified
firefox54 + verified
firefox55 + verified

People

(Reporter: tsmith, Assigned: bagder)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [necko-active][adv-main53+][adv-esr52.1+][post-critsmash-triage])

Attachments

(3 files, 3 obsolete files)

Attached file test_case.html (obsolete) —
Assertion failure: mLength > 0 (|First()| called on an empty string), at /home/worker/workspace/build/src/xpcom/string/nsTSubstring.cpp:41

==15253==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f1735108843 bp 0x7ffdf079c610 sp 0x7ffdf079c610 T0)
    #0 0x7f1735108842 in nsACString_internal::First() const /home/worker/workspace/build/src/xpcom/string/nsTSubstring.cpp:41:3
    #1 0x7f17359f342b in nsIndexedToHTML::OnIndexAvailable(nsIRequest*, nsISupports*, nsIDirIndex*) /home/worker/workspace/build/src/netwerk/streamconv/converters/nsIndexedToHTML.cpp:706:9
    #2 0x7f17359e0170 in nsDirIndexParser::ProcessData(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/netwerk/streamconv/converters/nsDirIndexParser.cpp:412:13
    #3 0x7f17359e2675 in nsDirIndexParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/worker/workspace/build/src/netwerk/streamconv/converters/nsDirIndexParser.cpp:350:10
    #4 0x7f17354142f2 in nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) /home/worker/workspace/build/src/netwerk/base/nsBaseChannel.cpp:856:17
    #5 0x7f173545fd93 in nsInputStreamPump::OnStateTransfer() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:601:22
    #6 0x7f173545e78a in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:429:25
    #7 0x7f173524642d in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:96:9
    #8 0x7f17352a8f62 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1264:7
    #9 0x7f17352a5810 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10
    #10 0x7f17360b7a6f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #11 0x7f1736029678 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3
    #12 0x7f1736029678 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #13 0x7f1736029678 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #14 0x7f173b51500f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #15 0x7f173eb7c0f1 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #16 0x7f173ed4c30c in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4476:10
    #17 0x7f173ed4de0c in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4654:8
    #18 0x7f173ed4f20c in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4745:16
    #19 0x4dffaf in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:10
    #20 0x4dffaf in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:307
    #21 0x7f175076182f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #22 0x41c3d8 in _start (/home/user/workspace/browsers/firefox_cnt/firefox+0x41c3d8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/xpcom/string/nsTSubstring.cpp:41:3 in nsACString_internal::First() const
Flags: in-testsuite?
Attached file raw_data.txt (obsolete) —
See Also: → CVE-2017-5444
INFO: Last good revision: cc682c2db247433a4aad347f0d20ddbc968eb26b
INFO: First bad revision: c214e63d44553f3391bd1f7dc686c008537c2f54
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cc682c2db247433a4aad347f0d20ddbc968eb26b&tochange=c214e63d44553f3391bd1f7dc686c008537c2f54

Bug 1340577 looks like a likely candidate in there.
Maybe that's a bogus regression range, though? I'm assuming that before bug 1340577, we would have just gotten into a dangerous state without crashing?
Apparently the caller to First() needs a fix, right?  If the string is empty, First may return \0 if the string is zero-terminated.  But we also have strings that are substrings defined by start pointer and length.  When length of such a string is 0, First() is undefined.  Hence, the caller must first check non-emptyness of the string First is called on.
Flags: needinfo?(honzab.moz)
I believe the fix I submitted to bug 1344461 will fix this case as well.
Assignee: nobody → daniel
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Maybe that's a bogus regression range, though? I'm assuming that before bug
> 1340577, we would have just gotten into a dangerous state without crashing?

This is a correct interpretation of the situation.  Comment 4 suggests that we *might* not be in a dangerous situation if First() simply returned \0, but we might return something else, which would be bogus, and we might run off into the weeds.
Track 54+/55+ as security relate issue.
daniel, please clarify if this is a dup, should be closed, or still needs work.
Flags: needinfo?(daniel)
Whiteboard: [necko-active]
As it has the same root cause, let's call it a dupe
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(daniel)
Resolution: --- → DUPLICATE
Duplicate of bug: CVE-2017-5444
I can still reproduce this on mozilla-central 20170321230853-8744e9f8eb which should have the fix for bug 1344461
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached file test_case.html
Attachment #8846158 - Attachment is obsolete: true
Attached file raw_data.bin
Attachment #8846159 - Attachment is obsolete: true
Ok, thank you. I'll investigate deeper.
Here's a simple patch that seems to fix it for me. It'd be great to hear how it performs for you :tsmith!

What tricked me with this bug compared to the other one, is that this issue is now only calling .First() on an empty string and there's really no other overflow/buffer boundary problem that I can spot.
Comment on attachment 8849908 [details] [diff] [review]
0001-bug-1346419-bail-out-on-zero-length-data-r-valentin.patch

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

::: netwerk/streamconv/converters/nsIndexedToHTML.cpp
@@ +703,5 @@
>  
>      // Adjust the length in case unescaping shortened the string.
>      loc.Truncate(nsUnescapeCount(loc.BeginWriting()));
> +
> +    if (loc.Length() == 0) {

loc.IsEmpty()
look at that, thanks! =)
Flags: needinfo?(twsmith)
Duplicate of this bug: 1349954
v2, uses IsEmpty() instead
Attachment #8849908 - Attachment is obsolete: true
Attachment #8850861 - Flags: review?(valentin.gosu)
Attachment #8850861 - Flags: review?(valentin.gosu) → review+
Are we sure callers of nsIndexedToHTML::OnIndexAvailable are ready to handle the failure result?
Yes I think so.

There's only one call to this method (in nsDirIndexParser:ProcessData) and it actually ignores the return code completely.

That, and this function can already return errors so this isn't the first place to do it. The main thing here is to get out when the string to work with is empty.
(In reply to Daniel Stenberg [:bagder] from comment #14)
> Here's a simple patch that seems to fix it for me. It'd be great to hear how
> it performs for you :tsmith!

I tested both patches and the assertion has been fixed. Running the fuzzer against it didn't turn up anything new either.
Flags: needinfo?(twsmith)
Thanks for that!
Keywords: checkin-needed
You shouldn't land security bugs until they have ratings. Though this one sounds not too bad.
This commit alone seems to not trigger those problems: https://treeherder.mozilla.org/#/jobs?repo=try&revision=975cd43dbf83eeeba783020765b7a17951ff3303&selectedJob=87813538

Later try runs in bug 1344467 seems to indicate that was the culprit.
Flags: needinfo?(daniel)
https://hg.mozilla.org/integration/mozilla-inbound/rev/820b47fb3e42663c49fb8acb2c10da321b3128db

Looks like we'll want to nominate this for backport to Aurora/Beta/ESR52 as well. Probably not worth ESR45 at this point in its lifecycle, though.
Comment on attachment 8850861 [details] [diff] [review]
v2-0001-bug-1346419-bail-out-on-zero-length-data-r-valent.patch

Approval Request Comment
[Feature/Bug causing the regression]: 1346419
[User impact if declined]: security flaw remains
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: it is about to
[Needs manual test from QE? If yes, steps to reproduce]: included in bug
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minimal patch in code that hasn't changed
[String changes made/needed]: none
Flags: needinfo?(daniel)
Attachment #8850861 - Flags: approval-mozilla-esr52?
Attachment #8850861 - Flags: approval-mozilla-beta?
Attachment #8850861 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/820b47fb3e42
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8850861 [details] [diff] [review]
v2-0001-bug-1346419-bail-out-on-zero-length-data-r-valent.patch

Fix a security issue. Aurora54+ & Beta53+.
Attachment #8850861 - Flags: approval-mozilla-beta?
Attachment #8850861 - Flags: approval-mozilla-beta+
Attachment #8850861 - Flags: approval-mozilla-aurora?
Attachment #8850861 - Flags: approval-mozilla-aurora+
Comment on attachment 8850861 [details] [diff] [review]
v2-0001-bug-1346419-bail-out-on-zero-length-data-r-valent.patch

might as well take this in esr52
Attachment #8850861 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [necko-active] → [necko-active][adv-main53+][adv-esr52.1+]
Group: network-core-security → core-security-release
Flagging this for manual testing, testcase in Comment 0.
Flags: qe-verify+
Hello,
I tried reproducing this but I'm having some issues with the attachment "test_case.html". Is this attachment still reliable? Is there any other way to manually test the issue?
(In reply to Alexandru Simonca, QA (:asimonca) from comment #36)
> Hello,
> I tried reproducing this but I'm having some issues with the attachment
> "test_case.html". Is this attachment still reliable? Is there any other way
> to manually test the issue?

I had no issue verifying this bug. What is the problem you are seeing?

A debug build is required to reproduce the original bug. The expected behavior is for an "Index of" page to be display and no assertions should be raised.
Whiteboard: [necko-active][adv-main53+][adv-esr52.1+] → [necko-active][adv-main53+][adv-esr52.1+][post-critsmash-triage]
I have reproduced the bug and can confirm that it is VERIFIED FIXED. The "index of" page is displayed and no assertions were raised. 
Verified on latest Nightly 55.0a1 (id: 20170412030252), the latest Aurora 54.0a2 (id: 20170412004024) and on the latest Firefox RC 53.0 (build1) (id: 20170410140322).
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.