Closed Bug 1003290 Opened 10 years ago Closed 10 years ago

Type of length parameter of mozilla::pkix::der::OID is too wide (uint16_t instead of uint8_t)

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(1 file)

Attached patch OID-type.patchSplinter Review
inline Result
ExpectTagAndLength(Input& input, uint8_t expectedTag, uint8_t expectedLength)

...

template <uint8_t Len>
Result
OID(Input& input, const uint8_t (&expectedOid)[Len])
{
  if (ExpectTagAndLength(input, OIDTag, Len) != Success) {
    return Failure;
  }

  return input.Expect(expectedOid, Len);
}

Len should have type uint8_t since ExpectTagAndLength's expectedLength parameter is uint8_t.
Attachment #8414602 - Flags: review?(dkeeler)
Comment on attachment 8414602 [details] [diff] [review]
OID-type.patch

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

I'm fine with this, but why does ExpectTagAndLength take a uint8_t while most of the other DER functions take a uint16_t? This means we can't have an OID of length greater than 255 or use ExpectTagAndLength for something longer than 255 bytes. It's not a big deal, but we may just be changing this in the future.
Attachment #8414602 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) from comment #1)
> I'm fine with this, but why does ExpectTagAndLength take a uint8_t while
> most of the other DER functions take a uint16_t? This means we can't have an
> OID of length greater than 255 or use ExpectTagAndLength for something
> longer than 255 bytes. It's not a big deal, but we may just be changing this
> in the future.

Basically, we only use ExpectTagAndLength with things we know to be very small, the extra code to support lengths larger than 127 bytes would be dead code and would make the function less efficient.

The reason for the extreme emphasis on efficiency in mozilla::pkix::der will become more evident if/when we write our own code to enforce name constraints, because we'll likely be reparsing the same data over and over again. Also, I wanted to minimize the chance of performance regressions.

If I were to write ExpectTagAndLength today, I would probably do things a different way, but since it's already that way and probably maximally efficient I don't see much reason to change it. Let me know if you feel otherwise.

Thanks for the quick review!
https://hg.mozilla.org/mozilla-central/rev/9bca81260db4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.