feat(ciborium-ll): fail decoding invalid two-byte encodings of simple values

[emonadeo]

Unblocks #137.

In context of simple values, the spec states:

An encoder MUST NOT issue two-byte sequences that start with 0xf8 (major type 7, additional information 24) and continue with a byte less than 0x20 (32 decimal).¹

While ciborium adheres to this, ciborium-ll does not.

let bytes = hex::decode("f800").unwrap();
let mut decoder = Decoder::from(&bytes[..]);
decoder.pull() // Is Ok(Simple(0)), but should be Err()

This PR adds a check that catches it in ciborium-ll.

RFC: Fail encoding Simple(24) .. Simple(30) and Simple(31)?

This PR only adds a check when decoding a.k.a. calling decoder.pull().
However vice-versa encoding Simple(24) .. Simple(30) should not be possible.

• Simple(24) is irrepresentable
• Simple(25) .. Simple(27) are floats
• Simple(28) .. Simple(30) are reserved and not considered well-formed

It's also worth noting that Simple(31) and Break are equivalent. Should both be allowed or should Simple(31) error in favor of Break?

Footnotes

1. https://www.rfc-editor.org/rfc/rfc8949.html#section-3.3-5 ↩

[rjzak]

LGTM

[rjzak]

Please reword the commit to use the -s flag to sign, or add the Signed-off-by: field to pass the DCO check.

[emonadeo]

Whoops, forgot about that. Force pushed.

[npmccallum]

@emonadeo @rjzak I don't think this patch was ready to merge. There are clearly unresolved questions in the comments in the patch.

I'm also not understanding why this patch is necessary. The spec says that an encoder must not produce those bytes. But, if I understand this patch correctly, it stops the decoder from decoding these bytes. Is that correct?

[rjzak]

@emonadeo @rjzak I don't think this patch was ready to merge. There are clearly unresolved questions in the comments in the patch.

My mistake. Shall I roll it back or let @emonadeo fix?

[emonadeo]

I don't think this patch was ready to merge. There are clearly unresolved questions in the comments in the patch.

I forgot about that comment I created. But I am a bit surprised this got merged without addressing the RFC about the encoding illegal Simple headers.

I'm also not understanding why this patch is necessary. The spec says that an encoder must not produce those bytes. But, if I understand this patch correctly, it stops the decoder from decoding these bytes. Is that correct?

Yes, that is correct.

Ciborium can decode cbor that was produced by different means.
E.g. Maliciously crafted CBOR submitted to an HTTP API that is implemented in Rust and uses Ciborium to decode it can run into this edge-case. In this case it should be pretty harmless, but if Simple values receive representation like in #137, the unittests asserting that Simple(0..15) aren't two-byte will no longer pass.

The unittests could alternatively be removed, but I think they are correct.
My thinking is, if something must not be encoded, it must not exist, and thus may not be decoded.

[npmccallum]

@emonadeo There is a long standing practice in codecs to be strict during encoding and liberal during decoding (so long as it doesn't create a security risk). This crate follows that methodology. This patch implements a requirement that goes above and beyond what the standard requires and goes against the methodology of this crate to be liberal during decoding.

The other problem is that ciborium-ll intends to implement syntactic checking but not semantic checking. That's what higher level code does. This patch adds semantic checking at the wrong layer. As I see it, ciborium is where semantic evaluation is performed. And we do error at that layer. Applications shouldn't generally use ciborium-ll unless they require a very minimal approach (i.e. embedded applications). If they do, they are responsible to validate semantic meaning since ciborium-ll only does syntactic evaluation.

But I want to be very clear that if there is justification to break that structure, I'm happy to be convinced.

[npmccallum]

@rjzak Let's rollback since I don't yet see justification for this approach.

[emonadeo]

But I want to be very clear that if there is justification to break that structure, I'm happy to be convinced.

There isn't besides unittest happy.

There is a long standing practice in codecs to be strict during encoding and liberal during decoding (so long as it doesn't create a security risk). This crate follows that methodology.

This PR definitely doesn't align with that. Reconsidering how the spec is worded it's probably intended.

+1 on rolling back. Much appreciate the feedback.

[rjzak]

@rjzak Let's rollback since I don't yet see justification for this approach.

What's the best approach? I was thinking of a rebase and drop this commit and force push. Is there a better way?

[rjzak]

Done.