Closed Bug 1029364 Opened 6 years ago Closed 6 years ago

Centralize certificate version parsing into mozilla::pkix::BackCert::Init

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(2 files)

See the patch. Basically, this is cleaning up a little bit of messiness and synchronizing the behavior of mozilla::pkix::der::OptionalVersion and the version parsing done for certificates, in preparation for switching to using mozilla::pkix::der::OptionalVersion when parsing certificates.
Attachment #8444989 - Flags: review?(cviecco)
Comment on attachment 8444989 [details] [diff] [review]
centralize-version-parsing.patch

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

LGTM.

::: security/pkix/lib/pkixbuild.cpp
@@ +64,5 @@
> +    // Explicit encoding of v1 is not allowed. We do not support any other
> +    // version except v3.
> +    return Fail(RecoverableError, SEC_ERROR_BAD_DER);
> +  }
> +

This whole block was surprisingly hard to parse, why not?

switch(nssCert->version.len) {
  case 0: version = der::Version::v1; break;
  case 1: switch (nssCert->version.data[0]) { 
            case (static_cast<uint8_t>(der::Version::v2)):
               version = der::Version::v2;
               break;
            case (static_cast<uint8_t>(der::Version::v3)):
               version = der::Version::v3;
               break;
            default:
               return Fail(RecoverableError,SEC_ERROR_BAD_DER);
           }
           break;
  default:
     return Fail(RecoverableError, SEC_ERROR_BAD_DER);
}
Attachment #8444989 - Flags: review?(cviecco) → review+
Comment on attachment 8444989 [details] [diff] [review]
centralize-version-parsing.patch

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

::: security/pkix/lib/pkixbuild.cpp
@@ +64,5 @@
> +    // Explicit encoding of v1 is not allowed. We do not support any other
> +    // version except v3.
> +    return Fail(RecoverableError, SEC_ERROR_BAD_DER);
> +  }
> +

IMO, that change isn't much clearer, even though we generally prefer switch. The advantages of the original version are that (1) the most common case is handled first, and (2) there is only one error case instead of two. I think (2) is especially good for the readability. Regardless, very soon I will post another patch which obliterates this chunk of code anyway.

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f21e9bc729a
sorry had to back this out in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=152f6b19ad9c since one of this 2 csets seems to caused a bustage in device builds and B2g Emulator like https://tbpl.mozilla.org/php/getParsedLog.php?id=42422444&tree=Mozilla-Inbound
(In reply to Carsten Book [:Tomcat] from comment #3)
> sorry had to back this out in
> https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&onlyunstarred=1&rev=152f6b19ad9c since one of this 2 csets seems to
> caused a bustage in device builds and B2g Emulator like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42422444&tree=Mozilla-
> Inbound

Thanks for backing out my patch. I didn't scroll down to notice the build failures on my try run.

Here is the new try run with the fix:
https://tbpl.mozilla.org/?tree=Try&rev=9f42b04d5307
Attachment #8446106 - Flags: review?(cviecco) → review+
https://hg.mozilla.org/mozilla-central/rev/bfd435db3f9e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1031338
Depends on: 1031366
Duplicate of this bug: 982783
You need to log in before you can comment on or make changes to this bug.