1548306 - (CVE-2019-11717) Caret character (^) not escaped for unsuffixed origins

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with: m-c 20190430-90234f4c094d

Assertion failure: false (NS_SUCCEEDED(GetPrincipalFromOrigin(aOrigin, getter_AddRefs(dbgPrincipal)))), at src/extensions/cookie/nsPermissionManager.cpp:3297

rax = 0x000055f5f6e03e40   rdx = 0x0000000000000000
rcx = 0x00007fdedd9638fd   rbx = 0x00007fffe36394f0
rsi = 0x00007fdee8cc28b0   rdi = 0x00007fdee8cc1680
rbp = 0x00007fffe3639600   rsp = 0x00007fffe36394a0
r8 = 0x00007fdee8cc28b0    r9 = 0x00007fdee9e44740
r10 = 0x0000000000000000   r11 = 0x0000000000000000
r12 = 0x00007fffe36394c8   r13 = 0x00007fffe36394b8
r14 = 0x00000000804b000a   r15 = 0x00007fdec692c580
rip = 0x00007fded88ced64
OS|Linux|0.0.0 Linux 4.19.13-coreos #1 SMP Mon Jan 7 23:51:04 -00 2019 x86_64
CPU|amd64|family 6 model 79 stepping 1|8
GPU|||
Crash|SIGSEGV|0x0|0
0|0|libxul.so|nsPermissionManager::GetKeyForOrigin(nsTSubstring<char> const&, nsTSubstring<char>&)|hg:hg.mozilla.org/mozilla-central:extensions/cookie/nsPermissionManager.cpp:90234f4c094dcc794df28fdd464793dfe065f943|3299|0x74
0|1|libxul.so|nsPermissionManager::GetKeyForPrincipal(nsIPrincipal*, nsTSubstring<char>&)|hg:hg.mozilla.org/mozilla-central:extensions/cookie/nsPermissionManager.cpp:90234f4c094dcc794df28fdd464793dfe065f943|3323|0xb
0|2|libxul.so|nsPermissionManager::GetAllKeysForPrincipal(nsIPrincipal*)|hg:hg.mozilla.org/mozilla-central:extensions/cookie/nsPermissionManager.cpp:90234f4c094dcc794df28fdd464793dfe065f943|3349|0xc
0|3|libxul.so|mozilla::dom::ContentParent::TransmitPermissionsForPrincipal(nsIPrincipal*)|hg:hg.mozilla.org/mozilla-central:dom/ipc/ContentParent.cpp:90234f4c094dcc794df28fdd464793dfe065f943|5377|0x5
0|4|libxul.so|mozilla::dom::ContentParent::AboutToLoadHttpFtpDocumentForChild(nsIChannel*)|hg:hg.mozilla.org/mozilla-central:dom/ipc/ContentParent.cpp:90234f4c094dcc794df28fdd464793dfe065f943|5343|0xd
0|5|libxul.so|mozilla::net::HttpChannelParent::OnStartRequest(nsIRequest*)|hg:hg.mozilla.org/mozilla-central:netwerk/protocol/http/HttpChannelParent.cpp:90234f4c094dcc794df28fdd464793dfe065f943|1354|0x18
0|6|libxul.so|mozilla::net::HttpBaseChannel::DoNotifyListener()|hg:hg.mozilla.org/mozilla-central:netwerk/protocol/http/HttpBaseChannel.cpp:90234f4c094dcc794df28fdd464793dfe065f943|3262|0x14
0|7|libxul.so|mozilla::net::HttpAsyncAborter<mozilla::net::nsHttpChannel>::HandleAsyncAbort()|hg:hg.mozilla.org/mozilla-central:netwerk/protocol/http/HttpBaseChannel.h:90234f4c094dcc794df28fdd464793dfe065f943|878|0x5
0|8|libxul.so|mozilla::detail::RunnableMethodImpl<mozilla::net::nsHttpChannel*, void (mozilla::net::nsHttpChannel::*)(), true, (mozilla::RunnableKind)0>::Run()|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.h:90234f4c094dcc794df28fdd464793dfe065f943|1174|0x13
0|9|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:90234f4c094dcc794df28fdd464793dfe065f943|1180|0x15
0|10|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:90234f4c094dcc794df28fdd464793dfe065f943|486|0x11
0|11|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:90234f4c094dcc794df28fdd464793dfe065f943|88|0xa
0|12|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:90234f4c094dcc794df28fdd464793dfe065f943|315|0x17
0|13|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:90234f4c094dcc794df28fdd464793dfe065f943|290|0x8
0|14|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:90234f4c094dcc794df28fdd464793dfe065f943|137|0xd
0|15|libxul.so|nsAppStartup::Run()|hg:hg.mozilla.org/mozilla-central:toolkit/components/startup/nsAppStartup.cpp:90234f4c094dcc794df28fdd464793dfe065f943|276|0xe
0|16|libxul.so|XREMain::XRE_mainRun()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:90234f4c094dcc794df28fdd464793dfe065f943|4554|0x11
0|17|libxul.so|XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:90234f4c094dcc794df28fdd464793dfe065f943|4692|0x8
0|18|libxul.so|XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:90234f4c094dcc794df28fdd464793dfe065f943|4773|0x5
0|19|firefox-bin|do_main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:90234f4c094dcc794df28fdd464793dfe065f943|212|0x22
0|20|firefox-bin|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:90234f4c094dcc794df28fdd464793dfe065f943|291|0xd
0|21|libc-2.27.so||||0x21b97
0|22|firefox-bin|MOZ_ReportCrash|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:90234f4c094dcc794df28fdd464793dfe065f943|184|0x5

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:johannh, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Johann Hofmann [:johannh]]

