Closed
Bug 1083325
Opened 10 years ago
Closed 10 years ago
HttpChannelChild's mSecurityInfo is null, fails for GetSecurityInfo
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
e10s | m4+ | --- |
People
(Reporter: jimm, Assigned: jimm)
References
()
Details
(Whiteboard: e10s)
Attachments
(1 file, 1 obsolete file)
2.55 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Whiteboard: e10s
Assignee | ||
Comment 1•10 years ago
|
||
(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.
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
Note this doesn't fail for every cert, afaict. Still digging..
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
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.
Keywords: regression,
regressionwindow-wanted
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Keywords: regression,
regressionwindow-wanted
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+
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8506145 -
Attachment is obsolete: true
Attachment #8506328 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/76899623e9f4
Comment 11•10 years ago
|
||
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.
Description
•