bit_string and bit_string_flags decoders crash on some invalid inputs

[NathanReb]

I recently stumbled upon a bug. A bit_string or a bit_string_flag decoder seem to fail on some invalid inputs with an Invalid_argument "Array.init" instead of returning a parsing error. Running those simple examples should reproduce the issue:

Asn.(decode (codec ber S.bit_string)) (Cstruct.of_hex "030101");;

or

Asn.(decode (codec ber (S.bit_string_flags (List.init 100 (fun i -> i, i))))) (Cstruct.of_hex "030101");;

The crash seems to happen when the first octet specifying the number of insignificant bits in the last octet has an inconsistent value per se or given the rest of the bit string.

Trying to decode 0x030101 will fail same as 0x03036a0303.

Note that the bit_string_cs isn't affected by the bug in the sense that it doesn't result in a crash but just returns whatever the bit string contains without the first octet encoding the insignificant bits.
This might not be the best behaviour. I'm guessing most people use the bit_string_cs in places where the first octet is always 0x00 such as in X509 SubjectPublicKeyInfo so they shouldn't be affected but it does seem a bit unsafe to just discard the first octet in those cases if it's not 0x00.

I guess a decent alternative would be to add an (int * Cstruct.t) Asn.S.t with the int being the number of insignificant bits in the last octet of the Cstruct.t. bit_string_cs could then be built on top of that and check that the first element is 0.

I'm of course happy to help by providing any further detail you'd need or even a patch.

[NathanReb]

Thanks for the fix! Just tested it and it works like a charm!

[pqwy]

The range of unused bits is a pure bug:

X.690:

8.6.2.2 The initial octet shall encode, as an unsigned binary integer with bit 1 as the least significant bit, the number of unused bits in the final subsequent octet. The number shall be in the range zero to seven.
8.6.2.3 If the bitstring is empty, there shall be no subsequent octets, and the initial octet shall be zero.

Fixed here.

The issue of partial last byte is more subtle. The canonical (and accurate) representation of bit strings is bool array, while the cstruct projection is just a convenience. This masks away inconsequential differences in the encoding, but the user still chose an octet-based representation.

Of course the real problem is a lack of a proper custom type without the heroic 64-fold size blowup of bool array, which I'll be adding later.

Thanks for the report and the handy repro!

[hannesm]

thanks for the report, this has been fixed and is now part of the 0.2.1 release.