Ryan Sleevi Assignee
	Description • 11 years ago

This is a regression introduced by Bug 816853 that affects the libpkix certificate verification cache.

Steps to reproduce:
1) Load a certificate/root into the NSS DB
2) Mark it as trusted for SSL
3) Perform a verification for a certificate that chains to that root
4) Remove the trust bits
5) Perform another verification

Expected:
It fails, due to the trust bits no longer existing.

Actual:
It succeeds, due to successfully hitting the cache.

	Ryan Sleevi Assignee
	Comment 1 • 11 years ago

This is a big issue for Chromium due to ChromeOS, and was originally reported at https://code.google.com/p/chromium/issues/detail?id=224612

The specific call sequence is that
- During the first verification, PKIX_PL_Ceert_IsCertTrusted is called
- Pre-cond: pl_cert->isUserTrustAnchor = PKIX_FALSE
- Because it fails the condition on lines 3318 (trustOnlyUserAnchors || cert->isUserTrustAnchor) libpkix queries NSS for the trust flags, discovers the cert is trusted (via PKIX_CertStore_GetTrustCallback - line 3330 - 3336)

It then works to add the chain to the cache, by calling
- PKIX_TrustAnchor_CreateWithCert
  - which calls PKIX_PL_Cert_SetAsTrustAnchor
  - which sets pl_cert->isUserTrustAnchor = PKIX_TRUE

On subsequent re-validations, it hits the cert->isUserTrustAnchor for the cert in the cache, which then by-passes requerying the CertStore to see what the trust flags are.

	Ryan Sleevi Assignee
	Comment 3 • 11 years ago

Bob: Would you take a look here.

The issue arises from the fact that there are two ways to create TrustAnchor objects, with very different meanings of cert->isUserTrustAnchor:
- User trust anchors (from the processing params), which are synthesized into TrustAnchor objects, with the cert updated to cert->isUserTrustAnchor
- Cached trust anchors, where we cache the result (including the original PKIX_PL_Certificate)

The issue is we only want to respect user trust anchors from the input set - we DON'T want to respect the flags on the cached cert, unless it ALSO exists in the newly supplied trust anchors.

I feel like this is somewhat of a hacky way to do it, but PKIX_PL_Cert_IsCertTrusted only has two callsites.

The alternative is to do a scan of the input trust anchors, rather than/before calling PKIX_PL_Cert_IsCertTrusted (which is what the cache code does), but that's a fairly inefficient means - which is why the original approach was used, I'd wager.

Hopefully this code makes sense to understand. We have some Chromium unit tests that are passing fine now, I have to figure a good way to distill them into NSS unit tests though.

	Wan-Teh Chang
	Comment 4 • 11 years ago

Comment on attachment 744008 [details] [diff] [review]
Initial patch (no tests)

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

::: lib/libpkix/include/pkix_pl_pki.h
@@ +1513,5 @@
>   *      States that we can only trust explicitly defined user trust anchors.
> + *  "ignoreTrustAnchors"
> + *      States that we will trust this certificate if it has been explicitly
> + *      defined as a trust anchor. This value is only meaningful if
> + *      trustOnlyUserAnchors is PKIX_FALSE. If set to PKIX_TRUE, then the

"If set to PKIX_TRUE": it is not clear whether this is referring
to trustOnlyUserAnchors or ignoreTrustAnchors.

I wonder if we should replace the two booleans (trustOnlyUserAnchors
and ignoreTrustAnchors) with an enum type.

::: lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
@@ +3321,1 @@
>              /* discard our |trusted| value since we are using the anchors */

This comment was written when the code looked like this:

+        if (trustOnlyUserAnchors) {
+                *pTrusted = cert->isUserTrustAnchor;
+
+		/* discard our trust old value since we are using the anchors */
+		goto cleanup;
+	}

Is this comment still accurate?

	Ryan Sleevi Assignee
	Comment 5 • 11 years ago

Comment on attachment 744008 [details] [diff] [review]
Initial patch (no tests)

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

Bob, have you had a chance to look at this?

::: lib/libpkix/include/pkix_pl_pki.h
@@ +1513,5 @@
>   *      States that we can only trust explicitly defined user trust anchors.
> + *  "ignoreTrustAnchors"
> + *      States that we will trust this certificate if it has been explicitly
> + *      defined as a trust anchor. This value is only meaningful if
> + *      trustOnlyUserAnchors is PKIX_FALSE. If set to PKIX_TRUE, then the

