Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior of bit_string_flags #25

Closed
wiml opened this issue Apr 26, 2019 · 5 comments
Closed

Unexpected behavior of bit_string_flags #25

wiml opened this issue Apr 26, 2019 · 5 comments

Comments

@wiml
Copy link

wiml commented Apr 26, 2019

The bit_string_flags combinator has unexpected and inconvenient behavior when reading a bit string containing a bit that is not given a corresponding value in xs.

What it does right now looks like unintentional behavior: depending on the position of the unexpected bit, it might ignore it (not unreasonable), or it might insert a spurious copy of whatever the first value in the xs list was (definitely wrong).

As a user there are a few behaviors I might find useful, in different situations:

  1. Ignore unknown bits
  2. Fail the parse
  3. Return a special value, e.g. UnexpectedFlagBit 12

None of those is appropriate in all situations though. Perhaps bit_string_flags could gain an optional parameter, e.g., ?unknown:(int -> (a, Asn.error) result) which is called to handle unexpectedly set bits, with the default value if unspecified being to completely ignore them? Or perhaps ?unknown:(int -> a option) if the default behavior should be a parse failure.

@wiml wiml changed the title Unexpected behavioro of bit_string_flags Unexpected behavior of bit_string_flags Apr 26, 2019
@pqwy
Copy link
Contributor

pqwy commented May 1, 2019

Yeah, this is totally wrong. I think it should fail -- more complex logic can be achieved with a custom combinator.

But I wonder how often people have encodings with dead bits? Maybe converting it into bit_string_flags: 'a list -> 'a list asn makes more sense, where the input is taken to map to consecutive bits starting at position 0..?

@wiml
Copy link
Author

wiml commented May 1, 2019

I noticed it because my application, processing Kerberos messages, has lots of dead bits — Kerberos' flag fields have lots of gaps, including bit 0, which seems to be Reserved in every flag field. Kerberos may be unusual though.

It doesn't seem unreasonable for bit_string_flags to require a gap-less list of bits but I don't think that doing so would simplify the API: it's always possible for the sender to send a bit that isn't contained in the list (since DER can represent an arbitrarily wide bitstring), so it still has to have some behavior when that happens.

@pqwy
Copy link
Contributor

pqwy commented May 2, 2019

Ughhhh.

   KerberosFlags   ::= BIT STRING (SIZE (32..MAX))
                       -- minimum number of bits shall be sent,
                       -- but no fewer than 32

So there's at least one reasonable use-case where unconditional error does not apply...

@pqwy
Copy link
Contributor

pqwy commented May 3, 2019

The Oracle has spoken.

X.680:

22.6 The presence of a "NamedBitList" has no effect on the set of abstract values of this type. Values containing 1 bits other than the named bits are permitted.
1 bits other than the named bits are permitted.

Named bits are to be understood simply as a shorthand for bit positions, and bit_string_flags is now a shorthand for plucking out the named positions, ignoring the ones that are not named.

hannesm added a commit to hannesm/opam-repository that referenced this issue Jan 28, 2020
CHANGES:

* disallow various constructs as suggested by ITU-T Rec X.690 (by @pqwy)
  * redundant OID component forms (X.690 8.20.2)
  * redundant integer forms (X.690 8.3.2)
  * empty integer (X.690 8.3.1, reported in mirleft/ocaml-asn1-combinators#23 by @emillon)
  * constructed strings in DER
* deeper implict -> explicit over choice (follow-up to v0.2.0 entry, by @pqwy)
* handle long-form length overflow (reported in mirleft/ocaml-asn1-combinators#24 by @emillon, fixed by @pqwy)
* disallow primitive with indefinite length (introduced in the bugfix above,
  reported by @emillon, fixed in mirleft/ocaml-asn1-combinators#32 by @hannesm)
* disallow nonsensical bitstring unused values (X690 8.6.2, reported in mirleft/ocaml-asn1-combinators#26
  by @NathanReb, fixed by @pqwy)
* fix non-continuous bit_string_flags (X680 22.6, reported in mirleft/ocaml-asn1-combinators#25 by @wiml,
  fixed by @pqwy)
* use Alcotest instead of oUnit for unit tests (by @pqwy)
* use dune as build system (by @pqwy, superseeds mirleft/ocaml-asn1-combinators#22)
* use bigarray-compat (mirleft/ocaml-asn1-combinators#27 by @TheLortex) and stdlib-shims (mirleft/ocaml-asn1-combinators#29 by @XVilka)
* raise lower bound to OCaml 4.05.0 (mirleft/ocaml-asn1-combinators#31 by @hannesm)
@hannesm
Copy link
Member

hannesm commented Jan 28, 2020

thanks, this has been fixed and is part of the 0.2.1 release. closing

@hannesm hannesm closed this as completed Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants