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)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox30 --- wontfix
firefox31 --- verified
firefox32 + verified
firefox33 --- verified
firefox-esr24 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jruderman, Assigned: baku)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main31+])

Attachments

(6 files, 4 obsolete files)

Same symptoms as bug 991471 and bug 1005578, different testcase.
Attached file stack
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: nobody → amarchesini
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
Very probable, yes.  In general, none of the nsIURI stuff expects to be directly exposed to web pages... and until URL it wasn't.
The validation should probably be done in nsStandardURL::ValidIPv6orHostname.
I can do it if that's ok.
Attached patch bug.patch (obsolete) — Splinter Review
Attachment #8434155 - Flags: review?(bzbarsky)
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)
> 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)
Attachment #8434155 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
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.
Flags: needinfo?(annevk)
Attached patch bug.patch (obsolete) — Splinter Review
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)
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)
(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)
Attached patch bug.patch (obsolete) — Splinter Review
Attachment #8434195 - Attachment is obsolete: true
Attachment #8434195 - Flags: review?(jduell.mcbugs)
Attachment #8434263 - Flags: review?(jduell.mcbugs)
How far back does this bug go (FF version)?
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+
Attached patch bug.patchSplinter Review
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)
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 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 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+
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
Flags: needinfo?(amarchesini)
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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 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+
Attached patch beta (obsolete) — Splinter Review
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)
(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 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+
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.
Attached patch id.patchSplinter Review
Interdiff for the beta.
Attachment #8442434 - Flags: review?(valentin.gosu)
Attached patch betaSplinter Review
it should be OK, but if you can take another glance.
Attachment #8441961 - Attachment is obsolete: true
Attachment #8442435 - Flags: review?(valentin.gosu)
Attachment #8442434 - Flags: review?(valentin.gosu) → review+
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?
Attachment #8442435 - Flags: review?(valentin.gosu) → review+
Attachment #8442435 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
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.
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.
Let's modify the tests. Thanks!
Flags: needinfo?(amarchesini)
(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)
As the original test author, it would be good if you did :)
Flags: needinfo?(ryanvm)
RyanMV, should you take care of this patch when/if green on try? Thanks.
Flags: needinfo?(ryanvm)
Sure thing :)
Flags: needinfo?(ryanvm)
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/
Whiteboard: [adv-main31+]
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-
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.