1083325 - HttpChannelChild's mSecurityInfo is null, fails for GetSecurityInfo

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jim Mathies [:jimm]]

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.

Comment 1

[Jim Mathies [:jimm]]

(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.

Comment 2

[Jim Mathies [:jimm]]

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

Comment 3

[Jim Mathies [:jimm]]

Note this doesn't fail for every cert, afaict. Still digging..

Comment 4

[Jim Mathies [:jimm]]

Attachment: tsi patch v.1

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.

Comment 5

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

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.

Comment 6

[Jim Mathies [:jimm]]

(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?

Comment 7

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

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 8

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

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.

Comment 9

[Jim Mathies [:jimm]]

Attachment: tsi patch v.2 r=dkeeler

Comment 10

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76899623e9f4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76899623e9f4