Closed Bug 861117 Opened 7 years ago Closed 6 years ago
"ASSERTION: Something is wrong with STS: Is
Sts URI failed" with U+2028 (line separator) in URL
###!!! ASSERTION: Something is wrong with STS: IsStsURI failed.: 'NS_SUCCEEDED(rv)', file netwerk/protocol/http/nsHttpChannel.cpp, line 377
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 376 NS_ASSERTION(NS_SUCCEEDED(rv), (gdb) p rv $1 = NS_ERROR_MALFORMED_URI
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?
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.
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
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
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?).
Brian: ping. This is tiny and the try run looks good. Can you review this?
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+
Target Milestone: --- → mozilla23
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Backed out of mozilla-central: https://hg.mozilla.org/mozilla-central/rev/55e790d0ff02 This caused a regression, see bug 871560.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 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.
Try looked good. Landed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/20a739529288
Target Milestone: mozilla23 → mozilla29
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.