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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: OID-type.patch

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.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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!

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bca81260db4

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bca81260db4