Closed Bug 1020683 Opened 5 years ago Closed 5 years ago

mozilla::pkix's VerifyEncodedOCSPResponse uses CERTCertificate internally

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(2 files, 1 obsolete file)

This patch removes all *direct* *internal* uses of CERTCertificate from VerifyEncodedOCSPResponse. It does not change the signature of VerifyEncodedOCSPResponse; another patch in another bug will replace the CERTCertificate parameter of that function with a parameter of another type, after the OCSP GTests land, so that it is easier to uplift the OCSP GTests to mozilla-aurora and mozilla-beta.

This patch removes the usage of CERT_CompareName. I found that NSS's ocsp.c was not using CERT_CompareName at all, so we must not need to use it either. This reduces another otherwise-hard-to-remove NSS dependency.

Use of NSS's SECKEYPublicKey is now isolated to pkixkey.cpp. This will help with portability later. I'm also going to be removing the declarations of the Scoped* types from pkixtypes.h as mozilla::pkix reduces its use of NSS types in favor of doing more things itself and/or making the uses of those types implementation details that are not exposed to the application.
Attachment #8434572 - Flags: review?(dkeeler)
Comment on attachment 8434572 [details] [diff] [review]
remove-CERTCertificate-from-inside-VerifyEncodedOCSPResponse.patch

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

MatchKeyHash and VerifySignature in pkixocsp.cpp need a little cleanup, I think, so I'd like to take another look at this. Otherwise looks good.

::: security/apps/AppTrustDomain.cpp
@@ +162,5 @@
>  }
>  
>  SECStatus
>  AppTrustDomain::VerifySignedData(const CERTSignedData* signedData,
> +                                  const SECItem& subjectPublicKeyInfo)

nit: in splinter, the indentation looks wrong here - are there tabs on this line? (In any case, it looks like this should be indented one less space.)

::: security/pkix/include/pkix/pkix.h
@@ -99,5 @@
>              /*optional*/ const SECItem* stapledOCSPResponse,
>                   /*out*/ ScopedCERTCertList& results);
>  
> -// Verify the given signed data using the public key of the given certificate.
> -// (EC)DSA parameter inheritance is not supported.

Is this no longer the case?

::: security/pkix/include/pkix/pkixtypes.h
@@ -38,5 @@
>  
>  typedef ScopedPtr<CERTCertificate, CERT_DestroyCertificate>
>          ScopedCERTCertificate;
>  typedef ScopedPtr<CERTCertList, CERT_DestroyCertList> ScopedCERTCertList;
> -typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>

Why remove this if it's still used in pkixkey.cpp?

@@ -112,5 @@
>  
> -  // Verify the given signature using the public key of the given certificate.
> -  // The implementation should be careful to ensure that the given certificate
> -  // has all the public key information needed--i.e. it should ensure that the
> -  // certificate is not trying to use EC(DSA) parameter inheritance.

I just want to confirm that this isn't a concern anymore.

::: security/pkix/lib/pkixbuild.cpp
@@ +187,5 @@
>      return rv;
>    }
>  
> +  return subject.VerifyOwnSignatureWithKey(
> +                    trustDomain, potentialIssuer.GetSubjectPublicKeyInfo());

This looks like it's indented three spaces from the beginning of "Verify...". How about either two spaces or two spaces from "subject..."