This function only has two callsites, so that works for me if you think it'd be clearer.

::: lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c
@@ +3321,1 @@
>              /* discard our |trusted| value since we are using the anchors */

trustOnlyUserAnchors = "using the anchors"
!ignoreTrustAnchors = "using the anchors"

So yeah, I think it's still accurate.

	Robert Relyea
	Comment 6 • 11 years ago

Comment on attachment 744008 [details] [diff] [review]
Initial patch (no tests)

r+ rrelyea

	Ryan Sleevi Assignee
	Comment 7 • 11 years ago

wtc: I updated the patch to use enums, rather than booleans, per your suggestion. Otherwise, the logic is the same. Please let me know if you're happy with the updated change.

NOTE: I considered moving the enum into a global header (such as pkixt.h), but the two considerations were:
1) This is an internal implementation detail of the PL layer, not part of the "public" API (ish...)
2) Revocation checking uses a mix of enums (pkixt.h) and #defines (eg: pkix_revocationchecker.h), so it seemed acceptable to mix the function definitions and enum definitions in the same file.

	Wan-Teh Chang
	Comment 8 • 11 years ago

Comment on attachment 748958 [details] [diff] [review]
Updated patch to use enum instead of bools

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

r=wtc. Thanks.

::: lib/libpkix/include/pkix_pl_pki.h
@@ +1488,4 @@
>          PKIX_PL_PublicKey *pubKey,
>          void *plContext);
>  
> +/* A set of flags to indicate how trust anchors should be handled by

Nit: perhaps we should always say "explicitly configured trust anchors"?

@@ +1492,5 @@
> + * PKIX_PL_Cert_IsCertTrusted
> + */
> +typedef enum PKIX_PL_TrustAnchorModeEnum {
> +        /* Indicates trust anchors should be ignored; only the underlying
> +         * platform's trust settings should be used.

Nit: it is not clear what "underlying platform" means. Does it mean "NSS"
rather than the OS?

@@ +1494,5 @@
> +typedef enum PKIX_PL_TrustAnchorModeEnum {
> +        /* Indicates trust anchors should be ignored; only the underlying
> +         * platform's trust settings should be used.
> +         */
> +        PKIX_PL_TrustAnchorMode_Ignore,

Please add blank lines between the enumerators.

@@ +1501,5 @@
> +         * Note: If the underlying platform supports marking a certificate as
> +         *       explicitly untrustworthy, explicitly configured trust anchors
> +         *       MAY be ignored/rejected.
> +         */
> +        PKIX_PL_TrustAnchorMode_Optional,

Nit: I think something like "supplemental" or "additive" would be more
descriptive than "optional".

@@ +1506,5 @@
> +        /* Indicates that ONLY trust anchors should be considered - that is,
> +         * do not consult the underlying platform.
> +         * Note: If the underlying platform supports marking a certificate as
> +         *       explicitly untrustworthy, explicitly configured trust anchors
> +         *       MAY be ignored/rejected.

Is this correct? This seems to contradict the statement "do not consult
the underlying platform".

::: lib/libpkix/pkix/top/pkix_build.c
@@ +3059,5 @@
> +        }
> +
> +        if ((!trusted && !state->buildConstants.trustOnlyUserAnchors) ||
> +            !state->buildConstants.anchors ||
> +            !state->buildConstants.anchors->length) {

Is state->buildConstants.anchors->length equal to state->buildConstants.numAnchors?

If so, it should be sufficient to just test !state->buildConstants.numAnchors.
See the test on line 853.

	Wan-Teh Chang
	Comment 9 • 11 years ago

Comment on attachment 748958 [details] [diff] [review]
Updated patch to use enum instead of bools

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

r=wtc. Thanks.

::: lib/libpkix/include/pkix_pl_pki.h
@@ +1488,4 @@
>          PKIX_PL_PublicKey *pubKey,
>          void *plContext);
>  
> +/* A set of flags to indicate how trust anchors should be handled by

Nit: perhaps we should always say "explicitly configured trust anchors"?

@@ +1492,5 @@
> + * PKIX_PL_Cert_IsCertTrusted
> + */
> +typedef enum PKIX_PL_TrustAnchorModeEnum {
> +        /* Indicates trust anchors should be ignored; only the underlying
> +         * platform's trust settings should be used.

Nit: it is not clear what "underlying platform" means. Does it mean "NSS"
rather than the OS?

@@ +1494,5 @@
> +typedef enum PKIX_PL_TrustAnchorModeEnum {
> +        /* Indicates trust anchors should be ignored; only the underlying
> +         * platform's trust settings should be used.
> +         */
> +        PKIX_PL_TrustAnchorMode_Ignore,

Please add blank lines between the enumerators.

@@ +1501,5 @@
> +         * Note: If the underlying platform supports marking a certificate as
> +         *       explicitly untrustworthy, explicitly configured trust anchors
> +         *       MAY be ignored/rejected.
> +         */
> +        PKIX_PL_TrustAnchorMode_Optional,

Nit: I think something like "supplemental" or "additive" would be more
descriptive than "optional".

@@ +1506,5 @@
> +        /* Indicates that ONLY trust anchors should be considered - that is,
> +         * do not consult the underlying platform.
> +         * Note: If the underlying platform supports marking a certificate as
> +         *       explicitly untrustworthy, explicitly configured trust anchors
> +         *       MAY be ignored/rejected.

Is this correct? This seems to contradict the statement "do not consult
the underlying platform".

::: lib/libpkix/pkix/top/pkix_build.c
@@ +3059,5 @@
> +        }
> +
> +        if ((!trusted && !state->buildConstants.trustOnlyUserAnchors) ||
> +            !state->buildConstants.anchors ||
> +            !state->buildConstants.anchors->length) {

I see that the code above is:

        if (state->buildConstants.anchors &&
            state->buildConstants.anchors->length) {

So your
            !state->buildConstants.anchors ||
            !state->buildConstants.anchors->length
is the "else" in the original code.

Please ignore my previous comment.

	Ryan Sleevi Assignee
	Comment 10 • 11 years ago

Comment on attachment 748958 [details] [diff] [review]
Updated patch to use enum instead of bools

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

::: lib/libpkix/include/pkix_pl_pki.h
@@ +1492,5 @@
> + * PKIX_PL_Cert_IsCertTrusted
> + */
> +typedef enum PKIX_PL_TrustAnchorModeEnum {
> +        /* Indicates trust anchors should be ignored; only the underlying
> +         * platform's trust settings should be used.

This is the libpkix abstraction term (eg: all the types prefixed with PL_)

In this case, yes, it means NSS.

The term "underlying platform" is consistent with the existing docs for PKIX_PL_Cert_IsCertTrusted.

@@ +1506,5 @@
> +        /* Indicates that ONLY trust anchors should be considered - that is,
> +         * do not consult the underlying platform.
> +         * Note: If the underlying platform supports marking a certificate as
> +         *       explicitly untrustworthy, explicitly configured trust anchors
> +         *       MAY be ignored/rejected.

Correct.

This is merely documenting how NSS already exists - which is, if a cert is blacklisted in nssckbi, there is *no* way to force it back on, even via Trust Anchors.

This hasn't been an issue to date - and you could argue it's desirable (eg: it guarantees NSS has a way to blacklist certs), but that's the reality of today's behaviour.

I thought about clarifying the comment to indicate "do not consult the underlying platform to see if a certificate is trusted, but consult it to see if it's distrusted", but I think that still creates confusion (since some might assume distrusted/trusted are a boolean set, rather than a tri-state)

::: lib/libpkix/pkix/top/pkix_build.c
@@ +3059,5 @@
> +        }
> +
> +        if ((!trusted && !state->buildConstants.trustOnlyUserAnchors) ||
> +            !state->buildConstants.anchors ||
> +            !state->buildConstants.anchors->length) {

It's a giant inconsistent mess.

This function stays consistent with lines 3050/3051, which directly access buildConstants.anchors

Line 3246/3247, however, uses PKIX_List_GetLength to poke at (what eventually becomes) buildConstants.anchors, which it uses to numAnchors.

I tried to follow local style here, even though it's inconsistent.

	Ryan Sleevi Assignee
	Comment 11 • 11 years ago

Comment on attachment 748958 [details] [diff] [review]
Updated patch to use enum instead of bools

https://hg.mozilla.org/projects/nss/rev/7033d1286a5f