Closed Bug 1020682 Opened 5 years ago Closed 5 years ago

mozilla::pkix constructs the result cert chain twice

Categories

(Core :: Security: PSM, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(1 file)

First, we construct the result cert chain in order to deal with key pinning. Then we through that cert chain away and build another copy of it to return to the application. We should only build one copy of the cert chain.

Also, I took this opportunity to simplify/reduce the code in preparation for getting rid of mozilla::pkix's use of CERTCertList completely.
Attachment #8434567 - Flags: review?(cviecco)
Comment on attachment 8434567 [details] [diff] [review]
build-the-result-chain-once.patch

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

::: security/pkix/lib/pkixbuild.cpp
@@ +231,5 @@
>    }
>  
>    if (trustLevel == TrustLevel::TrustAnchor) {
> +    // End of the recursion.
> +    

nit space

@@ +239,1 @@
>        return MapSECStatus(SECFailure);

why not fatal error here an also when building the chain?

@@ +253,1 @@
>      if (srv != SECSuccess) {

you need to delete results here.
Attachment #8434567 - Flags: review?(cviecco) → review-
Comment on attachment 8434567 [details] [diff] [review]
build-the-result-chain-once.patch

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

::: security/pkix/lib/pkixbuild.cpp
@@ +231,5 @@
>    }
>  
>    if (trustLevel == TrustLevel::TrustAnchor) {
> +    // End of the recursion.
> +    

Thanks. I will fix this before checkin.

@@ +239,1 @@
>        return MapSECStatus(SECFailure);

MapSECStatus's job is to map the error to FatalError or RecoverableError as needed, and it will map SEC_ERROR_NO_MEMORY to FatalError in this case.

@@ +253,1 @@
>      if (srv != SECSuccess) {

When a function returns an error code, any additional outputs from that function must be ignored because they are undefined unless explicitly documented otherwise in the function's documentation.

Also, consider how the construction of the results worked before. There was no place where we reset the results to be empty in the code before this patch. That's a good indication that we don't need to do it in this patch. If we do need to do so, it's a separate bug.
Attachment #8434567 - Flags: review- → review?(cviecco)
Comment on attachment 8434567 [details] [diff] [review]
build-the-result-chain-once.patch

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

::: security/pkix/lib/pkixbuild.cpp
@@ +253,1 @@
>      if (srv != SECSuccess) {

notice how before certChain was a scoped list, and results did not got created when isChainValid failed. With the current patch you will have a memory leak when trustDomain.IsChainValid fails (and latter succeeds) (allocate results first, then call it again, and you call it again). 

What I propose you should do is to make a scoped list and then on success tranfer the data to the results output.
Attachment #8434567 - Flags: review?(cviecco) → review-
Comment on attachment 8434567 [details] [diff] [review]
build-the-result-chain-once.patch

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

::: security/pkix/lib/pkixbuild.cpp
@@ +253,1 @@
>      if (srv != SECSuccess) {

It is true that when IsChainValid fails, the result list will still contain the cert chain. However, when re re-assign a new result list to the same ScopedCERTList later, the previous value won't leak; instead it will be destroyed as required. Here is ScopedPtr::operator=:

  void operator=(T* newValue)
  {
    if (mValue) {
      Destroyer(mValue);
    }
    mValue = newValue;
  }
Attachment #8434567 - Flags: review- → review?(cviecco)
Comment on attachment 8434567 [details] [diff] [review]
build-the-result-chain-once.patch

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

::: security/pkix/lib/pkixbuild.cpp
@@ +232,5 @@
>  
>    if (trustLevel == TrustLevel::TrustAnchor) {
> +    // End of the recursion.
> +    
> +    // Construct the results cert chain.

Please add a comment here on that this assignment cleanup results if previously assigned.

@@ +253,1 @@
>      if (srv != SECSuccess) {

You are right.
Attachment #8434567 - Flags: review?(cviecco) → review+
(In reply to Camilo Viecco (:cviecco) from comment #5)
> Comment on attachment 8434567 [details] [diff] [review]
> build-the-result-chain-once.patch
> 
> Review of attachment 8434567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/pkix/lib/pkixbuild.cpp
> @@ +232,5 @@
> >  
> >    if (trustLevel == TrustLevel::TrustAnchor) {
> > +    // End of the recursion.
> > +    
> > +    // Construct the results cert chain.
> 
> Please add a comment here on that this assignment cleanup results if
> previously assigned.

I didn't add the comment you requested. There are several other places in the code that depend on this working the way it works and this particular place isn't anything special.

Thanks for the reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/77f2f8f2c506
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/77f2f8f2c506
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.