991471 - nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Nightly: bp-aabf8718-c548-4461-83b9-182822140403

Comment 1

[Jesse Ruderman]

Attachment: stack (lldb)

Comment 2

[Jesse Ruderman]

Attachment: stack (ASan)

Comment 3

[Patrick McManus [:mcmanus]]

valentin, you want to have a look?

0 	XUL 	net_ToLowerCase(char*, unsigned int) 	netwerk/base/src/nsURLHelper.cpp
1 	XUL 	nsStandardURL::SetHost(nsACString_internal const&) 	netwerk/base/src/nsStandardURL.cpp
2 	XUL 	mozilla::dom::URL::SetHostname(nsAString_internal const&) 	dom/base/URL.cpp
3 	XUL 	mozilla::dom::URLBinding::set_hostname 	obj-firefox/x86_64/dom/bindings/URLBinding.cpp
4 	XUL 	mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
5 	XUL 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
6 	XUL 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
7 	XUL 	js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
8 	XUL 	js::Shape::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool, JS::MutableHandle<JS::Value>) 	js/src/vm/Shape-inl.h
9 	XUL 	bool js::baseops::SetPropertyHelper<(js::ExecutionMode)0>(js::ExecutionModeTraits<(js::ExecutionMode)0>::ContextType, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, JS::MutableHandle<JS::Value>, bool) 	js/src/jsobj.cpp
10 	XUL 	Interpret 	js/src/jsobj.h
11 	XUL 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
12 	XUL 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
13 	XUL 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
14 	XUL 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
15 	XUL 	mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) 	obj-firefox/x86_64/dom/bindings/EventHandlerBinding.cpp
16 	XUL 	JS::Value mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling) 	obj-firefox/x86_64/dist/include/mozilla/dom/EventHandlerBinding.h
17 	XUL 	nsJSEventListener::HandleEvent(nsIDOMEvent*) 	dom/events/nsJSEventListener.cpp
18 	XUL 	mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) 	dom/events/EventListenerManager.cpp
19 	XUL 	mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) 	dom/events/EventListenerManager.cpp
20 	XUL 	mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) 	dom/events/EventDispatcher.cpp
21 	XUL 	mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) 	dom/events/EventDispatcher.cpp
22 	XUL 	nsDocumentViewer::LoadComplete(tag_nsresult) 	layout/base/nsDocumentViewer.cpp
23 	XUL 	nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, tag_nsresult) 	docshell/base/nsDocShell.cpp
24 	XUL 	nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) 	docshell/base/nsDocShell.cpp
25 	XUL 	_ZThn344_N10nsDocShell13OnStateChangeEP14nsIWebProgressP10nsIRequestj12tag_nsresult 	docshell/base/nsDocShell.cpp
26 	XUL 	nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) 	uriloader/base/nsDocLoader.cpp
27 	XUL 	nsDocLoader::doStopDocumentLoad(nsIRequest*, tag_nsresult) 	uriloader/base/nsDocLoader.cpp
28 	XUL 	nsDocLoader::DocLoaderIsEmpty(bool) 	uriloader/base/nsDocLoader.cpp
29 	XUL 	nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) 	uriloader/base/nsDocLoader.cpp
30 	XUL 	_ZThn8_N11nsDocLoader13OnStopRequestEP10nsIRequestP11nsISupports12tag_nsresult 	obj-firefox/x86_64/uriloader/base/Unified_cpp_uriloader_base0.cpp
31 	XUL 	nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) 	netwerk/base/src/nsLoadGroup.cpp
32 	XUL 	nsDocument::DoUnblockOnload() 	content/base/src/nsDocument.cpp
33 	XUL 	nsDocument::UnblockOnload(bool) 	content/base/src/nsDocument.cpp
34 	XUL 	nsDocument::DispatchContentLoadedEvents() 	content/base/src/nsDocument.cpp
35 	XUL 	nsRunnableMethodImpl<void (nsDocument::*)(), void, true>::Run() 	obj-firefox/x86_64/dist/include/nsThreadUtils.h
36 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
37 	XUL 	NS_ProcessPendingEvents(nsIThread*, unsigned int) 	xpcom/glue/nsThreadUtils.cpp
38 	XUL 	nsBaseAppShell::NativeEventCallback() 	widget/xpwidgets/nsBaseAppShell.cpp
39 	XUL 	nsAppShell::ProcessGeckoEvents(void*) 	widget/cocoa/nsAppShell.

