1392739 - IPC: wild-addr-read in various messages [@CharAt]

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack.txt

The following crash occurs consistently on mozilla-central. Last tested revision is 20170817-a6a1f5c1d971.

INFO: This is an IPC crash found by the fuzzer faulty - there is no test-case available which leads to an immediate crash for reproduction.

The attached session.txt contains a trace of IPC messages which were sent and received during a session of visiting  https://html5test.com


*** Possible reproduction scenario:

pip install git+https://github.com/mozillasecurity/fuzzfetch
fuzzfetch -a --fuzzing -n firefox -o /tmp

export FAULTY_PROBABILITY=50000
export FAULTY_LARGE_VALUES=1
export FAULTY_PARENT=1
export FAULTY_ENABLE_LOGGING=1
export FAULTY_PICKLE=1
export MOZ_IPC_MESSAGE_LOG=1


*** Messages which correlate with the stack:

[...]
[8776->8724] [PCookieServiceChild] Sending  PCookieService::Msg_SetCookieString
[8724<-8776] [PCookieServiceParent] Received  PCookieService::Msg_SetCookieString
[8724->8776] [PCookieServiceParent] Sending  PCookieService::Msg_AddCookie
[8776->8724] [PContentChild] Sending  PContent::Msg_ClassifyLocal
[8724<-8776] [PContentParent] Received  PContent::Msg_ClassifyLocal
[8724->8776] [PContentParent] Sending reply  PContent::Reply_ClassifyLocal
[8776<-8724] [PContentChild] Received reply  PContent::Reply_ClassifyLocal
[8724->8776] [PHttpChannelParent] Sending  PHttpChannel::Msg_OnStartRequest
[8724->8776] [PHttpBackgroundChannelParent] Sending  PHttpBackgroundChannel::Msg_OnStartRequestSent
[8724->8776] [PHttpBackgroundChannelParent] Sending  PHttpBackgroundChannel::Msg_OnTransportAndData
[8724->8776] [PHttpBackgroundChannelParent] Sending  PHttpBackgroundChannel::Msg_OnStopRequest
[8776<-8724] [PHttpBackgroundChannelChild] Received  PHttpBackgroundChannel::Msg_OnStartRequestSent
[8776<-8724] [PHttpBackgroundChannelChild] Received  PHttpBackgroundChannel::Msg_OnTransportAndData
[8776<-8724] [PHttpBackgroundChannelChild] Received  PHttpBackgroundChannel::Msg_OnStopRequest
[Faulty] pickle field {int} of value: 0 changed to: 1
[8776->8724] [PCookieServiceChild] Sending  PCookieService::Msg_SetCookieString
[8724<-8776] [PCookieServiceParent] Received  PCookieService::Msg_SetCookieString
[...]


*** Stack

#0 http://searchfox.org/mozilla-central/rev/48ea4528/xpcom/string/nsTString.h#131
#1 http://searchfox.org/mozilla-central/rev/48ea4528/netwerk/base/nsStandardURL.h#386
#2 http://searchfox.org/mozilla-central/rev/48ea4528/netwerk/base/nsStandardURL.cpp#1656


*** 

The crash pops up through every IPC message which makes use of "mozilla::net::nsStandardURL::GetAsciiHost".

Another variation:

CharAt obj-firefox/dist/include/nsTString.h:131:12
Host netwerk/base/nsStandardURL.h:386
mozilla::net::nsStandardURL::GetAsciiHost netwerk/base/nsStandardURL.cpp:1656
nsUrlClassifierUtils::GetKeyForURI toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:194:13
nsUrlClassifierDBService::ClassifyLocalWithTables toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1915:22
mozilla::dom::ContentParent::RecvClassifyLocal dom/ipc/ContentParent.cpp:5368:25
mozilla::dom::PContentParent::OnMessageReceived obj-firefox/ipc/ipdl/PContentParent.cpp:7473:20
[...]

Comment 1

[Christoph Diehl [:posidron]]

Attachment: session.txt

Comment 2

[Christoph Diehl [:posidron]]

This CharAt is still a constant crasher in our cluster for IPC fuzzing.