Ah, missed this one, thank you auto nag bot.

Didn't we have a similar bug recently around weird characters when creating the principal? I can't seem to find it, but I know Ehsan and Nika were involved in writing this code. Forwarding the needinfo...

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Ok, so this one is a bit weird. Marking as a sec bug just to be safe.

For backwards compatibility reasons, when the origin string was first given access to originAttributes, it was designed such that the trailing attributes block is optional. Namely, if no attributes are non-default, the origin will be written as it is in the spec. The separator character to distinguish between the core origin and originAttributes is ^, so an origin might look like https://twitter.com^userContextId=1, or like https://twitter.com if there were no origin attributes set.

This leads to the spoofing issue. If it was possible for a site to include a ^ in its bare origin string, that could cause issues with the origin logic, as it could be possible to imitate originAttribute from a non-attributed origin. It used to be that the separator used was !, but that was found to be spoofable in bug 1172080. That bug is also where the ^ character was chosen.

Normally, a ^ is disallowed inside of a URI, as it will always be escaped as %5E. For example:

> Services.io.newURI("https://google.com/^", null, null).spec
"https://google.com/%5E"

However, we appear to allow this character to appear directly within the host section:

> Services.io.newURI("https://google.com^", null, null).spec
"https://google.com^/"

This oversight means it's possible to spoof originAttributes currently by forming the URLs correctly.

Fortunately for us, it appears that hostnames aren't allowed to contain the ^ character at all. RFC 952 (https://tools.ietf.org/html/rfc952) restricts the characters allowed quite a bit:

   1. A "name" (Net, Host, Gateway, or Domain name) is a text string up
   to 24 characters drawn from the alphabet (A-Z), digits (0-9), minus
   sign (-), and period (.).  Note that periods are only allowed when
   they serve to delimit components of "domain style names". (See
   RFC-921, "Domain Name System Implementation Schedule", for
   background).  No blank or space characters are permitted as part of a
   name. No distinction is made between upper and lower case.  The first
   character must be an alpha character.  The last character must not be
   a minus sign or period.  A host which serves as a GATEWAY should have
   "-GATEWAY" or "-GW" as part of its name.  Hosts which do not serve as
   Internet gateways should not use "-GATEWAY" and "-GW" as part of
   their names. A host which is a TAC should have "-TAC" as the last
   part of its host name, if it is a DoD host.  Single character names
   or nicknames are not allowed.