::: security/pkix/lib/pkixocsp.cpp
@@ +231,5 @@
> +  }
> +}
> +
> +static Result
> +VerifySignature(TrustDomain& trustDomain,

I don't think this should also be called VerifySignature - maybe VerifySignatureForSPKI?

@@ +277,5 @@
> +    rv = MatchResponderID(responderIDType, responderID,
> +                          cert.GetSubject(), cert.GetSubjectPublicKeyInfo(),
> +                          match);
> +    if (rv == Success) {
> +      if (match) {

This section needs a little reorganization - handle the errors early/where they happen so the success cases are the direct execution path (it makes the code clearer and uses less indentation).

@@ +278,5 @@
> +                          cert.GetSubject(), cert.GetSubjectPublicKeyInfo(),
> +                          match);
> +    if (rv == Success) {
> +      if (match) {
> +        rv = CheckOCSPResponseSignerCert(context.trustDomain, cert, 

nit: trailing space

@@ +288,5 @@
> +                                 cert.GetSubjectPublicKeyInfo());
> +        }
> +      }
> +    }
> +    if (rv == FatalError) {

Handle errors early (where they happen)

@@ +777,5 @@
> +  der::Input spki;
> +  //if (spki.Init(subjectPublicKeyInfo.data, subjectPublicKeyInfo.len)
> +  //      != der::Success) {
> +  //  return MapSECStatus(SECFailure);
> +  //}

some leftover code here?

@@ +779,5 @@
> +  //      != der::Success) {
> +  //  return MapSECStatus(SECFailure);
> +  //}
> +
> +  {

why the extra scope?

@@ +785,5 @@
> +    if (input.Init(subjectPublicKeyInfo.data, subjectPublicKeyInfo.len)
> +          != der::Success) {
> +      return MapSECStatus(SECFailure);
> +    }
> +    

ws

::: security/pkix/lib/pkixutil.h
@@ +134,5 @@
>    // which only takes a non-const pointer because VerifyEncodedOCSPResponse
>    // requires it, and that is only because the implementation of
>    // VerifyEncodedOCSPResponse does a CERT_DupCertificate. TODO: get rid
>    // of that CERT_DupCertificate so that we can make all these things const.
> +  /*const*/ CERTCertificate* GetNSSCert() const { return nssCert.get(); }

Doesn't this patch fix this? In fact, it builds for me just fine if I make this const.
Attachment #8434572 - Flags: review?(dkeeler) → review-
Comment on attachment 8434572 [details] [diff] [review]
remove-CERTCertificate-from-inside-VerifyEncodedOCSPResponse.patch

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

Thanks for the review.

::: security/pkix/include/pkix/pkix.h
@@ -99,5 @@
>              /*optional*/ const SECItem* stapledOCSPResponse,
>                   /*out*/ ScopedCERTCertList& results);
>  
> -// Verify the given signed data using the public key of the given certificate.
> -// (EC)DSA parameter inheritance is not supported.

It is still the case. But, note that with the new interface it is impossible to do (EC)DSA parameter inheritance because you don't even know what certificates you're dealing with, so I don't think it makes sense to talk about it any more.

::: security/pkix/lib/pkixbuild.cpp
@@ +187,5 @@
>      return rv;
>    }
>  
> +  return subject.VerifyOwnSignatureWithKey(
> +                    trustDomain, potentialIssuer.GetSubjectPublicKeyInfo());

OK. I shifted it over to the left one space.

::: security/pkix/lib/pkixocsp.cpp
@@ +231,5 @@
> +  }
> +}
> +
> +static Result
> +VerifySignature(TrustDomain& trustDomain,

How about VerifyOCSPSignedData?

@@ +277,5 @@
> +    rv = MatchResponderID(responderIDType, responderID,
> +                          cert.GetSubject(), cert.GetSubjectPublicKeyInfo(),
> +                          match);
> +    if (rv == Success) {
> +      if (match) {

Done.

@@ +777,5 @@
> +  der::Input spki;
> +  //if (spki.Init(subjectPublicKeyInfo.data, subjectPublicKeyInfo.len)
> +  //      != der::Success) {
> +  //  return MapSECStatus(SECFailure);
> +  //}

Removed.

@@ +779,5 @@
> +  //      != der::Success) {
> +  //  return MapSECStatus(SECFailure);
> +  //}
> +
> +  {

I thought it would be clearer to limit the scope of variable "input" here since we're parsing things "inside out" compared to how we normally do. Normally, we parse the interior of a value and then verify that we're at the end of the input. Here, we're doing the check that the internal thing is the only thing in the wrapping thing, and then we parse the internal thing.

Also, I am trying to reduce the chance for confusion between the input and spki variables (i.e. reduce the chance I'd pass input instead of spki).

::: security/pkix/lib/pkixutil.h
@@ +134,5 @@
>    // which only takes a non-const pointer because VerifyEncodedOCSPResponse
>    // requires it, and that is only because the implementation of
>    // VerifyEncodedOCSPResponse does a CERT_DupCertificate. TODO: get rid
>    // of that CERT_DupCertificate so that we can make all these things const.
> +  /*const*/ CERTCertificate* GetNSSCert() const { return nssCert.get(); }

I made it const.
Comment on attachment 8434572 [details] [diff] [review]
remove-CERTCertificate-from-inside-VerifyEncodedOCSPResponse.patch

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

::: security/pkix/include/pkix/pkixtypes.h
@@ -38,5 @@
>  
>  typedef ScopedPtr<CERTCertificate, CERT_DestroyCertificate>
>          ScopedCERTCertificate;
>  typedef ScopedPtr<CERTCertList, CERT_DestroyCertList> ScopedCERTCertList;
> -typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey>

My intent is to eventually remove all references of NSS types from the public interface of mozilla::pkix.
Comment on attachment 8435312 [details] [diff] [review]
remove-CERTCertificate-from-inside-VerifyEncodedOCSPResponse.patch [v2]

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

I think this addresses all your concerns. See also my earlier replies to your review feedback.

::: security/pkix/lib/pkixutil.h
@@ +134,5 @@
>    // which only takes a non-const pointer because VerifyEncodedOCSPResponse
>    // requires it, and that is only because the implementation of
>    // VerifyEncodedOCSPResponse does a CERT_DupCertificate. TODO: get rid
>    // of that CERT_DupCertificate so that we can make all these things const.
> +  /*const*/ CERTCertificate* GetNSSCert() const { return nssCert.get(); }

David, I tried making this return a const CERTCertificate*. However, it turns out that my patch for bug 1020682 requires it to be non-const. I think leaving it non-const for now makes sense, until we can remove the need to call GetNSSCert at all (soon).
Comment on attachment 8435312 [details] [diff] [review]
remove-CERTCertificate-from-inside-VerifyEncodedOCSPResponse.patch [v2]

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

Wow - interdiff actually worked for once. Anyway, looks great.

::: security/pkix/lib/pkixocsp.cpp
@@ +287,5 @@
> +    if (match) {
> +      rv = CheckOCSPResponseSignerCert(context.trustDomain, cert,
> +                                        context.issuerSubject,
> +                                        context.issuerSubjectPublicKeyInfo,
> +                                        context.time);

nit: indentation on these three lines

@@ +780,5 @@
> +  // SubjectPublicKeyInfo  ::=  SEQUENCE  {
> +  //    algorithm            AlgorithmIdentifier,
> +  //    subjectPublicKey     BIT STRING  }
> +
> +  der::Input spki;

Ok - a comment explaining the rationale here would be nice (something like "Extract just the spki from the DER encoding. The der::Input input is in a private scope to avoid confusing it with spki.")
Attachment #8435312 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #7)
> Wow - interdiff actually worked for once. Anyway, looks great.

Amazing!

> nit: indentation on these three lines

Fixed

> Ok - a comment explaining the rationale here would be nice (something like
> "Extract just the spki from the DER encoding. The der::Input input is in a
> private scope to avoid confusing it with spki.")

I added a similar comment.

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa797212429e
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fa797212429e

^ That is part 1. This is part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d168ba8fb
Duplicate of this bug: 980163
You need to log in before you can comment on or make changes to this bug.