==1829==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f9fb597aac4 at pc 0x7f9fa784376e bp 0x7ffdcd147f50 sp 0x7ffdcd147f48
READ of size 1 at 0x7f9fb597aac4 thread T0
    #0 0x7f9fa784376d in CharAt /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTString.h:175:12
    #1 0x7f9fa784376d in Host /builds/worker/workspace/build/src/netwerk/base/nsStandardURL.h:386
    #2 0x7f9fa784376d in mozilla::net::nsStandardURL::GetAsciiHost(nsTSubstring<char>&) /builds/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:1656
    #3 0x7f9fb32270dd in nsUrlClassifierUtils::GetKeyForURI(nsIURI*, nsTSubstring<char>&) /builds/worker/workspace/build/src/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:214:13
    #4 0x7f9fb321b780 in nsUrlClassifierDBService::AsyncClassifyLocalWithTables(nsIURI*, nsTSubstring<char> const&, nsIURIClassifierCallback*) /builds/worker/workspace/build/src/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1787:31
    #5 0x7f9fae610d98 in mozilla::dom::URLClassifierLocalParent::StartClassify(nsIURI*, nsTSubstring<char> const&) /builds/worker/workspace/build/src/dom/ipc/URLClassifierParent.cpp:68:25
    #6 0x7f9fae58e6bc in mozilla::dom::ContentParent::RecvPURLClassifierLocalConstructor(mozilla::dom::PURLClassifierLocalParent*, mozilla::ipc::URIParams const&, nsTString<char> const&) /builds/worker/workspace/build/src/dom/ipc/ContentParent.cpp:5315:17
    #7 0x7f9fa8c0adb8 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentParent.cpp:4209:20
    #8 0x7f9fa83edd69 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 
[...]

0x7f9fb597aac4 is located 60 bytes to the left of global variable '<string literal>' defined in '/builds/worker/workspace/build/src/xpcom/string/nsSubstring.cpp:309:3' (0x7f9fb597ab00) of size 59
  '<string literal>' is ascii string 'data[aLen] == char16_t(0) (data should be null terminated)'
0x7f9fb597aac4 is located 2 bytes to the right of global variable 'gNullChar' defined in '/builds/worker/workspace/build/src/xpcom/string/nsSubstring.cpp:43:23' (0x7f9fb597aac0) of size 2
SUMMARY: AddressSanitizer: global-buffer-overflow /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTString.h:175:12 in CharAt

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to Christoph Diehl [:posidron] from comment #2)
> This CharAt is still a constant crasher in our cluster for IPC fuzzing.

Christoph, could you let me know what the fuzzing actually does?
From what I get from comment 0 it randomly changes values in IPC messages?
If so, what would be the way to protect against this type of error?

Comment 4

[Christoph Diehl [:posidron]]

That's correct, Valentin.

I believe it is about how we deal with "URIParams& aHost", |URIParams| seems to be involved on the higher level in all these crashes related to "CharAt()".

- mozilla::dom::ContentParent::RecvPURLClassifierLocalConstructor
- mozilla::dom::ContentParent::RecvClassifyLocal 
- mozilla::net::CookieServiceParent::RecvSetCookieString

Perhaps in this case adjusting the assertion condition and / or replace the NS_ASSERTION with a NS_ABORT_IF_FALSE() ?

    char_type CharAt(index_type aIndex) const
     {
       NS_ASSERTION(aIndex <= mLength, "index exceeds allowable range");
*      return mData[aIndex];
     }


    inline const nsDependentCSubstring
    nsStandardURL::Host()
    {
       uint32_t pos=0, len=0;
       if (mHost.mLen > 0) {
           pos = mHost.mPos;
           len = mHost.mLen;
*          if (mSpec.CharAt(pos) == '[' && mSpec.CharAt(pos + len - 1) == ']') {
               pos++;
               len -= 2;
           }
       }
       return Substring(mSpec, pos, len);
    }


    // result is ASCII
    NS_IMETHODIMP
    nsStandardURL::GetAsciiHost(nsACString &result)
    {
*      result = Host();
       CALL_RUST_GETTER_STR(result, GetAsciiHost, result);
       return NS_OK;
    }

Comment 5

[Christoph Diehl [:posidron]]

