Closed Bug 1041344 Opened 6 years ago Closed 6 years ago

Refactor CheckCertificatePolicies to remove use of mozilla::pkix::Input::MatchTLV

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(1 file, 1 obsolete file)

1. Input::MatchTLV is the only DER-specific method left in the Input class. I'd like to remove it so that Input is more clearly perceived to be a general-purpose device.

2. We've mostly changed the style we've written code that loops over sequences, so that we use mozilla::pkix::bind less and so that we can exit from the loops as soon as we find a match. I'd like this code to use the more modern conventions.

3. I'm planning to extend this code very soon to do add new features, and this refactoring helps with that. Note that I don't do "return Success" from within the do-while loop for this reason.
Attachment #8459344 - Flags: review?(cviecco)
Comment on attachment 8459344 [details] [diff] [review]
refactor-CertificatePolicies.patch

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

::: security/pkix/lib/pkixcheck.cpp
@@ +187,3 @@
>  
>  /*static*/ const CertPolicyId CertPolicyId::anyPolicy = {
> +  4, { 0x55, 0x1d, 0x20, 0x00 }

umm any way to merge these two (even with a macro?)

@@ +211,5 @@
>        requiredPolicy.numBytes > sizeof requiredPolicy.bytes) {
>      return Result::FATAL_ERROR_INVALID_ARGS;
>    }
>  
> +  bool requiredPolicyFound = requiredPolicy.IsAnyPolicy();

I would return early here.

@@ -269,4 @@
>  
> -  Input input(*encodedCertificatePolicies);
> -  if (der::NestedOf(input, der::SEQUENCE, der::SEQUENCE, der::EmptyAllowed::No,
> -                    bind(CheckPolicyInformation, _1, endEntityOrCA,

why not remove CheckPolicyInformation also? I think this is the only caller
Attachment #8459344 - Flags: review?(cviecco) → review-
Comment on attachment 8459344 [details] [diff] [review]
refactor-CertificatePolicies.patch

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

::: security/pkix/lib/pkixcheck.cpp
@@ +187,3 @@
>  
>  /*static*/ const CertPolicyId CertPolicyId::anyPolicy = {
> +  4, { 0x55, 0x1d, 0x20, 0x00 }

I'd rather keep it the way it is, especially because the anyPolicy variable is generated by DottedOIDToCode.py. I agree it isn't ideal but I don't think using a macro is going to be significantly better.

@@ +211,5 @@
>        requiredPolicy.numBytes > sizeof requiredPolicy.bytes) {
>      return Result::FATAL_ERROR_INVALID_ARGS;
>    }
>  
> +  bool requiredPolicyFound = requiredPolicy.IsAnyPolicy();

Done.

@@ -269,4 @@
>  
> -  Input input(*encodedCertificatePolicies);
> -  if (der::NestedOf(input, der::SEQUENCE, der::SEQUENCE, der::EmptyAllowed::No,
> -                    bind(CheckPolicyInformation, _1, endEntityOrCA,

CheckPolicyInformation is removed by this patch already.
Attachment #8459344 - Attachment is obsolete: true
Attachment #8461201 - Flags: review?(cviecco)
Comment on attachment 8461201 [details] [diff] [review]
refactor-CheckCertificatePolicies.patch

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

Good, r+ with comment addressed or commented in the patch

::: security/pkix/include/pkix/Input.h
@@ +216,5 @@
> +    // Normally we use EnsureLength which compares (input + len < end), but
> +    // here we want to be sure that there is nothing following the matched
> +    // bytes
> +    size_t remaining = static_cast<size_t>(end - input);
> +    if (toMatch.GetLength() != remaining) {

to be extra cautious I think we should check end >= input.
Attachment #8461201 - Flags: review?(cviecco) → review+
(In reply to Camilo Viecco (:cviecco) from comment #4)
> > +    // Normally we use EnsureLength which compares (input + len < end), but
> > +    // here we want to be sure that there is nothing following the matched
> > +    // bytes
> > +    size_t remaining = static_cast<size_t>(end - input);
> > +    if (toMatch.GetLength() != remaining) {
> 
> to be extra cautious I think we should check end >= input.

I filed bug 1048037 for checking that invariant for all the methods in the class; it doesn't make sense to do it only in this method.

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/039dd305c567
https://hg.mozilla.org/mozilla-central/rev/039dd305c567
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.