Closed Bug 861117 Opened 7 years ago Closed 6 years ago

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


(Core :: Networking: HTTP, defect)

Not set





(Reporter: jruderman, Assigned: geekboy)


(Blocks 1 open bug)


(Keywords: assertion, testcase)


(3 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: Something is wrong with STS: IsStsURI failed.: 'NS_SUCCEEDED(rv)', file netwerk/protocol/http/nsHttpChannel.cpp, line 377
Attached file stack
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?
> 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

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

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?
Flags: needinfo?
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.
Flags: needinfo? → needinfo?(dkeeler)
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.
Attached patch proposed patch (obsolete) — Splinter Review
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:
Assignee: nobody → sstamm
Attachment #741577 - Flags: review?(bsmith)
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?).
Flags: needinfo?(dkeeler)
Brian: ping.  This is tiny and the try run looks good.  Can you review this?
Flags: needinfo?(bsmith)
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.
Attachment #741577 - Flags: review?(bsmith) → review+
Flags: needinfo?(bsmith)
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 871560
Backed out of mozilla-central:

This caused a regression, see bug 871560.
Resolution: FIXED → ---
Depends on: 633001
Attached patch patchSplinter Review
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:
Attachment #741577 - Attachment is obsolete: true
Attachment #8365185 - Flags: review+
Comment on attachment 8365185 [details] [diff] [review]

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.
Try looked good.  Landed again:
Target Milestone: mozilla23 → mozilla29
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.