Different variation

==18246==ERROR: AddressSanitizer: SEGV on unknown address 0x606100a17e67 (pc 0x7fe63aceeadb bp 0x7fff072964e0 sp 0x7fff072963e0 T0)
==18246==The signal is caused by a READ memory access.
    #0 0x7fe63aceeada in CharAt /builds/worker/workspace/build/src/obj-firefox/dist/include/nsTString.h:211:12
    #1 0x7fe63aceeada in mozilla::net::nsStandardURL::Deserialize(mozilla::ipc::URIParams const&) /builds/worker/workspace/build/src/netwerk/base/nsStandardURL.cpp:3708
    #2 0x7fe63acf9d3c in InitFromIPCParams /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIURIMutator.h:65:21
    #3 0x7fe63acf9d3c in mozilla::net::nsStandardURL::TemplatedMutator<mozilla::net::nsStandardURL>::Deserialize(mozilla::ipc::URIParams const&) /builds/worker/workspace/build/src/netwerk/base/nsStandardURL.h:358
    #4 0x7fe63b95f90c in mozilla::ipc::DeserializeURI(mozilla::ipc::URIParams const&) /builds/worker/workspace/build/src/ipc/glue/URIUtils.cpp:121:26
    #5 0x7fe641edc1fe in mozilla::dom::ContentParent::RecvVisitURI(mozilla::ipc::URIParams const&, mozilla::ipc::OptionalURIParams const&, unsigned int const&) /builds/worker/workspace/build/src/dom/ipc/ContentParent.cpp:3636:29
    #6 0x7fe63baec4eb in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PContentParent.cpp:4459:20
    #7 0x7fe63b94a7ee in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2135:25
    #8 0x7fe63b947771 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2065:17
    #9 0x7fe63b948f6c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1911:5
    #10 0x7fe63b9495c8 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1944:15
    #11 0x7fe63aa72c78 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1096:14
    #12 0x7fe63aa8efe0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:517:10
    #13 0x7fe63b952356 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:125:5
    #14 0x7fe63b8a0b89 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #15 0x7fe63b8a0b89 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #16 0x7fe63b8a0b89 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #17 0x7fe64265984a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #18 0x7fe64696abab in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #19 0x7fe646b7705c in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4766:22
    #20 0x7fe646b7a196 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4911:8
    #21 0x7fe646b7b654 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:5003:21
    #22 0x4f6d45 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:231:22
    #23 0x4f6d45 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:304
    #24 0x7fe65a29882f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #25 0x4265bc in _start (/home/ubuntu/firefox/firefox+0x4265bc)

Comment 6

[Christoph Diehl [:posidron]]

Attachment: messages.zip

Faulty received some upgrades.
 
This specific crash is still a frequent crasher and involves any message which makes use of those functions and there seem to be quite a lot of those messages. :-)

Message: PContent::Msg_StartVisitedQuery

#0 0x7fcaac12b634 in CharAt obj/ff-asan-release/dist/include/nsTString.h:211:12
#1 0x7fcaac12b634 in mozilla::net::nsStandardURL::Deserialize(mozilla::ipc::URIParams const&) netwerk/base/nsStandardURL.cpp:3643
#2 0x7fcaac13b407 in InitFromIPCParams obj/ff-asan-release/dist/include/nsIURIMutator.h:69:21
#3 0x7fcaac13b407 in mozilla::net::nsStandardURL::TemplatedMutator<mozilla::net::nsStandardURL>::Deserialize(mozilla::ipc::URIParams const&) /netwerk/base/nsStandardURL.h:339
#4 0x7fcaad0af1e7 in mozilla::ipc::DeserializeURI(mozilla::ipc::URIParams const&) ipc/glue/URIUtils.cpp:121:26
#5 0x7fcab3cf0ee4 in mozilla::dom::ContentParent::RecvStartVisitedQuery(mozilla::ipc::URIParams const&) dom/ipc/ContentParent.cpp:3648:29
#6 0x7fcaad2fd7d5 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) obj/ff-asan-release/ipc/ipdl/PContentParent.cpp:4428:20

