Closed Bug 1349621 Opened 3 years ago Closed 3 years ago

Use of uninitialized memory [@ NS_GetFinalChannelURI]

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: tsmith, Assigned: tsmith)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uninitialized, sec-moderate, testcase, Whiteboard: [fixed by bug 1349987][adv-main53+][adv-esr52.1+][post-critsmash-triage])

Attachments

(2 files)

I'm not sure if core networking is the correct component but I'll start here.

Conditional jump or move depends on uninitialised value(s)
   at 0x11411A66: NS_GetFinalChannelURI(nsIChannel*, nsIURI**) (nsNetUtil.cpp:1847)
   by 0x1249F4AF: nsContentSecurityManager::CheckChannel(nsIChannel*) (nsContentSecurityManager.cpp:553)
   by 0x124AAB32: nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&) (nsContentSecurityManager.cpp:475)
   by 0x119858B4: nsExtProtocolChannel::AsyncOpen2(nsIStreamListener*) (nsExternalProtocolHandler.cpp:227)
   by 0x1197E76D: nsURILoader::OpenURI(nsIChannel*, unsigned int, nsIInterfaceRequestor*) (nsURILoader.cpp:857)
   by 0x130028E8: nsDocShell::DoChannelLoad(nsIChannel*, nsIURILoader*, bool) (nsDocShell.cpp:11519)
   by 0x13013F03: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString const&, nsIURI*, unsigned int) [clone .part.380] (nsDocShell.cpp:11333)
   by 0x1301B2A5: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString const&, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:10792)
   by 0x1301FA88: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) [clone .part.452] (nsDocShell.cpp:1591)
   by 0x11CD2538: nsFrameLoader::ReallyStartLoadingInternal() (nsFrameLoader.cpp:873)
   by 0x11CD2622: nsFrameLoader::ReallyStartLoading() (nsFrameLoader.cpp:757)
   by 0x11CD272F: nsDocument::MaybeInitializeFinalizeFrameLoaders() (nsDocument.cpp:7196)
 Uninitialised value was created by a heap allocation
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4056C4: moz_xmalloc (mozalloc.cpp:83)
   by 0x11988AC1: operator new (mozalloc.h:194)
   by 0x11988AC1: nsExternalProtocolHandler::NewChannel2(nsIURI*, nsILoadInfo*, nsIChannel**) (nsExternalProtocolHandler.cpp:554)
   by 0x11417F05: mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal(nsIURI*, nsIURI*, unsigned int, nsILoadInfo*, nsIChannel**) (nsIOService.cpp:793)
   by 0x1141720B: NS_NewChannelInternal(nsIChannel**, nsIURI*, nsILoadInfo*, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*) (nsNetUtil.cpp:177)
   by 0x130133EE: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString const&, nsIURI*, unsigned int) [clone .part.380] (nsDocShell.cpp:11047)
   by 0x1301B2A5: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString const&, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:10792)
   by 0x1301FA88: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) [clone .part.452] (nsDocShell.cpp:1591)
   by 0x11CD2538: nsFrameLoader::ReallyStartLoadingInternal() (nsFrameLoader.cpp:873)
   by 0x11CD2622: nsFrameLoader::ReallyStartLoading() (nsFrameLoader.cpp:757)
   by 0x11CD272F: nsDocument::MaybeInitializeFinalizeFrameLoaders() (nsDocument.cpp:7196)
   by 0x11CD28B0: nsDocument::EndUpdate(unsigned int) (nsDocument.cpp:5030)
do you have an about:buildconfig cset so we're looking at the same code? (always a useful thing to include with a stacktrace..)
Attached file build_config.txt
Built from https://hg.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9

See attached build_config.txt for full details.
Assignee: nobody → twsmith
Attached file test_case.html
It looks like nsExternalProtocolHandler::mLoadFlags is not intialized in the ctor. Maybe that's it.
I'm going to mark this sec-moderate. If we end up bypassing a security check by returning the wrong URI, that sounds bad, but I don't know how much that matters for the mailto: protocol. (Tyson said it didn't work with other protocols, but presumably the user could register other handlers.)
Keywords: sec-moderate
(In reply to Andrew McCreight [:mccr8] from comment #4)
> It looks like nsExternalProtocolHandler::mLoadFlags is not intialized in the
> ctor. Maybe that's it.

worth trying - some kind of impressive inlining tho. the naive complaint is about something on the stack clearly initialized being &'d with a constant.
tyson - can you very the cset in comment 7? my machine wasn't up to the challenge of your testcase+valgrind :)
(In reply to Patrick McManus [:mcmanus] from comment #8)
> tyson - can you very the cset in comment 7? my machine wasn't up to the
> challenge of your testcase+valgrind :)

Verified, the patch removes the issue.
bizarrely honza patched the same long standing issue in the last 24 hours. I won't mark it as dup just because this is a sec bug - but this is really a dup of 1349987
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Maybe worth landing the testcase at some point still.
Flags: in-testsuite?
Whiteboard: [fixed by bug 1349987]
Group: network-core-security → core-security-release
Whiteboard: [fixed by bug 1349987] → [fixed by bug 1349987][adv-main53+][adv-esr52.1+]
Flags: qe-verify-
Whiteboard: [fixed by bug 1349987][adv-main53+][adv-esr52.1+] → [fixed by bug 1349987][adv-main53+][adv-esr52.1+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.