Closed Bug 1083325 Opened 10 years ago Closed 10 years ago

HttpChannelChild's mSecurityInfo is null, fails for GetSecurityInfo

Categories

(Core :: Networking: HTTP, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m4+ ---

People

(Reporter: jimm, Assigned: jimm)

References

()

Details

(Whiteboard: e10s)

Attachments

(1 file, 1 obsolete file)

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4812

This breaks most of our bad cert handling. It occurs on the failed channel passed to nsDocShell::DisplayLoadError.
Whiteboard: e10s
(In reply to Jim Mathies [:jimm] from comment #0)
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#4812
> 
> This breaks most of our bad cert handling. It occurs on the failed channel
> passed to nsDocShell::DisplayLoadError.

This breaks some of our bad cert handling. There are a default error messages for a number of issues but for some reason pinning errors aren't in there.
On the parent side in HttpChannelParent, we try to serialize security info via NS_SerializeToString [1][2], when we call TransportSecurityInfo::Write [3], mSSLStatus is null. So the write operation fails, and we don't get sec info in the child.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#669
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSerializationHelper.cpp#21
[3] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/TransportSecurityInfo.cpp#297
Note this doesn't fail for every cert, afaict. Still digging..
Attached patch tsi patch v.1 (obsolete) — Splinter Review
A bit of a stab in the dark since I've not worked with TransportSecurityInfo before. When mSSLStatus isn't available, there's still information in TransportSecurityInfo that we need to get across to the child, including the proper error message (for example, cert pinning errors).

Without this patch the test url displays as about:blank, with this change we get the cert error about page with detailed error message about pinning violations.
Assignee: nobody → jmathies
Attachment #8506145 - Flags: review?(dkeeler)
Unless I'm misremembering, this used to work in e10s (or maybe it was only overridable errors that worked?). That patch seems reasonable, but I'd like to know what changed that broke this before going forward.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #5)
> Unless I'm misremembering, this used to work in e10s (or maybe it was only
> overridable errors that worked?). That patch seems reasonable, but I'd like
> to know what changed that broke this before going forward.

I did spot checks on on the first of the month going back to 4-2014. This particular pinning test case hasn't worked over tha last six months. Around May we started seeing the current about:blank behavior, prior to that the url generated a 404 not found page.

Does that cover enough distance back?
Oh, I think I get it. For overridable errors, there will be an mSSLStatus on the TransportSecurityInfo. However, for non-overridable errors, there won't. So, when we serialize/deserialize a TransportSecurityInfo as a result of a non-overridable error, it fails.
Comment on attachment 8506145 [details] [diff] [review]
tsi patch v.1

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

This looks good. r=me with comments addressed.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +325,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +
> +  // May be null

A comment like this would be useful I think:

"For successful connections and for connections with overridable errors, mSSLStatus will be non-null. However, for connections with non-overridable errors, it will be null."

@@ +330,2 @@
>    nsCOMPtr<nsISerializable> serializable(mSSLStatus);
> +  rv = NS_WriteOptionalCompoundObject(stream,

Looks like this will change the underlying serialized representation of TransportSecurityInfo, so we have to rev TRANSPORTSECURITYINFOMAGIC by updating everything but the first 4 bytes.

@@ +388,5 @@
>    rv = stream->ReadString(mErrorMessageCached);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  

nit: unnecessary whitespace

@@ +396,4 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  // May be be null

Maybe have the same/a similar comment here.
Attachment #8506145 - Flags: review?(dkeeler) → review+
Attachment #8506145 - Attachment is obsolete: true
Attachment #8506328 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76899623e9f4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.