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

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

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

Comment 1

[Tyson Smith [:tsmith]]

Attachment: raw_data.txt

Comment 2

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 4

[Honza Bambas (:mayhemer)]

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.

Comment 5

[Daniel Stenberg [:bagder]]

I believe the fix I submitted to bug 1344461 will fix this case as well.

Comment 6

[Nathan Froyd [:froydnj]]

(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.

Comment 7

[Gerry Chang [:gchang]]

Track 54+/55+ as security relate issue.

Comment 8

[Patrick McManus [:mcmanus]]

daniel, please clarify if this is a dup, should be closed, or still needs work.

Comment 9

[Daniel Stenberg [:bagder]]

As it has the same root cause, let's call it a dupe

Comment 10

[Tyson Smith [:tsmith]]

I can still reproduce this on mozilla-central 20170321230853-8744e9f8eb which should have the fix for bug 1344461

Comment 11

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 12

[Tyson Smith [:tsmith]]

Attachment: raw_data.bin

Comment 13

[Daniel Stenberg [:bagder]]

Ok, thank you. I'll investigate deeper.

Comment 14

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1346419-bail-out-on-zero-length-data-r-valentin.patch

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 15

[Honza Bambas (:mayhemer)]

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()

Comment 16

[Daniel Stenberg [:bagder]]

look at that, thanks! =)

Comment 18

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1346419-bail-out-on-zero-length-data-r-valent.patch

v2, uses IsEmpty() instead

Comment 19

[Honza Bambas (:mayhemer)]

Are we sure callers of nsIndexedToHTML::OnIndexAvailable are ready to handle the failure result?

Comment 20

[Daniel Stenberg [:bagder]]

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.

Comment 21

[Tyson Smith [:tsmith]]

(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.

Comment 22

[Daniel Stenberg [:bagder]]

Thanks for that!

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Carsten Book [:Tomcat]]

backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1e98eaf0f70f7ce12244ac5bf9293048b507bb23&filter-searchStr=Linux+opt+Mochitests+executed+by+TaskCluster+test-linux32%2Fopt-mochitest-browser-chrome-4+tc-M(bc4) it seems that one of this 3 checkins cause times like https://treeherder.mozilla.org/logviewer.html#?job_id=86621316&repo=mozilla-inbound and seems the tests fail as example on connecting -> https://public-artifacts.taskcluster.net/B3lwHCOzQTe1j9McYz1LLg/0/public/test_info//mozilla-test-fail-screenshot_DRpsBk.png

Comment 25

[Andrew McCreight [:mccr8]]

You shouldn't land security bugs until they have ratings. Though this one sounds not too bad.

Comment 26

[Daniel Stenberg [:bagder]]

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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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 28

[Daniel Stenberg [:bagder]]

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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/820b47fb3e42

Comment 30

[Gerry Chang [:gchang]]

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+.

Comment 31

[Carsten Book [:Tomcat]]

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

Comment 32

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-beta/rev/39bd60385e7475126c9655dd2ecd5648e5161ce5

Comment 33

[Julien Cristau [:jcristau]]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1b148cf9c545

Comment 35

[Andrei Vaida [:avaida]]

Flagging this for manual testing, testcase in Comment 0.

Comment 36

[Alexandru Simonca [:asimonca], Release Desktop QA]

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?

Comment 37

[Tyson Smith [:tsmith]]

(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.

Comment 38

[Alexandru Simonca [:asimonca], Release Desktop QA]

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).