1121982 - Update PSM to use NSS name constraints

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Richard Barnes [:rbarnes]]

Bug 991783 is adding a generic mechanism for name constraints to NSS.  Rather than the current duplicative mechanism in PSM [1], we should just call through to the NSS constraints.

[1] https://dxr.mozilla.org/mozilla-central/source/security/certverifier/NSSCertDBTrustDomain.cpp#77

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1121982.0.patch

This patch just removes the special ANSSI stuff from NSSCertDBTrustDomain.cpp and replaces it with a call down to CERT_GetImposedNameConstraints().

Keeler, I think since last time you reviewed the patch for bug 991783 (which adds CERT_GetImposedNameConstraints), it changed from returning a pointer to static data to returning a copy of the data that the caller owns.  Thus the use of ScopedSECItem.  Please make sure that looks OK.

Comment 2

[Richard Barnes [:rbarnes]]

Attachment: bug-1121982.0.patch

Oops, for got to `hg qref`

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8591452 [details] [diff] [review]
bug-1121982.0.patch

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

The use of ScopedSECItem looks correct to me. The changes to cert.h and genname.c should actually come from an update to NSS in something like bug 1144055 (currently that bug says "39", but any further changes to NSS 3.18.1 need to land in 40 as well). r=me with that addressed.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +90,5 @@
>      if (rv != Success) {
>        continue; // probably too big
>      }
>  
> +    const SECItem encodedIssuerNameSECItem = {

nit: I think it's common to omit the "SEC" part of the name in cases like this, but I'm not sure it matters

@@ +97,5 @@
> +      encodedIssuerName.GetLength()
> +    };
> +    ScopedSECItem nameConstraints(::SECITEM_AllocItem(nullptr, nullptr, 0));
> +    SECStatus srv = CERT_GetImposedNameConstraints(&encodedIssuerNameSECItem,
> +                                                  nameConstraints.get());

nit: indentation

@@ +104,4 @@
>          return Result::FATAL_ERROR_LIBRARY_FAILURE;
>        }
> +
> +      // If no constraints were found, continue without them

nit: let's say "imposed constraints"

Comment 4

[Richard Barnes [:rbarnes]]

Attachment: bug-1121982.1.patch r=keeler

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8591452 [details] [diff] [review]
> bug-1121982.0.patch
>
> The use of ScopedSECItem looks correct to me. The changes to cert.h and
> genname.c should actually come from an update to NSS in something like bug
> 1144055 (currently that bug says "39", but any further changes to NSS 3.18.1
> need to land in 40 as well). r=me with that addressed.

Oops, meant to take those out.  Done now.


> ::: security/certverifier/NSSCertDBTrustDomain.cpp
> @@ +90,5 @@
> >      if (rv != Success) {
> >        continue; // probably too big
> >      }
> >  
> > +    const SECItem encodedIssuerNameSECItem = {
> 
> nit: I think it's common to omit the "SEC" part of the name in cases like
> this, but I'm not sure it matters

Also fixed the same thing in FindIssuer below while I was there.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91f989aedf12

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91f989aedf12