Alex Gaynor [:Alex_Gaynor] Reporter
	Description • 6 years ago

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

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 1 • 6 years ago

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

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 2 • 6 years ago

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

	Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }} Assignee
	Comment 4 • 6 years ago

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

	Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }} Assignee
	Comment 5 • 6 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 6 • 6 years ago

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

	Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }} Assignee
	Comment 8 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 6 years ago

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.

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 10 • 6 years ago

You should clear to land now.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 6 years ago

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

	Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }} Assignee
	Comment 13 • 6 years ago

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

See Comment 8

	Alex Gaynor [:Alex_Gaynor] Reporter
	Comment 14 • 6 years ago

Should the ESR52 approval be on the second patch?

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago

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.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 6 years ago
uplift

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

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 6 years ago
uplift

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