nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer, #3

VERIFIED FIXED in Firefox 31, Firefox OS v1.3

Status

()

Core
Networking
--
critical
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: baku)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla33
x86_64
Mac OS X
crash, qawanted, sec-critical, testcase, verifyme
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(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)

Details

(Whiteboard: [adv-main31+])

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8433759 [details]
testcase (crashes Firefox)

Same symptoms as bug 991471 and bug 1005578, different testcase.
(Reporter)

Comment 1

4 years ago
Created attachment 8433761 [details]
stack
(Assignee)

Comment 2

4 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

4 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8434155 [details] [diff] [review]
bug.patch
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)
(Assignee)

Comment 7

4 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

4 years ago
Attachment #8434155 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)

Comment 8

4 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

4 years ago
Flags: needinfo?(annevk)
(Assignee)

Comment 9

4 years ago
Created attachment 8434195 [details] [diff] [review]
bug.patch

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)
(Assignee)

Comment 11

4 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

4 years ago
Created attachment 8434263 [details] [diff] [review]
bug.patch
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)?
status-firefox32: --- → affected
tracking-firefox32: --- → +
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

4 years ago
Created attachment 8436922 [details] [diff] [review]
bug.patch

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
Last Resolved: 4 years ago
status-firefox33: --- → fixed
Flags: needinfo?(amarchesini)
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 22

4 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 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

4 years ago
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.
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.
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
status-firefox32: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 28

4 years ago
Created attachment 8442434 [details] [diff] [review]
id.patch

Interdiff for the beta.
Attachment #8442434 - Flags: review?(valentin.gosu)
(Assignee)

Comment 29

4 years ago
Created attachment 8442435 [details] [diff] [review]
beta

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+
(Assignee)

Comment 30

4 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?
Attachment #8442435 - Flags: review?(valentin.gosu) → review+
Attachment #8442435 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: qawanted, verifyme
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.
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
status-b2g-v1.3: fixed → affected
status-b2g-v1.4: fixed → affected
Flags: needinfo?(amarchesini)
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)
(Assignee)

Comment 38

4 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)
As the original test author, it would be good if you did :)
Flags: needinfo?(ryanvm)
(Assignee)

Comment 41

4 years ago
RyanMV, should you take care of this patch when/if green on try? Thanks.
Flags: needinfo?(ryanvm)
Sure thing :)
Flags: needinfo?(ryanvm)
status-b2g-v1.3T: affected → fixed
status-firefox-esr24: --- → unaffected
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
status-firefox31: fixed → verified
status-firefox32: fixed → verified
status-firefox33: fixed → verified
status-firefox30: affected → wontfix
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-

Updated

3 years ago
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.