Closed
Bug 991471
Opened 10 years ago
Closed 10 years ago
nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer
Categories
(Core :: Networking, defect)
Tracking
()
VERIFIED
FIXED
mozilla31
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)
394 bytes,
text/html
|
Details | |
12.28 KB,
text/plain
|
Details | |
10.90 KB,
text/plain
|
Details | |
2.78 KB,
patch
|
mcmanus
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr24+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8401634 -
Attachment is obsolete: true
Attachment #8402202 -
Flags: review?(mcmanus)
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
Fixed build issue on windows.
Attachment #8402202 -
Attachment is obsolete: true
Attachment #8402284 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
These tests seem to cover the edge cases.
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8402971 [details] [diff] [review] bug.patch https://tbpl.mozilla.org/?tree=Try&rev=54a40c33e490
Attachment #8402971 -
Flags: review?(mcmanus)
Updated•10 years ago
|
Attachment #8402971 -
Flags: review?(mcmanus) → review+
Updated•10 years ago
|
Attachment #8402973 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
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?
Updated•10 years ago
|
status-b2g18:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-firefox28:
--- → wontfix
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox-esr24:
--- → affected
tracking-b2g-v1.2:
--- → ?
tracking-b2g-v1.3:
--- → ?
tracking-firefox29:
--- → +
tracking-firefox30:
--- → +
tracking-firefox31:
--- → +
tracking-firefox-esr24:
--- → +
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
Ryan, could you check-in bug.patch for me? I don't have Level 3 access. Thanks!
Flags: needinfo?(ryanvm)
Updated•10 years ago
|
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e20983ae82d
Keywords: checkin-needed
Comment 20•10 years ago
|
||
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
Assignee | ||
Comment 21•10 years ago
|
||
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)
Updated•10 years ago
|
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+
Updated•10 years ago
|
Comment 22•10 years ago
|
||
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
Assignee | ||
Comment 23•10 years ago
|
||
Thanks Ryan.
Updated•10 years ago
|
Comment 24•10 years ago
|
||
Reproduced the initial crash on a old Nightly build and verified as fixed on Firefox 29 beta 8, latest Nightly and latest Aurora.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Updated•10 years ago
|
Whiteboard: [adv-main29+][adv-esr24.5+]
Reporter | ||
Updated•10 years ago
|
Blocks: fuzz-urlutils
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•