Closed
Bug 1020041
Opened 10 years ago
Closed 10 years ago
nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer, #3
Categories
(Core :: Networking, defect)
Tracking
()
VERIFIED
FIXED
mozilla33
People
(Reporter: jruderman, Assigned: baku)
References
(Blocks 1 open bug)
Details
(5 keywords, Whiteboard: [adv-main31+])
Attachments
(6 files, 4 obsolete files)
285 bytes,
text/html
|
Details | |
10.79 KB,
text/plain
|
Details | |
8.91 KB,
patch
|
valentin
:
review+
valentin
:
feedback+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.57 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
8.84 KB,
patch
|
valentin
:
review+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
8.83 KB,
patch
|
Details | Diff | Splinter Review |
Same symptoms as bug 991471 and bug 1005578, different testcase.
Reporter | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
I think the main problem is that SetHost does not validate the hostname. When we set the hostname to '?' and then the pathname to 'j', nsStandardURL parses the new URL and considers '?' as the beginning of the query string. var url = new URL("http://foo.bar/j"); url.host = '?'; url.pathname = 'j' alert(url.host) // expected '?' but it shows '' alert(url.searchParam) // expected '', it shows '?/j'
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → amarchesini
Flags: needinfo?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Comment 3•10 years ago
|
||
Very probable, yes. In general, none of the nsIURI stuff expects to be directly exposed to web pages... and until URL it wasn't.
Comment 4•10 years ago
|
||
The validation should probably be done in nsStandardURL::ValidIPv6orHostname. I can do it if that's ok.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8434155 -
Flags: review?(bzbarsky)
Comment 6•10 years ago
|
||
Comment on attachment 8434155 [details] [diff] [review] bug.patch 1) I'm not a necko peer, so you'll need someone else to review the nsStandardURL change. 2) What does the spec say to do here? I thought we tried pretty hard to not have any of these setters ever throw on HTMLAnchorElement (no-opping instead of throwing, basically), but what ended up in the spec?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 7•10 years ago
|
||
> 2) What does the spec say to do here? I thought we tried pretty hard to > not have any of these setters ever throw on HTMLAnchorElement (no-opping > instead of throwing, basically), but what ended up in the spec? http://url.spec.whatwg.org/#host-state and http://url.spec.whatwg.org/#concept-host-parser talk about 'return failure'. Annevk?
Flags: needinfo?(amarchesini) → needinfo?(annevk)
Assignee | ||
Updated•10 years ago
|
Attachment #8434155 -
Flags: review?(bzbarsky) → review?(jduell.mcbugs)
Comment 8•10 years ago
|
||
If you set .host to "?" buffer will be the empty string when the host parser is invoked on it. That returns failure because it does not support empty host names. That means the URL remains unchanged and no exceptions are thrown.
Updated•10 years ago
|
Flags: needinfo?(annevk)
Assignee | ||
Comment 9•10 years ago
|
||
No exceptions are thrown with this patch.
Attachment #8434155 -
Attachment is obsolete: true
Attachment #8434155 -
Flags: review?(jduell.mcbugs)
Attachment #8434195 -
Flags: review?(jduell.mcbugs)
Comment 10•10 years ago
|
||
I don't think the check in nsStandardURL is correct. First of all SetHostPort isn't even called when doing url.hostname = "?" Maybe moving it to ValidIPv6orHostname would be a better idea?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #10) > I don't think the check in nsStandardURL is correct. First of all > SetHostPort isn't even called when doing url.hostname = "?" > Maybe moving it to ValidIPv6orHostname would be a better idea? if the hostname is: 'some.thing?' this is not "invalid" for the URL API but just "some.thing" must be used as hostname. This is the reason why ValidIPv6orHostname is not the right place for this check. I'm adding a similar check in SetHost.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8434195 -
Attachment is obsolete: true
Attachment #8434195 -
Flags: review?(jduell.mcbugs)
Attachment #8434263 -
Flags: review?(jduell.mcbugs)
Comment 13•10 years ago
|
||
How far back does this bug go (FF version)?
status-firefox32:
--- → affected
tracking-firefox32:
--- → +
Comment 14•10 years ago
|
||
Comment on attachment 8434263 [details] [diff] [review] bug.patch Review of attachment 8434263 [details] [diff] [review]: ----------------------------------------------------------------- > I'm not a necko peer, so you'll need someone else to review bz: consider yourself delegated to review URI stuff (and really, most other necko stuff) whenever it's expedient. You actually know it better than anyone else. That said I know you're busy. The patch looks fine to me, but this is one of the first times I've looked at this code. So I'd like Valentin, who's been fixing this code recently, to also review. ::: netwerk/base/src/nsStandardURL.cpp @@ +1509,5 @@ > + nsACString::const_iterator start, end; > + hostname.BeginReading(start); > + hostname.EndReading(end); > + > + const char limit[] = { '/', '\\', '?', '#', 0 }; It would be nice to have some of this code--at least the list of chars?--in a single function or macro, etc. Not a deal-breaker, though--I can live with it, it's not that much code duplication.
Attachment #8434263 -
Flags: review?(valentin.gosu)
Attachment #8434263 -
Flags: review?(jduell.mcbugs)
Attachment #8434263 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
I moved the check in a method called FindHostLimit.
Attachment #8434263 -
Attachment is obsolete: true
Attachment #8434263 -
Flags: review?(valentin.gosu)
Attachment #8436922 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 16•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7f21e1564560 green on try
Comment 17•10 years ago
|
||
Comment on attachment 8436922 [details] [diff] [review] bug.patch r? :mcmanus, since I am not a Necko peer. But it all looks good to me.
Attachment #8436922 -
Flags: review?(valentin.gosu)
Attachment #8436922 -
Flags: review?(mcmanus)
Attachment #8436922 -
Flags: feedback+
Comment 18•10 years ago
|
||
Comment on attachment 8436922 [details] [diff] [review] bug.patch Review of attachment 8436922 [details] [diff] [review]: ----------------------------------------------------------------- I'll delegate the review to valentin
Attachment #8436922 -
Flags: review?(mcmanus) → review?(valentin.gosu)
Comment 19•10 years ago
|
||
Comment on attachment 8436922 [details] [diff] [review] bug.patch Review of attachment 8436922 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good. r=me with the following change ::: netwerk/base/src/nsStandardURL.cpp @@ +1464,5 @@ > // if the first char isn't [ then there should be no ] character > return NS_ERROR_MALFORMED_URI; > } > + > + FindHostLimit(start, end); The ipv6 case bypasses this check. Move the check right before << if (*start == '[') { >> and add another test for { host: '[2001::1]#bla:10', expected: '[2001::1]'}
Attachment #8436922 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/412a7f011ebb
https://hg.mozilla.org/mozilla-central/rev/412a7f011ebb Does this need uplifted to Aurora now that the trains moved?
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox33:
--- → fixed
Flags: needinfo?(amarchesini)
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8436922 [details] [diff] [review] bug.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 887364 User impact if declined: a crash Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): This patch touches nsStnadardURL that is a important component in gecko, but it has been reviewed by 2 necko peers and the change is not so complex. String or IDL/UUID changes made by this patch: none
Attachment #8436922 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
Comment 23•10 years ago
|
||
Comment on attachment 8436922 [details] [diff] [review] bug.patch Aurora approval granted. Given that this is sec-critical and, according to the referenced bug, introduced in Firefox 26, I think we need to consider taking this on beta as well. If you agree, please request beta approval and ensure that the patch applies cleanly on beta as well.
Attachment #8436922 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 24•10 years ago
|
||
This patch is fore beta. Changes: 1. FindHostLimit is not used in SetHostPort because this calls SetHost. 2. The test test_url_malformedHost.html doesn't parse [2001::1] and this check has been removed.
Attachment #8441961 -
Flags: review?(valentin.gosu)
Comment 25•10 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #24) > Created attachment 8441961 [details] [diff] [review] > beta > > This patch is fore beta. Changes: > > 1. FindHostLimit is not used in SetHostPort because this calls SetHost. > 2. The test test_url_malformedHost.html doesn't parse [2001::1] and this > check has been removed. Hi Andrea, Please keep the call in SetHostPort and change the test to { host: 'abcd#bla:10', expected: 'abcd'} According to the spec, the parsing should not move past the # marker.
Comment 26•10 years ago
|
||
Comment on attachment 8441961 [details] [diff] [review] beta Review of attachment 8441961 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the issues noted in comment 25.
Attachment #8441961 -
Flags: review?(valentin.gosu) → review+
Comment 27•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b110a17ac17b (In reply to Lawrence Mandel [:lmandel] from comment #23) > Given that this is sec-critical and, according to the referenced bug, > introduced in Firefox 26, I think we need to consider taking this on beta as > well. If you agree, please request beta approval and ensure that the patch > applies cleanly on beta as well. Taking that one step further, this should have had security approval before ever landing in the first place. And probably not with a test. And we may want to consider this as a ride-along in the event of an Fx30 chemspill release.
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox30:
--- → affected
status-firefox31:
--- → affected
Flags: in-testsuite+
Assignee | ||
Comment 28•10 years ago
|
||
Interdiff for the beta.
Attachment #8442434 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 29•10 years ago
|
||
it should be OK, but if you can take another glance.
Attachment #8441961 -
Attachment is obsolete: true
Attachment #8442435 -
Flags: review?(valentin.gosu)
Updated•10 years ago
|
Attachment #8442434 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 30•10 years ago
|
||
Comment on attachment 8442435 [details] [diff] [review] beta [Approval Request Comment] Bug caused by (feature/regressing bug #): 887364 User impact if declined: a crash Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): This patch touches nsStnadardURL that is a important component in gecko, but it has been reviewed by 2 necko peers and the change is not so complex. String or IDL/UUID changes made by this patch: none
Attachment #8442435 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Attachment #8442435 -
Flags: review?(valentin.gosu) → review+
Updated•10 years ago
|
Attachment #8442435 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•10 years ago
|
Comment 32•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c2e0f245d909 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/3373cd35d4c7
Comment 33•10 years ago
|
||
Comment on attachment 8442435 [details] [diff] [review] beta We should consider this for an Fx30 chemspill release if we end up making one given that it's a sec-crit that landed w/o sec-approval and with tests no less.
Attachment #8442435 -
Flags: approval-mozilla-release?
Comment 34•10 years ago
|
||
Will leave the nom so it's on our radar, but this is a long-time issue and not a serious driver for a chemspill. We're still continuing to collect data and will look at this as a ridealong should the need arise.
Comment 35•10 years ago
|
||
Backed out from b2g28 and b2g30 for test failures. Andrea, any idea what the problem is? https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4e9d3d4412f9 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/295033efc42e https://tbpl.mozilla.org/php/getParsedLog.php?id=42301777&tree=Mozilla-B2g30-v1.4
Comment 36•10 years ago
|
||
I think the problem is before Bug 996055 you were allowed to set an empty hostname. We could either uplift that one too, or change the tests to take this issue into account.
Assignee | ||
Comment 38•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37) > Let's modify the tests. Thanks! Should I modify the test? or somebody else is doing it?
Flags: needinfo?(ryanvm)
Comment 39•10 years ago
|
||
As the original test author, it would be good if you did :)
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 40•10 years ago
|
||
Test updated: https://tbpl.mozilla.org/?tree=Try&rev=8c2f215517c4
Assignee | ||
Comment 41•10 years ago
|
||
RyanMV, should you take care of this patch when/if green on try? Thanks.
Flags: needinfo?(ryanvm)
Comment 43•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/da45cc741860 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8405b3819ac8
Updated•10 years ago
|
Updated•10 years ago
|
status-firefox-esr24:
--- → unaffected
Comment 44•10 years ago
|
||
Reproduced the original issue from comment #0 using the following build: (crashed as soon as the poc was opened) * http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/06/2014-06-04-03-02-02-mozilla-central/ Went through verification using the following builds: * fx-33.0a1: ** OSX 10.9.4 x64: [Passed] ** Ubuntu 13.10 x64: [Passed] ** Win 8.1 x64: [Passed] ** Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-08-03-02-03-mozilla-central/ * fx-32.0a2: ** OSX 10.9.4 x64: [Passed] ** Ubuntu 13.10 x64: [Passed] ** Win 8.1 x64: [Passed] ** Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-08-00-40-01-mozilla-aurora/ * fx-31.0b8: ** OSX 10.9.4 x64: [Passed] ** Ubuntu 13.10 x64: [Passed] ** Win 8.1 x64: [Passed] ** Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0b8/
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
Whiteboard: [adv-main31+]
Comment 45•10 years ago
|
||
Comment on attachment 8442435 [details] [diff] [review] beta 31 is going to ship next week. So, not taking the patch in release.
Attachment #8442435 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•