Closed Bug 921887 Opened 8 years ago Closed 7 years ago

Add minimal DER decoder to insanity::pkix

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #878932 +++

Although insanity::pkix uses NSS for most DER decoding, there are some structures like the isCritical flag of extensions, the version field of certificates, and OCSP responses, that need DER decoding done outside of NSS.
No longer blocks: 921888
Assignee: nobody → brian
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla29
It would be a great idea to write some GTest-based unit tests for this. However, because I have to go on this last-minute trip, I will not have time to do that this week. Is this something that you can do? If not, perhaps we can r+ with with the understanding that we'll write and check in the unit tests before this code gets to Mozilla-Beta stage.
Attachment #8363486 - Flags: review?(dkeeler)
No longer blocks: 878932
Attachment #8363486 - Attachment description: bug-921887.patch → Part 1: Add parsers for generic ASN.1 DER types
Comment on attachment 8363486 [details] [diff] [review]
Part 1: Add parsers for generic ASN.1 DER types

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

Looks good.

::: security/insanity/lib/pkixder.h
@@ +20,5 @@
> +
> +#include "stdint.h"
> +#include "prerror.h"
> +#include "prlog.h"
> +#include "secerr.h"

nit: sorting includes

@@ +29,5 @@
> +{
> +   UNIVERSAL = 0 << 6,
> +// APPLICATION = 1 << 6,
> +   CONTEXT_SPECIFIC = 2 << 6,
> +// PRIVATE = 3 << 6

I assume the commented-out ones are not going to be used, ever? Maybe it would be good to have a comment saying so.

@@ +115,5 @@
> +  const uint8_t* input;
> +  const uint8_t* end;
> +
> +  Input(const Input&); /* = delete */
> +  void operator=(const Input&); /* = delete */

I think there's a MOZ_DELETE or something...

@@ +125,5 @@
> +  PR_ASSERT((expectedTag & 0x1F) != 0x1F); // high tag number form not allowed
> +  PR_ASSERT(expectedLength < 128); // must be a single-byte length
> +
> +  uint16_t tagAndLength;
> +  if (input.Read(tagAndLength)) {

This would be more clear if you added the implied " != Success".

@@ +154,5 @@
> +
> +inline Result
> +Boolean(Input& input, /*out*/ bool& value)
> +{
> +  if (ExpectTagAndLength(input, BOOLEAN, 1)) {

!= Success?

@@ +159,5 @@
> +    return Failure;
> +  }
> +
> +  uint8_t intValue;
> +  if (input.Read(intValue)) {

!= Success?

@@ +165,5 @@
> +  }
> +  switch (intValue) {
> +    case 0: value = false; return Success;
> +    case 0xFF: value = true; return Success;
> +    default: PR_SetError(SEC_ERROR_BAD_DER, 0);

nit: maybe have the PR_SetError on a separate line from the label
Attachment #8363486 - Flags: review?(dkeeler) → review+
Comment on attachment 8363503 [details] [diff] [review]
Part 2: Add parser for optional X.509 version field

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

Looks good. I would like to see Peek, though.

::: security/insanity/lib/pkixder.h
@@ +180,5 @@
> +// X.509 Certificate and OCSP ResponseData both use this
> +// "[0] EXPLICIT Version DEFAULT <defaultVersion>" construct, but with
> +// different default versions.
> +inline Result
> +OptionalVersion(Input & input, /*out*/ uint8_t & version)

Input&, uint8_t&

@@ +183,5 @@
> +inline Result
> +OptionalVersion(Input & input, /*out*/ uint8_t & version)
> +{
> +  const uint8_t tag = CONTEXT_SPECIFIC | CONSTRUCTED | 0;
> +  if (!input.Peek(tag)) {

Looks like Peek didn't make it into either this or the last patch.

@@ +187,5 @@
> +  if (!input.Peek(tag)) {
> +    version = v1;
> +    return Success;
> +  }
> +  if (ExpectTagAndLength(input, tag, 3)) {

!= Success

@@ +190,5 @@
> +  }
> +  if (ExpectTagAndLength(input, tag, 3)) {
> +    return Failure;
> +  }
> +  if (ExpectTagAndLength(input, INTEGER, 1)) {

!= Success

@@ +193,5 @@
> +  }
> +  if (ExpectTagAndLength(input, INTEGER, 1)) {
> +    return Failure;
> +  }
> +  if (input.Read(version)) {

!= Success
Attachment #8363503 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) from comment #3)
> I think there's a MOZ_DELETE or something...

I am trying to avoid using MOZ_* in the insanity::pkix code so I don't make this change.

> This would be more clear if you added the implied " != Success".

I agree. I made these changes.

The reason I avoided the " != Success" originally was because, in the OCSP parser that is going to be posted soon, the " != Success" causes many more lines to wrap, and that seemed wasteful. But, I agree that it is clearer the way you suggest, so I will change all of this.

I made the other changes you suggested.
Attached patch bug-921887-p2.patch (obsolete) — Splinter Review
David, here's the rest of the DER decoder, including OptionalVersion. Let me know what needs more explanation and I will add comments. The "Nested" type stuff will be easier to understand when reading the OCSP parser implementation based on it.
Attachment #8363503 - Attachment is obsolete: true
Attachment #8367037 - Flags: review?(dkeeler)
Comment on attachment 8367037 [details] [diff] [review]
bug-921887-p2.patch

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

Looks good.

::: security/insanity/lib/pkixder.h
@@ +299,5 @@
> +inline Result
> +NestedOf(Input& input, uint8_t outerTag, uint8_t innerTag,
> +         EmptyAllowed mayBeEmpty, Decoder decoder)
> +{
> +  // TODO: check producedAt validity against context.time

This comment seems to be unrelated to this code.

@@ +307,5 @@
> +  }
> +
> +  Input inner;
> +  if (input.Skip(responsesLength, inner) != Success) {
> +    return Failure;

It's not clear to me when we should be returning Failure vs. Fail(SEC_ERROR_BAD_DER) - maybe document this?

@@ +414,5 @@
> +
> +  // Check for overly-long encodings.
> +  if (value.len > 1) {
> +    if ((value.data[0] == 0x00 && (value.data[1] & 0x80) == 0) ||
> +        (value.data[0] == 0xff && (value.data[1] & 0x80) != 0)) {

I think I understand the first check here (if data[0] is 0, then data[1] must have its highest bit set, otherwise it could just be represented as data[1..]), but I don't understand the second check - why does the highest bit of data[1] need to be unset if data[0] is 0xff?
Attachment #8367037 - Flags: review?(dkeeler) → review+
David, I removed the unnecessary comment and I added a comment about the overly-long integer encodings in insanity::der::Integer:

  // Check for overly-long encodings. If the first byte is 0x00 then the high
  // bit on the second byte must be 1; otherwise the same *positive* value
  // could be encoded without the leading 0x00 byte. If the first byte is 0xFF
  // then the second byte must NOT have its high bit set; otherwise the same
  // *negative* value could be encoded without the leading 0xFF byte.
Attachment #8367037 - Attachment is obsolete: true
Attachment #8371073 - Flags: review+
Thanks for the reviews. Folded both parts together:

https://hg.mozilla.org/integration/mozilla-inbound/rev/52194466b166
Target Milestone: mozilla29 → mozilla30
https://hg.mozilla.org/mozilla-central/rev/52194466b166
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #8)
> Created attachment 8371073 [details] [diff] [review]
> bug-921887-p2.patch
> 
> David, I removed the unnecessary comment and I added a comment about the
> overly-long integer encodings in insanity::der::Integer:
> 
>   // Check for overly-long encodings. If the first byte is 0x00 then the high
>   // bit on the second byte must be 1; otherwise the same *positive* value
>   // could be encoded without the leading 0x00 byte. If the first byte is
> 0xFF
>   // then the second byte must NOT have its high bit set; otherwise the same
>   // *negative* value could be encoded without the leading 0xFF byte.

Great - thanks.
Comment on attachment 8363486 [details] [diff] [review]
Part 1: Add parsers for generic ASN.1 DER types

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8363486 - Flags: approval-mozilla-aurora?
Comment on attachment 8371073 [details] [diff] [review]
bug-921887-p2.patch

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8371073 - Flags: approval-mozilla-aurora?
Comment on attachment 8363486 [details] [diff] [review]
Part 1: Add parsers for generic ASN.1 DER types

Uplifted granted to the patches relative to the new feature: "Add insanity::pkix as certificate verification option"
Lukas and I discussed with Brian and we think it is important to have this feature for 29 (but disabled by default).
It is early in the aurora process and they have plenty of tests for these feature (and to make sure that the current behaviors are still performing correctly).
Attachment #8363486 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8371073 [details] [diff] [review]
bug-921887-p2.patch

Uplifted granted to the patches relative to the new feature: "Add insanity::pkix as certificate verification option"
Lukas and I discussed with Brian and we think it is important to have this feature for 29 (but disabled by default).
It is early in the aurora process and they have plenty of tests for these feature (and to make sure that the current behaviors are still performing correctly).
Attachment #8371073 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.