der, x509-cert: fix handling of negative integers #831

woodruffw · 2023-01-02T06:24:44Z

This is my first attempt at a fix for the problem in #826.

I tried going with a signed: bool field at first, but ran into a lot of friction: I ended up having to essentially duplicate all of uint.rs in int.rs. So I scrapped that, and went with @baloo's suggestion of an enum: IntRef now varies over SintRef and UintRef, and provides a uniform interface by forwarding to each.

Advantages to this approach:

Less churn: I only had to make a small number of changes to int.rs to accommodate these changes, mostly around refusing to accept positive numbers and making sure that we handle the sign byte correctly.
Construction correctness in the API: SintRef (now poorly named) can be used to guarantee construction of a negative integer, similar to how UintRef can only construct positive integers.

Disadvantages:

Sint and SintRef are poorly named, since they only represent negative integers (and zero). I'm open to naming suggestions here; I left them this way just to get an initial PR up.
There's still some dual state around zero: Int::Unsigned(0) != Int::Signed(0). This relates to the first problem, and I can fix it as part of naming Sint something better (like Negative) and forbidding zero by construction.

Notably, that second disadvantage would still exist with the signed: bool approach, since care would need to be taken to ensure a canonical signed: false value when building Int(0).

cc @baloo

Fixes #826.

woodruffw · 2023-01-02T06:28:53Z

(I can also confirm that these changes allow me to successfully parse and re-encode all 447 X.509 certs in the Windows trust list 🙂)

woodruffw · 2023-01-02T06:36:00Z