I have attached of a couple of "Msg_StartVisitedQuery" messages with the original and the mutated message. Unzip and do for each message to see which bytes got mutated.

$ hexdiff message.12126.177628.{o,m}

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Bug 1459961 has a reproducer under libFuzzer; I could turn it into a gtest if that'd be helpful?

Comment 9

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #8)
> Bug 1459961 has a reproducer under libFuzzer; I could turn it into a gtest
> if that'd be helpful?

I've been caught up with some other bugs, and didn't manage to get to this yet.
A gtest would actually be very helpful. Thanks!

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Attachment: 1392739-test.diff

Here's a gtest for this -- the problem is almost certainly the integer overflow here: https://searchfox.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#3497

Using mozilla::CheckedInt could resolve this.

Comment 11

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Thanks for the test Alex. You're right, we needed to use CheckedInt there.
I do have a question about these failures, though. After fixing Deserialize to return false in case of overflow, the test would still crash, as DeserializeURI asserts when it fails.
Is this OK for fuzzing? Or do we only fuzz non-debug builds?
Users are obviously OK, since they are also running non-debug.

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Where do we crash? It looks like we return |false| upwards from |nsStandardURL::Deserialize|.

Right now I'm fuzzing with a non-debug opt build, so I guess it's fine :-)

Comment 13

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Use CheckedInt in nsStandardURL::Deserialize()

Comment 14

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #12)
> Where do we crash? It looks like we return |false| upwards from
> |nsStandardURL::Deserialize|.
> 
> Right now I'm fuzzing with a non-debug opt build, so I guess it's fine :-)

DeserializeURI asserts here: https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/ipc/glue/URIUtils.cpp#123
I've changed the test check nsStandardURL::Deserialize instead.

Comment 15

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Add test for faulty nsStandardURL deserialization

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8980086 [details] [diff] [review]
Use CheckedInt in nsStandardURL::Deserialize()

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

::: netwerk/base/nsStandardURL.cpp
@@ +3491,5 @@
>      if (NS_WARN_IF(aSegment.length() < -1)) {
>          return false;
>      }
>  
> +    CheckedInt<uint32_t> segmentLen = CheckedInt<uint32_t>(aSegment.position()) + aSegment.length();

would 

`
CheckedInt<uint32_t> segmentLen = aSegment.position();
segmentLen += aSegment.length();
`

be a bit more optimal?  (no need to create the temp CheckedInt)

Comment 17

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Use CheckedInt in nsStandardURL::Deserialize()

Updated the patch to match Honza's suggestion in comment 16

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c47f8a1bad20a61a1ec699cc6508e21f39a7a98
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72ceb91e620b9b95806132fe9ca3687fab49b68

Please request Beta/ESR60 approval on this when you get a chance. It grafts cleanly as-landed. Please also attach a rebased patch for ESR52 and request approval.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c47f8a1bad2
https://hg.mozilla.org/mozilla-central/rev/f72ceb91e620

Comment 20

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: [esr52] Use CheckedInt in nsStandardURL::Deserialize()

* Add test for faulty nsStandardURL deserialization r=mayhemer

Comment 21

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Comment on attachment 8983421 [details] [diff] [review]
[esr52] Use CheckedInt in nsStandardURL::Deserialize()

Approval Request Comment
[Feature/Bug causing the regression]:
Unknown. Has existed for a long time.

[User impact if declined]:
Potential out of bounds memory access.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[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?]:
No.

[Why is the change risky/not risky?]:
It just adds a check for overflow to the existing code.

[String changes made/needed]:
None.

Comment 22

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Comment on attachment 8980091 [details] [diff] [review]
Add test for faulty nsStandardURL deserialization

See comment 21

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8980091 [details] [diff] [review]
Add test for faulty nsStandardURL deserialization

Fixes a Necko IPC sec issue and includes a test. Approved for 61.0b12, ESR 60.1, and ESR 52.9.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c8797e136acf
https://hg.mozilla.org/releases/mozilla-beta/rev/6c3374416afc

https://hg.mozilla.org/releases/mozilla-esr60/rev/5e8ae758ac7f
https://hg.mozilla.org/releases/mozilla-esr60/rev/f1465ba4ff3a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0ddfc03c0454