This appears to be relaxed a bit in rfc 1123 to allow a number as the first character. (https://tools.ietf.org/html/rfc1123#page-13)

      The syntax of a legal Internet host name was specified in RFC-952
      [DNS:4].  One aspect of host name syntax is hereby changed: the
      restriction on the first character is relaxed to allow either a
      letter or a digit.  Host software MUST support this more liberal
      syntax.

This would suggest that ^ is not a valid character within a hostname. Unfortunately the whatwg URL standard doesn't have that restriction, limiting only the characters NULL, TAB, LF, CR, SPACE, #, %, /, :, ?, @, [, , and ]. (https://url.spec.whatwg.org/#forbidden-host-code-point)

For that reason, Firefox's parser allows the character. Interestingly, chromium doesn't allow it, raising an exception:

// Firefox
> new URL("https://google.com^")
URL { href: "https://google.com^/", origin: "https://google.com^", ... }

// Chromium
> new URL("https://google.com^")
Uncaught TypeError: Failed to construct 'URL': Invalid URL

I think the fix to this issue may be to fix the whatwg URL spec to limit what ASCII characters can appear in hostnames more, and bring our implementation more in line with chrome's.

ni? to :bholley as he was involved in the original decision to use ^, and might have more insight than I do.

Comment 4

[Bobby Holley (:bholley)]

(In reply to :Nika Layzell (ni? for response) from comment #3)

I think the fix to this issue may be to fix the whatwg URL spec to limit what ASCII characters can appear in hostnames more, and bring our implementation more in line with chrome's.

ni? to :bholley as he was involved in the original decision to use ^, and might have more insight than I do.

That sounds sensible to me. NI bz, and CCing a few others who may have insight here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Seems fine to me if other browsers already disallow it...

Comment 6

[Anne (:annevk)]

Disallowing ^ seems doable given that Chrome appears to indeed do that. Safari will likely accept, though they have said in the past that they don't like such changes. (It'd be interesting to know if other layers of the stack end up forbidding ^ or if you can connect to a server that uses it anyway, in particular if Safari has a problem with this.)

Note that the reason the URL Standard is somewhat "liberal" has two reasons:

1. Despite various definitions of hosts existing, browsers don't enforce them fully (in particular in other layers than the URL parser) and therefore servers end up relying on them. (E.g., a Google server has been known to use an underscore. Various servers use hosts that look like IDNA but are not, etc.)
2. For better or worse URLs can carry other hosts than those allowed by DNS and this is also supported by browsers to various degrees.

Comment 7

[(no longer active)]

FWIW in my testing, webkit (Epiphany on Linux) seems to not parse ^ as %5E in the path section either...

new URL("https://google.com^")
URL {href: "https://www.google.com/^", origin: "https://www.google.com", protocol: "https:", username: "", password: "", …} = $1

Comment 8

[Daniel Veditz [:dveditz]]

I'm guessing this will eventually lead to some "sec-high" level issues at some point if we don't fix it, but sec-moderate for now. We can't rely on DNS to outlaw bad characters, because bad people can write their own nameservers (e.g. note comment above about the google server with illegal '_' in the name).

Comment 9

[Johann Hofmann [:johannh]]

That seems like a good plan to me, thanks for looking into this!

Comment 10

[Anne (:annevk)]

So a potential problem here is that for non-special URLs, e.g., test://test^test/, no browser currently fails parsing. Not all end up treating test^test as the hostname, but Safari does (and the URL Standard requires it). We could make that continue to work however and hope we never create origins with hosts et al out of non-special URLs...

Comment 12

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

(In reply to Anne (:annevk) from comment #10)

So a potential problem here is that for non-special URLs, e.g., test://test^test/, no browser currently fails parsing. Not all end up treating test^test as the hostname, but Safari does (and the URL Standard requires it). We could make that continue to work however and hope we never create origins with hosts et al out of non-special URLs...

I think it may be OK to fix this for special URIs only, as those would be the only ones that the browser actually loads.

Comment 13

[Anne (:annevk)]

Short-to-medium term, yes, but long term who knows what new scheme might load resources.

Comment 14

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

Attachment: Bug 1548306 - Do not allow the ^ character to appear in the hostname r=mayhemer

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #12)

(In reply to Anne (:annevk) from comment #10)

So a potential problem here is that for non-special URLs, e.g., test://test^test/, no browser currently fails parsing. Not all end up treating test^test as the hostname, but Safari does (and the URL Standard requires it). We could make that continue to work however and hope we never create origins with hosts et al out of non-special URLs...

I think it may be OK to fix this for special URIs only, as those would be the only ones that the browser actually loads.

The assertion fails at nsPermissionManager.cpp - mozsearch because we are trying to build a uri from "http://", clearly invalid - built by OriginAttributes.PopulateFromOrigin as 'no suffix' from "http://^".

I think the problem is not to load something wrong, the problem is that you could possibly spoof our OA isolation logic just by creation of a URL being passed around, for which '^' falls in to the part we consider as origin. We cut at the first '^' occurrence, kinda unpredictably, creating randomly broken origins. So, I'd vote for disabling this for all scheme's origins (make it early and clearly fail); but the reason behind would only be because we use it to separate the OA string in origins, internally in Gecko...

Comment 16

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

https://hg.mozilla.org/integration/autoland/rev/91fa2984a24991fc23bb7f7482dc59ae7d58d58d

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/91fa2984a249

Comment 18

[Anne (:annevk)]

So for what schemes did we fix this in the end? There's only a test for http as far as I can tell. We also haven't really coordinated this standards-wise yet...

Comment 19

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

(In reply to Anne (:annevk) from comment #18)

So for what schemes did we fix this in the end? There's only a test for http as far as I can tell. We also haven't really coordinated this standards-wise yet...

This only applies for schemes that use nsStandardURL: http,https,ftp,ws,wss - special schemes basically.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. Not sure if this warrants ESR60 consideration or not, but feel free to nominate for that too if you feel strongly that we should take it there.

Comment 21

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

Comment on attachment 9068173 [details]
Bug 1548306 - Do not allow the ^ character to appear in the hostname r=mayhemer

Beta/Release Uplift Approval Request

• User impact if declined: Possible other security issues with URLs that contain ^ in the hostname. We use the character as a separator in originAttributes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This character is not allowed by other browsers in the hostname, so we should not see any web-compat regressions.

However, users that have saved such disallowed URLs may encounter issues such as bug 1401401.

• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: (In reply to Daniel Veditz [:dveditz] from comment #8)

I'm guessing this will eventually lead to some "sec-high" level issues at some point if we don't fix it, but sec-moderate for now. We can't rely on DNS to outlaw bad characters, because bad people can write their own nameservers (e.g. note comment above about the google server with illegal '_' in the name).

As Daniel says, this is a just in case fix. I think it's worth uplifting in case someone figures out a way to exploit the bug.

• User impact if declined: Possible other security issues with URLs that contain ^ in the hostname. We use the character as a separator in originAttributes.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 22

[Anne (:annevk)]

I created https://github.com/whatwg/url/security/advisories/GHSA-7f42-348j-53xw to track this standards-wise. Let me know if you need access.

Comment 23

[Julien Cristau [:jcristau]]

Should we be uplifting bug 1401401 at the same time?

Comment 24

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

(In reply to Julien Cristau [:jcristau] from comment #23)

Should we be uplifting bug 1401401 at the same time?

We've had other breaking URL changes in the past, so I'm ambivalent about needing to uplift it. But I'll let Marco answer this one, as he has better understanding about what's affected by that bug.

Comment 25

[Marco Bonardo [:mak]]

(In reply to Julien Cristau [:jcristau] from comment #23)

Should we be uplifting bug 1401401 at the same time?

It's probably a good idea, since now we have that fix. That change should not be risky, so I will ask for an uplift. Whether it happens at the same time or a week later than this uplift, it's likely not important, considered we indeed broke URLs in the past already.

Anyway it won't solve all the problems, we still need a global strategy to remove urls that suddently become invalid from history, for which we have opened Bug 1557445.

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 9068173 [details]
Bug 1548306 - Do not allow the ^ character to appear in the hostname r=mayhemer

sec fix for 68.0b9

Comment 27

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33d0d344ea16b9219b36d922fe45549c29326e6a

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9068173 [details]
Bug 1548306 - Do not allow the ^ character to appear in the hostname r=mayhemer

Approved for 60.8esr.

Comment 29

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/4cb3565a91b1
Changes to test have been dropped because that doesn't exist on ESR60.