1141959 - (CVE-2020-12390) nsIPrincipal.origin mishandles IPv6 host literals

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Giorgio Maone [:ma1]]

For codebase URIs with IPv6 host literals like

http://[::1]/

nsPrincipal::GetOriginForURI() returns

http:/::1/

because it just concatenates nsIURI.scheme + "://" + nsIURI.asciiHost + ":" + nsIURI.port, "forgetting" to special case IPv6 hosts enclosing them with brackets.

This gets especially confusing and likely unsafe when explicit ports are added to the mix, potentially producing invalid URIs and/or duplicate strings for actually different origins.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This should probably use NS_GenerateHostPort instead of doing the the manual AppendInt thing...

That said, is there a reason this can't just use nsContentUtils::GetAsciiOrigin instead of duplicating code?  Presumably after we do the isChrome check.

Comment 2

[Andrew McCreight [:mccr8]]

It looks like this method is only used for app-y stuff.

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

CritSmash would like to rate this, but we need to understand where this code is used. Is this a URI parsing method that lives in a general location and is used for IPv6 hosts everywhere? Or is it just used - as Andrew asks - for something like apps or the like?

Comment 4

[Giorgio Maone [:ma1]]

It lives in nsPrincipal, which is a "general location" mostly used for security checks.

Direct callers:
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsPrincipal%3A%3AGetOriginForURI%28class+nsIURI+*%2C+char+**%29%22

Callers of the "interesting" client nsScriptSecurityManager::CheckLoadURIWithPrincipal():
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsScriptSecurityManager%3A%3ACheckLoadURIWithPrincipal%28class+nsIPrincipal+*%2C+class+nsIURI+*%2C+uint32_t%29%22

Comment 5

[Daniel Veditz [:dveditz]]

I'm calling this moderate because we don't know of a place that will misuse this and end up with a security problem, but it's quite plausible that something's going to get into trouble.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

We handle this correctly, I'll upload a test to make sure we don't regress that.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1141959: Test nsIPrincipal.origin treats IPv6 host correctly.r=tritter

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Dragana,

Tom pointed out that in the test (see attachment) prinicpal.host is not canonicalized, e.g.
"http://[20D1:0000:3238:DFE1:63:0000:0000:FEFB]/"
"http://[20D1:0:3238:DFE1:63::FEFB]/"

are the exact same, which implies that storage from one could be accessed by the other.

I tried to trace down the problem, and I think the problem happens within the creation of an nsIURI. Anyway, all the code within nsIURI seems critical. Is there even an easy fix to account for canonicalized IPV6 addresses?

Comment 9

[Dragana Damjanovic [:dragana]]

Valentin, can you help Christoph here?

Comment 10

[Valentin Gosu [:valentin] (he/him)]

I'm not sure I understand. The URL parser turns the IPv6 into canonicalized versions, and principal.origin reflects the same canonical form.
Since "http://[20D1:0000:3238:DFE1:63:0000:0000:FEFB]/" always gets parsed as "http://[20d1:0:3238:dfe1:63::fefb]/", that shouldn't really be a problem. Or is there something else I'm missing?

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

That may the answer then. "The Principal object will construct a principal based on the url it is given; if the url is not canonicalized, it won't be a canonicalized principal. But there should be no way to construct a principal with a non-canonicalized url, so that behavior is okay."

In which case we ought to be able to assert in Principal if we receive a non-canonicalized url.

I'd propose sending a try run with a moz_assert, and then land a diagnostic assert behind a pref (on by default, but this enables people to turn the assert off), and monitor crashes and be prepared to backout. If Nightly is clean w.r.t this we could make it a release assert.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/00aeb92fee68696ef67a4b891dc1711b2068d237

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/00aeb92fee68

Comment 14

[Ciprian Georgiu, Desktop QA]

As per my discussion on slack with Christoph, there's nothing actionable here, in terms on manual testing. Thanks!

I'll remove the qe+ flag.

Comment 15

[Frederik Braun [:freddy]]

Attachment: advisory.txt