x509-cert/src/serial_number.rs

 pub fn new(bytes: &[u8]) -> Result<Self> {
- let inner = Int::new(bytes)?;
+ let inner = Uint::new(bytes)?;

 if inner.len() > SerialNumber::MAX_LEN {
 return Err(ErrorKind::Overlength.into());
 }

- Ok(Self { inner })
+ Ok(Self {
+ inner: inner.into(),
+ })


With this change, users will no longer be able to directly build a new negative serial number: the only way they'll be able to end up with a SerialNumber with a negative internal representation is by loading an existing one.

tarcieri · 2023-01-02T14:11:23Z

I very strongly think signed integers should just use the 2's complement representation and not be any sort of enum which seems to be adding a lot of incidental complexity.

2's complement already solves the problem of "how do we store an integer which can be negative or positive" without resorting to an enum.

The INTEGER type is natively 2's complement, and the way "unsigned integers" are stored with them is to use the 2's complement representation but always add a leading 0 in the event the high bit of the MSB is set.

With that said, another approach is making UInt a newtype for Int which upholds that invariant and disallows the most significant bit of the most significant byte from being set, which would eliminate duplication.

I'm happy to take a crack at this if you're having trouble.

woodruffw · 2023-01-02T18:17:09Z

That makes sense, thanks! I agree about the complexity (and I'm not happy with how big this change is).

I think the thing that tripped me up is over-thinking RFC 5280's length constraint on serial numbers, when really it's ambiguous: the RFC limits serial number values to 20 octets, but doesn't say whether that's 20 with the zero-extension byte or not.

It looks like this also caused some confusion in Go's crypto/x509 (golang/go#33310), and that their decision was to allow users to build serial numbers that encode to 21 bytes under the reasoning that their unsigned representation is only 20 bytes (source ref).

The current Int type that's been merged is 2's complement internally, so I think all that really needs to be done is to (1) adjust SerialNumber so that it can only be constructed with positive integers, and (2) re-think its internal length check (in particular, we probably need to allow a length of 21 bytes iff the highest byte is a zero extension).

Does that make sense?

tarcieri · 2023-01-02T18:20:47Z

Yeah sure. The existing Int/IntRef types are indeed already using 2's complement.

In parallel I can look at composing Uint/UintRef in terms of Int/IntRef, which should cut down on duplication.

woodruffw · 2023-01-02T18:28:30Z

Sounds good, I can make those changes this afternoon/evening.

This reverts commit 130f52c.

Take two.

woodruffw · 2023-01-03T05:52:02Z

der/src/asn1/integer/int.rs

 #[inline]
-pub(super) fn encoded_len(bytes: &[u8]) -> Result<Length> {
+pub(super) fn negative_encoded_len(bytes: &[u8]) -> Result<Length> {


N.B.: I renamed this to emphasize what this function is actually doing, as currently implemented: it doesn't calculate the encoded length of any signed integer represented as bytes, but only negative signed integers. Passing a positive signed integer here would result in the wrong length, since we only trim leading ones, not zeroes.

(I confirmed that the one remaining callsite for this only calls it on negative integers, and removed the other misleading callsites.)

der/src/asn1/integer/bigint.rs

woodruffw · 2023-01-03T05:57:40Z

x509-cert/src/serial_number.rs

 pub fn new(bytes: &[u8]) -> Result<Self> {
- let inner = Int::new(bytes)?;
-
- if inner.len() > SerialNumber::MAX_LEN {
+ let inner = Uint::new(bytes)?;
+
+ // The user might give us a 20 byte unsigned integer with a high MSB,
+ // which we'd then encode with 21 octets to preserve the sign bit.
+ // RFC 5280 is ambiguous about whether this is valid, so we limit
+ // `SerialNumber` *encodings* tp 20 bytes or fewer while permitting
+ // `SerialNumber` *decodings* to have up to 21 bytes below.
+ if inner.value_len()? > SerialNumber::MAX_LEN {
 return Err(ErrorKind::Overlength.into());
 }

- Ok(Self { inner })
+ Ok(Self {
+ inner: inner.into(),
+ })
 }


This ensures that we never create a new negative serial number, but the way it does that is potentially surprising because of how the Int::new API (unchanged) works. In particular, Int::new assumes that its input is positive, and encodes it correctly to enforce this assumption.

I've updated the tests below to show this behavior.

(This arguably violates POLA, since we're changing the serial number given to us by the user. But it's how the current Int::new API behaves, and I didn't want to make changes to that before putting something initial up here 🙂)

curious: what does POLA stands for?

Principle of least astonishment

Yep! Sorry for the acronym abuse.

By POLA in this case I meant that users might consider it astonishing that the serial they provide doesn't end up in the cert, even if the result is technically more correct.

tarcieri · 2023-01-19T17:59:56Z

@woodruffw can you rebase?

woodruffw · 2023-01-19T18:16:03Z

@woodruffw can you rebase?

Yep, can do this afternoon!

woodruffw · 2023-01-20T05:20:49Z

Okay, should be up-to-date again. It looks like a bunch of TryFrom<AnyRef> impls were removed with #834, so I dropped the one here as well.

tarcieri · 2023-01-20T17:40:34Z

Thanks!

der, x509-cert: fix handling of negative integers

130f52c

woodruffw marked this pull request as ready for review January 2, 2023 06:30

woodruffw commented Jan 2, 2023

View reviewed changes

woodruffw added 2 commits January 2, 2023 23:41

Revert "der, x509-cert: fix handling of negative integers"

b9ee5e4

This reverts commit 130f52c.

der, x509-cert: clean up negative int handling

054d0ca

Take two.

woodruffw commented Jan 3, 2023

View reviewed changes

der/src/asn1/integer/bigint.rs Show resolved Hide resolved

woodruffw commented Jan 3, 2023

View reviewed changes

serial_number: typo

234f858

tarcieri mentioned this pull request Jan 19, 2023

Bump SerialNumber max length limit #848

Closed

baloo mentioned this pull request Jan 20, 2023

der, x509-cert: fix handling of negative integers #852

Closed

Merge remote-tracking branch 'upstream/master' into ww/fix-int

5652f96

x509-cert: fixup merge

20f0dd8

tarcieri approved these changes Jan 20, 2023

View reviewed changes

tarcieri merged commit 8ab2bb0 into RustCrypto:master Jan 20, 2023

woodruffw deleted the ww/fix-int branch January 20, 2023 17:56

mikemcdermottredjack mentioned this pull request Jan 25, 2023

CryptoPAN #861

Open

tarcieri mentioned this pull request Mar 19, 2023

x509-cert v0.2.0 #934

Merged

tarcieri mentioned this pull request Apr 4, 2023

x509-cert/der regression #978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

der, x509-cert: fix handling of negative integers #831

der, x509-cert: fix handling of negative integers #831

woodruffw commented Jan 2, 2023 •

edited

Loading

woodruffw commented Jan 2, 2023

woodruffw Jan 2, 2023

tarcieri commented Jan 2, 2023

woodruffw commented Jan 2, 2023

tarcieri commented Jan 2, 2023

woodruffw commented Jan 2, 2023

woodruffw Jan 3, 2023

woodruffw Jan 3, 2023

woodruffw Jan 3, 2023

baloo Jan 4, 2023

tarcieri Jan 4, 2023

woodruffw Jan 4, 2023

tarcieri commented Jan 19, 2023

woodruffw commented Jan 19, 2023

woodruffw commented Jan 20, 2023

tarcieri commented Jan 20, 2023

der, x509-cert: fix handling of negative integers #831

der, x509-cert: fix handling of negative integers #831

Conversation

woodruffw commented Jan 2, 2023 • edited Loading

woodruffw commented Jan 2, 2023

woodruffw Jan 2, 2023

Choose a reason for hiding this comment

tarcieri commented Jan 2, 2023

woodruffw commented Jan 2, 2023

tarcieri commented Jan 2, 2023

woodruffw commented Jan 2, 2023

woodruffw Jan 3, 2023

Choose a reason for hiding this comment

woodruffw Jan 3, 2023

Choose a reason for hiding this comment

woodruffw Jan 3, 2023

Choose a reason for hiding this comment

baloo Jan 4, 2023

Choose a reason for hiding this comment

tarcieri Jan 4, 2023

Choose a reason for hiding this comment

woodruffw Jan 4, 2023

Choose a reason for hiding this comment

tarcieri commented Jan 19, 2023

woodruffw commented Jan 19, 2023

woodruffw commented Jan 20, 2023

tarcieri commented Jan 20, 2023

woodruffw commented Jan 2, 2023 •

edited

Loading