Fix panic in the JER OctetString decoder if the string isn't even length

[decryphe]

If the OctetString isn't of even length in the JSON file, the decoder panics with an index out of bounds error.

This PR introduces a bounds check, such that the index access octet_string[i..=i + 1] on line 613 doesn't panic on malformed input. Sample octet string triggering this panic: "!".

[Nicceboy]

Would you like adding a test case? That helps avoiding any regressions, especially if there is any refactoring coming related to performance optimizations.

[decryphe]

Would you like adding a test case? That helps avoiding any regressions, especially if there is any refactoring coming related to performance optimizations.

Sure, can do, I'll amend this in the coming days.

[decryphe]

Okay, while implementing the tests for decoding OctetString in the JER decoder, I noticed that constraints (e.g. size) are not checked at all. Is this intentional?

This also means, that something JER-encoded may not always round-trip without errors via some other encoding. Specifically I use rasn to convert an UPER encoded payload (sent via a bandwidth constrained LPWAN) to JER (for easier consumption), and back, which is why I noticed this.

The Overview of JER doesn't actually mention constraints. It's also not clear on whether non-hex characters should just ignored or rejected.

[Nicceboy]

Okay, while implementing the tests for decoding OctetString in the JER decoder, I noticed that constraints (e.g. size) are not checked at all. Is this intentional?

This also means, that something JER-encoded may not always round-trip without errors via some other encoding. Specifically I use rasn to convert an UPER encoded payload (sent via a bandwidth constrained LPWAN) to JER (for easier consumption), and back, which is why I noticed this.

The Overview of JER doesn't actually mention constraints. It's also not clear on whether non-hex characters should just ignored or rejected.

Constraint should not affect the encoding output in JER, while we could add still error checks and some other performance improvements based on that.

You can find constraint definitions from the official standard

7.2 Constraints
NOTE – The fact that some ASN.1 constraints may not be JER-visible for the purposes of encoding and decoding does not in any
way affect the use of such constraints in the handling of errors detected during decoding, nor does it imply that values violating
such constraints are allowed to be transmitted by a conforming sender. However, this Recommendation | International Standard
makes no use of such constraints in the specification of encodings.
7.2.1 In general, the constraint on a type will consist of individual constraints combined using some or all of set
arithmetic, contained subtype constraints and serial application of constraints.
The following constraints are JER-visible:
a) b) non-extensible size constraints on bitstring types;
non-extensible single value constraints on real types where the single value is either plus zero or minus
zero or one of the special real values PLUS-INFINITY, MINUS-INFINITY and NOT-A-NUMBER;
c) d) non-extensible single value constraints and value range constraints on the base of a real type;
an inner type constraint that applies a non-extensible single value constraint or value range constraint to
the base of a real type;
e) f) a contents constraint with CONTAINING but without ENCODED BY;
a contained subtype constraint in which the constraining type carries a JER-visible constraint.
7.2.2 All other constraints are not JER-visible. In particular, the following constraints are not JER-visible:
a) constraints that are expressed in human-readable text or in ASN.1 comment;
b) variable constraints (see Rec. ITU-T X.683 | ISO/IEC 8824-4, clauses 10.3 and 10.4);
c) user-defined constraints (see Rec. ITU-T X.682 | ISO/IEC 8824-3, 9.1);
d) table constraints (see Rec. ITU-T X.682 | ISO/IEC 8824-3);
e) component relation constraints (see Rec. ITU-T X.682 | ISO/IEC 8824-3, 10.7);
f) constraints whose evaluation is textually dependent on a table constraint or a component relation constraint
(see Rec. ITU-T X.682 | ISO/IEC 8824-3);
g) extensible subtype constraints;
h) size constraints applied to a character string or octet string type;
i) single value subtype constraints applied to a character string type;
j) permitted alphabet constraints;
k) pattern constraints;
l) value and value range constraints on integer types;
m) constraints on real types except those specified in clause 7.2.1 b) and c);
n) constraints on the time type and on the useful and defined time types;
o) inner type constraints except those specified in clause 7.2.1 d);
p) constraints on the useful types

There should no be relationship between other codecs. OctetString allows any byte. If you encode with UPER, and decode it back with UPER, the data structure is similar and you can re-encode it with JER as well without losing values.

[decryphe]

Perfect, then I understood it right!

Also, while writing the test, I noticed it panics for multi-byte chars as well - I'll replace that part entirely and also check if the BitString isn't equally broken, it looks suspiciously similar.

[decryphe]

Updated, this also fixes issues with multi-byte characters, both for OctetString and BitString, plus adds lots of tests.

I believe I have covered the cases of excess bytes in BitStrings correctly, by rejecting JER with actual length being more than one byte too long.

[XAMPPRocky]

Thank you for your PR! There's some clippy lints that need fixing.

[decryphe]

Fixed and rebased.

[XAMPPRocky]

still another 😄

[decryphe]

Running cargo clippy doesn't yield any other lints using the given toolchain cargo 1.84.0-beta.3 (66221abde 2024-11-19). What did I miss?

[Nicceboy]

You likely need to run cargo clippy --all-targets to include tests as well.

[decryphe]

Yeah, that yielded another warning. Fixed, amended, pushed.

Will you be creating a CONTRIBUTE.md or similar with some hints and maybe a checklist what not to forget?

[XAMPPRocky]

That's a good idea, can you make an issue for that so we don't forget. 🙂

[XAMPPRocky]

Thank you for your PR!