Comment 4

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

I found the problem. 

1.  var url = new URL("http://localhost/");
2.  url.hostname = "";
3.  url.username = "tttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt";
4.  url.hostname = "www.mozilla.org";
5.  url.username = "";
6.  url.hostname = "www.mozilla.org";

After line 6, at frame #2 in the stack trace, 
mSpec is http://tttttttttttttt@/www.mozilla.org
mHost is [mPos:4294967238, mLen:15]
mUser is [mPos:7, mLen:-1] 

One question though, are http:/// and http://tttttt@/ valid urls?
Should instruction 2. actually fail?

Comment 5

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

Attachment: bug.patch

This seems to fix the crash, and it has the same behaviour as Chromium (nothing happens when setting the host to "" ).

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8401634 [details] [diff] [review]
bug.patch

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

::: netwerk/base/src/nsStandardURL.cpp
@@ +1459,5 @@
>  
>      LOG(("nsStandardURL::SetHost [host=%s]\n", host));
>  
> +    if (flat.IsEmpty())
> +        return NS_OK;

use {} to avoid goto fail

but the more interesting part is I need you to walk me through why this code is different than the if(!*host){} code that exists a few lines down.. afaict they should both be run for the "" input right?

is there something wrong with the code that resets things in that path ("remove entire authority")

