Closed Bug 991471 Opened 10 years ago Closed 10 years ago

nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer

Categories

(Core :: Networking, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + verified
firefox30 + verified
firefox31 + verified
firefox-esr24 29+ fixed
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 ? fixed
b2g-v1.3 ? fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jruderman, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-main29+][adv-esr24.5+])

Crash Data

Attachments

(5 files, 3 obsolete files)

Attached file stack (lldb)
Attached file stack (ASan)
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.
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?
Assignee: nobody → valentin.gosu
Attached patch bug.patch (obsolete) — Splinter Review
This seems to fix the crash, and it has the same behaviour as Chromium (nothing happens when setting the host to "" ).
Attachment #8401634 - Flags: review?(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
Attachment #8401634 - Flags: review?(mcmanus)
(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.
Attached patch bug.patch (obsolete) — Splinter Review
Attachment #8401634 - Attachment is obsolete: true
Attachment #8402202 - Flags: review?(mcmanus)
Attached patch port_parsing.patch (obsolete) — Splinter Review
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.
(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 on attachment 8402202 [details] [diff] [review]
bug.patch

let's go with the port parsing patch instead of this one
Attachment #8402202 - Flags: review?(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?
Attachment #8402284 - Flags: feedback+
Attached patch bug.patchSplinter Review
Fixed build issue on windows.
Attachment #8402202 - Attachment is obsolete: true
Attachment #8402284 - Attachment is obsolete: true
These tests seem to cover the edge cases.
Attachment #8402971 - Flags: review?(mcmanus) → review+
Attachment #8402973 - Flags: review+
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.
Attachment #8402971 - Flags: sec-approval?
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.
Attachment #8402971 - Flags: sec-approval? → sec-approval+
Ryan, could you check-in bug.patch for me? I don't have Level 3 access.
Thanks!
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8e20983ae82d

Please request Aurora/Beta/ESR24 approval on this when you get a chance :)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(valentin.gosu)
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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.
Attachment #8402971 - Flags: approval-mozilla-esr24?
Attachment #8402971 - Flags: approval-mozilla-beta?
Attachment #8402971 - Flags: approval-mozilla-aurora?
Flags: needinfo?(valentin.gosu)
Attachment #8402971 - Flags: approval-mozilla-esr24?
Attachment #8402971 - Flags: approval-mozilla-esr24+
Attachment #8402971 - Flags: approval-mozilla-beta?
Attachment #8402971 - Flags: approval-mozilla-beta+
Attachment #8402971 - Flags: approval-mozilla-aurora?
Attachment #8402971 - Flags: approval-mozilla-aurora+
Thanks Ryan.
Keywords: verifyme
Reproduced the initial crash on a old Nightly build and verified as fixed on Firefox 29 beta 8, latest Nightly and latest Aurora.
Whiteboard: [adv-main29+][adv-esr24.5+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.