861117 - "ASSERTION: Something is wrong with STS: IsStsURI failed" with U+2028 (line separator) in URL

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Something is wrong with STS: IsStsURI failed.: 'NS_SUCCEEDED(rv)', file netwerk/protocol/http/nsHttpChannel.cpp, line 377

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

So that assertion happens if nsIStrictTransportSecurityService->IsStsURI() returns a failure.

The likely culprits:
* Called off main thread
* nsIURI->GetHost() fails
* Rewriting URI (normalized scheme) for permissions DB fails

Jesse: do you have the causal error code handy?

Comment 3

[Jesse Ruderman]

> WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file /Users/jruderman/trees/mozilla-central/docshell/base/nsDocShell.cpp, line 8412
> WARNING: malformed hostname: file /Users/jruderman/trees/mozilla-central/netwerk/base/src/nsURLParsers.cpp, line 567
> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Users/jruderman/trees/mozilla-central/security/manager/boot/src/nsStrictTransportSecurityService.cpp, line 139
> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Users/jruderman/trees/mozilla-central/security/manager/boot/src/nsStrictTransportSecurityService.cpp, line 430

(gdb) f 4
#4  0x000000010181389f in mozilla::net::nsHttpChannel::Connect (this=0x120eb3a00) at /Users/jruderman/trees/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:376
376	        NS_ASSERTION(NS_SUCCEEDED(rv),

(gdb) p rv
$1 = NS_ERROR_MALFORMED_URI

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Looks like it's probably dying on nsIURI::GetHost() then.

http://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsStrictTransportSecurityService.cpp#417

Currently I'm thinking we should return NS_OK (and False for IsStsUri), but return early, when the URI is malformed.  Is there currently a commonly accepted way to signal "this may be bad" in debug builds without asserting?  NS_WARNING?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Actually, it looks like this could also be a problem in GetPrincipalForURI() -- for URIs that are malformed.

keeler, was this specifically added for webapps?  Why are we looking for the principal and using that instead of the URI?

It appears I can get around the assertion by changing lines 418 and 430 to be NS_ENSURE_SUCCESS(rv, NS_OK) instead of NS_ENSURE_SUCCESS(rv,rv).  This is probably an ok fix since it basically says that all malformed URIs aren't STS URIs.  Not sure how to best test this though.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Spoke with bsmith about this one.  He thinks that really the caller of IsStsURI() needs to be making sure the URI is not malformed.  I agree that it's a bit strange that the HTTP channel that's calling the STS service is trying to open an invalid URI/host (this should be validated in HttpBaseChannel::Init().

We decided that the IsStsURI should fail when the URI is malformed since it's only called on HTTP channels (and all of those probably need well-formed hosts and such).  So I think we should change the assertion (see above) to an NS_ENSURE_SUCCESS that relies on the creator of the connection to catch any errors.  The effect will be that the call to nsHttpChannel::Connect will return an error with this test case.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Tested this NS_ASSERT -> NS_ENSURE_SUCCESS change manually with the attached testcase and it doesn't seem to cause any additional assertions.  Trying it out: https://tbpl.mozilla.org/?tree=Try&rev=9a9c2444b712

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

For what it's worth, I think using NS_ENSURE_SUCCESS sounds good. I've been doing some digging, and I think there's a bug in the code that creates URIs (otherwise, how could we have a valid nsIURI but result in an invalid nsIURI when we take the host value from the former and just slap "https://" on the front?).

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Brian: ping.  This is tiny and the try run looks good.  Can you review this?

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 741577 [details] [diff] [review]
proposed patch

Review of attachment 741577 [details] [diff] [review]:
-----------------------------------------------------------------

It is bad that the creator of the channel tried to open the channel with an invalid URI. That caller really should also be fixed (in another bug). Unfortunately, it would require some detective work to find out what that caller is.

Regardless, this is the correct thing to do in this part of the code.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/000ed86d069f

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/000ed86d069f

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Backed out of mozilla-central: https://hg.mozilla.org/mozilla-central/rev/55e790d0ff02

This caused a regression, see bug 871560.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch

I rebased this patch on mozilla-central and ran through the STR on the regression that caused this backout (bug 871560 comment 1, IPV6 literals without DNS do not work).  Applying this patch no longer causes the regression so I think we should reland it.  Just to be safe, I'll spot check for latent regressions. 

Try on windows: https://tbpl.mozilla.org/?tree=Try&rev=1b2360090ebf

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8365185 [details] [diff] [review]
patch

Review of attachment 8365185 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Someday I want to figure out how we get here with a URI like that, but I don't think it's important atm.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Try looked good.  Landed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/20a739529288

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/20a739529288