1456975 - Segfault - buffer overflow / arbitrary memory read in IPC due to unvalidated field in nsMozIconURI deserialization

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: minimized-from-67447ff63f6161ff7b3a7220157c2bebec80051f

The bug is triggered here https://searchfox.org/mozilla-central/source/image/decoders/icon/nsIconURI.cpp#103 because there exists some path in the IPC that sets mIconSize without validating it.

If you want access to bug 1451859 so you can reproduce, let me know and I can CC you.

osboxes@osboxes:~/mozilla-central$ (cd obj-x86_64-pc-linux-gnu/dist/bin/; MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=ContentParentIPC ./firefox -artifact_prefix=/home/osboxes/content-parent-artifacts/ /home/osboxes/content-parent-artifacts/minimized-from-67447ff63f6161ff7b3a7220157c2bebec80051f -len_control=0 -max_len=384 -rss_limit_mb=0 -close_fd_mask=2)
Running Fuzzer tests...
INFO: Seed: 3613430132
INFO: Loaded 1 modules   (1636978 guards): 1636978 [0x7fbca02a7d80, 0x7fbca08e6748),
./firefox: Running 1 inputs 1 time(s) each.
Running: /home/osboxes/content-parent-artifacts/minimized-from-67447ff63f6161ff7b3a7220157c2bebec80051f
=================================================================
==29686==ERROR: AddressSanitizer: SEGV on unknown address 0x7fc039beeec8 (pc 0x7fbc8b0bab23 bp 0x7ffd3e64fed0 sp 0x7ffd3e64fd40 T0)
==29686==The signal is caused by a READ memory access.
    #0 0x7fbc8b0bab22 in nsTSubstring<char>::Append(char const*, unsigned int) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTSubstring.h:368:31
    #1 0x7fbc8b0bab22 in nsTSubstring<char>::operator+=(char const*) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTSubstring.h:503
    #2 0x7fbc8b0bab22 in nsMozIconURI::GetSpec(nsTSubstring<char>&) /home/osboxes/mozilla-central/image/decoders/icon/nsIconURI.cpp:103
    #3 0x7fbc95ffa6c0 in nsNavHistory::CanAddURI(nsIURI*, bool*) /home/osboxes/mozilla-central/toolkit/components/places/nsNavHistory.cpp:923:23
    #4 0x7fbc95ffefc8 in mozilla::places::History::SetURITitle(nsIURI*, nsTSubstring<char16_t> const&) /home/osboxes/mozilla-central/toolkit/components/places/History.cpp:3017:29
    #5 0x7fbc90989f6a in mozilla::dom::ContentParent::RecvSetURITitle(mozilla::ipc::URIParams const&, nsTString<char16_t> const&) /home/osboxes/mozilla-central/dom/ipc/ContentParent.cpp:3625:14
    #6 0x7fbc889eb514 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/ipc/ipdl/PContentParent.cpp:4458:20
    #7 0x7fbc9813bc18 in void mozilla::ipc::FuzzProtocol<mozilla::dom::ContentParent>(mozilla::dom::ContentParent*, unsigned char const*, unsigned long, std::unordered_set<unsigned int, std::hash<unsigned int>, std::equal_to<unsigned int>, std::allocator<unsigned int> >&) /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/ProtocolFuzzer.h:48:18
    #8 0x7fbc9813ae26 in RunContentParentIPCFuzzing(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/dom/ipc/fuzztest/content_parent_ipc_libfuzz.cpp:67:3
    #9 0x5ee47d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
    #10 0x5c65ef in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:280:6
    #11 0x5d2a21 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/osboxes/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:703:9
    #12 0x7fbc967101e1 in mozilla::FuzzerRunner::Run(int*, char***) /home/osboxes/mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10
    #13 0x7fbc96621528 in XREMain::XRE_mainStartup(bool*) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4021:35
    #14 0x7fbc96636196 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:4957:12
    #15 0x7fbc96637e6d in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/osboxes/mozilla-central/toolkit/xre/nsAppRunner.cpp:5064:21
    #16 0x51ea9c in do_main(int, char**, char**) /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:231:22
    #17 0x51ea9c in main /home/osboxes/mozilla-central/browser/app/nsBrowserApp.cpp:304
    #18 0x7fbcad9eb1c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #19 0x421bc9 in _start (/home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x421bc9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/osboxes/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/nsTSubstring.h:368:31 in nsTSubstring<char>::Append(char const*, unsigned int)
