1018633 - Simplify the max cert chain length check code in mozilla::pkix and make it more efficient

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Once upon a time there might have been a reason that the cert chain length maintenance was split between BuildForward and BuildForwardInner but now there is no reason for it to be like that. The code is simpler, clearer, and slightly more efficient with it all in one spot.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: simplify-cert-chain-length-checks.patch

The tests in bug 1001188 will serve as regression tests for this patch.

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8432152 [details] [diff] [review]
simplify-cert-chain-length-checks.patch

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

r+ with comment addressed

::: security/pkix/lib/pkixbuild.cpp
@@ +253,5 @@
>  
> +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    // Avoid stack overflows and poor performance by limiting cert chain
> +    // length.
> +    if (subCACount >= 6) {

Please make "6" a named const, also why the change fro 7 to 6?

@@ +256,5 @@
> +    // length.
> +    if (subCACount >= 6) {
> +      return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
> +    }
> +    ++subCACount;

// begin micro rant (ignore this section)
preincrements OK but my joda comparisons not OK (CONST == var), makes me sad
// end micro rant

@@ +258,5 @@
> +      return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
> +    }
> +    ++subCACount;
> +  } else {
> +    PR_ASSERT(subCACount == 0);

Like this addition.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #2)
> > +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> > +    // Avoid stack overflows and poor performance by limiting cert chain
> > +    // length.
> > +    if (subCACount >= 6) {
> 
> Please make "6" a named const, also why the change fro 7 to 6?

I named it MAX_SUBCA_COUNT;

We used 7 when we were checking AFTER incrementing subCACount. Now, we use 6 because we're checking BEFORE incrementing subCACount.

> // begin micro rant (ignore this section)
> preincrements OK but my joda comparisons not OK (CONST == var), makes me sad
> // end micro rant

I do not understand what you mean here. If you mean that you prefer (CONST == var) over (var == CONST) so the compiler will reject (CONST = var) where it would accept (var = CONST), I'd say that it doesn't matter because with the warning levels we're using + warnings-as-errors enabled, the compiler (some of them, anyway) will reject (var = CONST) and it will ALSO reject (var = var), which the (CONST = var) style can't help with. So, I think we're OK. If you mean something else, perhaps start an email thread or file a bug about it.

Thanks for the quick reviews today!

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0804cee835

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b0804cee835