@@ +1522,5 @@
>  
>      // Now canonicalize the host to lowercase
>      net_ToLowerCase(mSpec.BeginWriting() + mHost.mPos, mHost.mLen);
>  
> +    printf("Spec: %s | mHost[%u %d] | mUser[%u %d] \n",

accidentally left in patch

Comment 7

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

(In reply to Patrick McManus [:mcmanus] from comment #6)
> Comment on attachment 8401634 [details] [diff] [review]
> bug.patch
> 
> Review of attachment 8401634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/src/nsStandardURL.cpp
> @@ +1459,5 @@
> >  
> >      LOG(("nsStandardURL::SetHost [host=%s]\n", host));
> >  
> > +    if (flat.IsEmpty())
> > +        return NS_OK;
> 
> use {} to avoid goto fail
> 
> but the more interesting part is I need you to walk me through why this code
> is different than the if(!*host){} code that exists a few lines down..
> afaict they should both be run for the "" input right?
> 
> is there something wrong with the code that resets things in that path
> ("remove entire authority")

You're right, that code correctly removes the entire authority, and I'm still not sure that's something we should always do.

However, the problem is setting a host when there is a username but no host.

When url.hostname = "www.mozilla.org"; calls SetHost(input)

 spec      =
http://tttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@/
 port      = -1
 scheme    = (0,4)
 authority = (7,65)
 username  = (7,64)
 password  = (72,-1)
 hostname  = (72,-1)
 path      = (72,1)
 filepath  = (72,1)
 directory = (72,1)
 basename  = (73,0)
 extension = (73,-1)
 query     = (72,-1)
 ref       = (72,-1)


The code at http://hg.mozilla.org/mozilla-central/annotate/5fa70bd90a8b/netwerk/base/src/nsStandardURL.cpp#l1509

if (mHost.mLen < 0) { // assumes there is no username or password
    mHost.mPos = mAuthority.mPos; // = 7
    mHost.mLen = 0; // = 0
}

int32_t shift = ReplaceSegment(mHost.mPos, mHost.mLen, host, len); // inserts the hostname before the username

if (shift) {
    mHost.mLen = len;
    mAuthority.mLen += shift;
    ShiftFromPath(shift);
}

So at the end of SetHost(input) it ends up being:

spec      = http://www.mozilla.orgtttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt@/
 port      = -1
 scheme    = (0,4)
 authority = (7,80)
 username  = (7,64)
 password  = (72,-1)
 hostname  = (7,15)
 path      = (87,1)
 filepath  = (87,1)
 directory = (87,1)
 basename  = (88,0)
 extension = (88,-1)
 query     = (87,-1)
 ref       = (87,-1)

Because of the overlap, in SetUserPass()
http://hg.mozilla.org/mozilla-central/annotate/5fa70bd90a8b/netwerk/base/src/nsStandardURL.cpp#l1243

ShiftFromHost(-mUsername.mLen); //does mHost.mPos += -mUsername.mLen where mUsername.mLen = 65
// mHost underflows and becomes a wrong value 

Indeed, the first patch did not fix the underlying problem, in case you managed to create an URL with an empty host.

Comment 8

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

Attachment: bug.patch

Comment 9

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

Attachment: port_parsing.patch

I really think we should reconsider if we want to allow setting an empty host. The logic for setting a port also assumes there is always a host. Attaching fix for this second issue.

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu [:valentin] from comment #9)

> I really think we should reconsider if we want to allow setting an empty
> host. The logic for setting a port also assumes there is always a host.
> Attaching fix for this second issue.

not really something I want to make the call on. Feel free to open a new unrestricted bug and ask annevk for direction

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8402202 [details] [diff] [review]
bug.patch

let's go with the port parsing patch instead of this one

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8402284 [details] [diff] [review]
port_parsing.patch

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

I think we can go down this road if we add a significant xpcshell test with the appropriate variations included.. make sense?

Comment 13

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

Attachment: bug.patch

Fixed build issue on windows.

Comment 14

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

Attachment: test_standardurl.patch

These tests seem to cover the edge cases.

Comment 15

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

Comment on attachment 8402971 [details] [diff] [review]
bug.patch

https://tbpl.mozilla.org/?tree=Try&rev=54a40c33e490

Comment 16

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

Comment on attachment 8402971 [details] [diff] [review]
bug.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
A careful look at the patch can reveal that there was a parsing problem when the host was empty. Potentially, one could manipulate the value of mHost.mPos in order to write to arbitrary memory locations.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not this patch, but the tests in attachment 8402973 [details] [diff] [review] include the use case which caused the crash, and several others which might reveal the underlying problem.

Which older supported branches are affected by this flaw?
All branches are affected by this problem. Based on the revision history of the file, this issue was likely present in rev@1

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch can be applied to ESR24 with no conflicts. This file rarely changes.

How likely is this patch to cause regressions; how much testing does it need?
Low risk. We have good test coverage for the URL bits.

Comment 17

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

Comment on attachment 8402971 [details] [diff] [review]
bug.patch

sec-approval+ for trunk. Please create and nominate Aurora, Beta, and ESR24 patches as soon as it lands and goes green on trunk.

Comment 18

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

Ryan, could you check-in bug.patch for me? I don't have Level 3 access.
Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e20983ae82d

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e20983ae82d

Please request Aurora/Beta/ESR24 approval on this when you get a chance :)

Comment 21

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

Comment on attachment 8402971 [details] [diff] [review]
bug.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): preexisting issue
User impact if declined: security vulnerabilty
Testing completed: on m-c
Risk to taking this patch (and alternatives if risky): low.
String or IDL/UUID changes made by this patch: none.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c50f6cbac4a2
https://hg.mozilla.org/releases/mozilla-beta/rev/1be8ef9bf661
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/db3a01dbe27b
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ffd775bb82a4
https://hg.mozilla.org/releases/mozilla-esr24/rev/4fe403b52dbd

Comment 23

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

Thanks Ryan.

Comment 24

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash on a old Nightly build and verified as fixed on Firefox 29 beta 8, latest Nightly and latest Aurora.