==29686==ABORTING
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x28,0x0,0x0,0x0,0xff,0xff,0xff,0x7f,0x97,0x0,0x2d,0x0,0x45,0x6e,0x76,0x50,0x6c,0x69,0x67,0x6e,0x75,0x7b,0x6c,0x75,0x67,0x6e,0x73,0x0,0x0,0x0,0x0,0x0,0x4,0x0,0x0,0x0,0x1,0x0,0x0,0x0,0x9b,0x9a,0x9a,0xba,0x73,0x0,0x0,0x0,0x0,0x78,0x69,0x74,0x63,0x6f,0x64,0x65,0x65,0x72,0x5f,0x73,0x6d,0x78,0x69,0x74,0x63,0x0,0x0,0x72,0x5f,0x73,0x6d,0x61,
(\x00\x00\x00\xff\xff\xff\x7f\x97\x00-\x00EnvPlignu{lugns\x00\x00\x00\x00\x00\x04\x00\x00\x00\x01\x00\x00\x00\x9b\x9a\x9a\xbas\x00\x00\x00\x00xitcodeer_smxitc\x00\x00r_sma

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

I suspect the offending assignment is here: https://searchfox.org/mozilla-central/source/image/decoders/icon/nsIconURI.cpp#718

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Note: You'll also want to check |params.iconState()| against |ArrayLength(kStateStrings)|.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Check fields in nsMozIconURI deserialization

Comment 4

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc00c6d214ffd963e777753b0882ee8bc163aa3d

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8972559 [details] [diff] [review]
Check fields in nsMozIconURI deserialization

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
An exploit would require another vulnerability in order to compromise the child process.

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

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?
Has always existed.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes. Very easy to backport.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions - just a couple of checks added.

Comment 6

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

sec-approval+ for checkin on May 29, three weeks into the new cycle (and not before!). 
Please nominate Beta and ESR patches then too.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: [esr52] Check fields in nsMozIconURI deserialization

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8972559 [details] [diff] [review]
Check fields in nsMozIconURI deserialization

Approval Request Comment
[Feature/Bug causing the regression]:
Always existed.

[User impact if declined]:
Potential attack vector from a compromised content process.

[Is this code covered by automated tests?]:
While there are a few tests for IconURIs, the deserialization code isn't tested in depth.

[Has the fix been verified in Nightly?]:
It will land on Firefox 62.

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

[Why is the change risky/not risky?]:
We just add some sanity checks to IPC deserialization code.

[String changes made/needed]:
None.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8972559 [details] [diff] [review]
Check fields in nsMozIconURI deserialization

Thanks for doing the rebased patches. Let's hold off on the nominations until later this month though to avoid bug query noise.

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

You should clear to land now.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6af9d912eef5f79eda192689ca8414664502cf37

Comment 12

[Ryan VanderMeulen [:RyanVM]]

If someone could do the Beta/ESR60/ESR52 approval requests, that would be appreciated :)

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8972559 [details] [diff] [review]
Check fields in nsMozIconURI deserialization

See Comment 8

Comment 14

[Alex Gaynor [:Alex_Gaynor]]

Should the ESR52 approval be on the second patch?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8972559 [details] [diff] [review]
Check fields in nsMozIconURI deserialization

Add some sanity checks to avoid possibly-exploitable situations in the IPC code. Approved for 61.0b10, ESR 60.1, and ESR 52.9.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6af9d912eef5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d31a2ab994fd
https://hg.mozilla.org/releases/mozilla-esr60/rev/c23f3d22028a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/042f80f3befd