Closed Bug 1018633 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

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.
The tests in bug 1001188 will serve as regression tests for this patch.
Attachment #8432152 - Flags: review?(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.
Attachment #8432152 - Flags: review?(cviecco) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/9b0804cee835
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.