Closed Bug 1120393 Opened 6 years ago Closed 6 years ago

Serialize/deserialize nsITransportSecurity.errorCode

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
https://hg.mozilla.org/mozilla-central/rev/74b8493b5610 added the errorCode field to the nsITransportSecurity interface, but it didn't change the serialization code.
The SSL error reporting will not obtain correct error codes without this change.

I also fixed a bug of serializing errorMessage. This bug was not uncovered because errorCode was always zero in the e10 child before this change.
Attachment #8547526 - Flags: review?(dkeeler)
Comment on attachment 8547526 [details] [diff] [review]
patch

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

Looks good, but needs some changes. r- for now.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +322,5 @@
>    rv = stream->Write32(mSubRequestsNoSecurity);
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  rv = stream->Write32(mErrorCode);

Let's be explicit about this cast: static_cast<uint32_t>(mErrorCode)
(bug 711408 should fix this in a better way, but we should do this for now).

@@ +327,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  // Do not call formatErrorMessage if mErrorMessageCached is already set
> +  // because formatErrorMessage will fail in an e10s child process, 

nit: trailing whitespace (also, end the comment with a '.' instead of ',')
Also, does this just fail in e10s, or is it an assertion failure? If the latter, we should check if we're in the child process and return NS_ERROR_FAILURE if so.

@@ +408,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  // PRErrorCode will be a negative value
> +  mErrorCode = errorCode;

We should be explicit about this cast: static_cast<PRErrorCode>(errorCode)
Attachment #8547526 - Flags: review?(dkeeler) → review-
Attached patch patch v2Splinter Review
Resolved review comments.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> Also, does this just fail in e10s, or is it an assertion failure? If the
> latter, we should check if we're in the child process and return
> NS_ERROR_FAILURE if so.

Good point. I added an error check in formatMessage.
Attachment #8547526 - Attachment is obsolete: true
Attachment #8548173 - Flags: review?(dkeeler)
Blocks: 1120942
Comment on attachment 8548173 [details] [diff] [review]
patch v2

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

Great - r=me with comments addressed. Although, it would be good to test this - can you add a test somewhere that uses add_connection_test and then serializes and deserializes the nsITransportSecurityInfo in the aWithSecurityInfo callback? (i.e. it should check that errorCode survives the process)

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +239,5 @@
>      return NS_OK;
>    }
>  
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    result.Truncate();

We should just hoist this Truncate to the top of the function and remove it from the if (errorCode == 0) block.

@@ +331,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +  // Do not call formatErrorMessage if mErrorMessageCached is already set
> +  // because formatErrorMessage will fail in an e10s child process.

Actually, I don't think this comment is necessary.
Attachment #8548173 - Flags: review?(dkeeler) → review+
Actually, it would be more useful to check that the error code is serialized/deserialized correctly in the SSL error reporting test. Is that test enabled for e10s?
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> Actually, it would be more useful to check that the error code is
> serialized/deserialized correctly in the SSL error reporting test. Is that
> test enabled for e10s?

Currently the error code will not survive even in non-e10s mode because the SSL error reporting will explicitly serialize/deserialize TranportSecurityInfo on sending the event regardless whether the e10s is enabled [1][2].
Looks like browser_bug846489.js did not test for the error code at all [3]. I'll add a test for it.

[1] https://hg.mozilla.org/mozilla-central/annotate/63006936ab99/browser/base/content/content.js#l268
[2] https://hg.mozilla.org/mozilla-central/annotate/63006936ab99/browser/base/content/browser.js#l2641
[3] https://hg.mozilla.org/mozilla-central/rev/287ddaf9b9df
Attached patch Unit testSplinter Review
Attachment #8548987 - Flags: review?(dkeeler)
Attachment #8548990 - Flags: review?(felipc)
Attachment #8548990 - Attachment is patch: true
Comment on attachment 8548987 [details] [diff] [review]
Unit test

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

Great - r=me.

::: security/manager/ssl/tests/unit/test_cert_chains.js
@@ +50,5 @@
> +  let deserialized = serHelper.deserializeObject(serialized);
> +  deserialized.QueryInterface(Ci.nsITransportSecurityInfo);
> +  do_check_eq(securityInfo.securityState, deserialized.securityState);
> +  do_check_eq(securityInfo.errorMessage, deserialized.errorMessage);
> +  do_check_eq(securityInfo.errorCode, deserialized.errorCode);

Let's also do_check_neq(deserialized.errorCode, 0); for thoroughness.
Attachment #8548987 - Flags: review?(dkeeler) → review+
Comment on attachment 8548990 [details] [diff] [review]
mochitest_browser

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

I'm just curious why is this necessary
Attachment #8548990 - Flags: review?(felipc) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=553a27466906

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> ::: security/manager/ssl/tests/unit/test_cert_chains.js
> @@ +50,5 @@
> > +  let deserialized = serHelper.deserializeObject(serialized);
> > +  deserialized.QueryInterface(Ci.nsITransportSecurityInfo);
> > +  do_check_eq(securityInfo.securityState, deserialized.securityState);
> > +  do_check_eq(securityInfo.errorMessage, deserialized.errorMessage);
> > +  do_check_eq(securityInfo.errorCode, deserialized.errorCode);
> 
> Let's also do_check_neq(deserialized.errorCode, 0); for thoroughness.

I did not add this because:
1. It already tested do_chack_eq(securityInfo.errorCode, <nonzero>) and do_chack_eq(securityInfo.errorCode, deserialized.errorCode)
2. deserialized.errorCode can be zero if the connection succeeds.

Instead, I added expectedErrorCode parameter to the function, moved the do_chack_eq into test_security_info_serialization, and added a test for the successful connection case.
In general, we should avoid do_check_neq if we know what is the expected value.
(In reply to :Felipe Gomes from comment #9)
> I'm just curious why is this necessary

Because it would have caught this earlier and SSL error report will be less useful if the server will always see zero for all errors.
You need to log in before you can comment on or make changes to this bug.