rusticata / asn1-rs

Parsers/Encoders for ASN.1 BER/DER data
Apache License 2.0
10 stars 14 forks source link

TaggedImplicit: Fix construction bit inherent #18

Closed chessnokov closed 8 months ago

chessnokov commented 2 years ago

The problem is, this bit not set for Seguence and Set types, for example:

Person ::= SEQUENCE {
  name UTF8String
  age INTEGER,
}
TaggedPerson ::= [2] IMPLICIT Person

because the value of this bit was calculated based on the new tag: https://github.com/rusticata/asn1-rs/blob/bc877237161cde337bfa442b5654af8701fb1d59/src/asn1_types/tagged/implicit.rs#L113

The X.690 says: 8.14.4 If implicit tagging was used in the definition of the type, then: a) the encoding shall be constructed if the base encoding is constructed, and shall be primitive otherwise;

chifflier commented 1 year ago

Hi, Thanks for your pull request, and sorry for the long delay in reply. I think I understand the problem, but I am not entirely sure the proposed patch fixes it.

Basically, the TAG parameter of the TaggedValue is supposed to be SEQUENCE here, so this should have worked. I'm not sure why dynamically requesting the tag fixes it, and I think your problem was caused by the static TAG parameter only. Can you provide more details?

Now, to be exhaustive on the problem, I'm wondering if we need more complex changes, because what X.690 says implies that TaggedValue should ask the underlying type if it is constructed or not. Implementing this could require introducing a trait, I think

chifflier commented 8 months ago

Looking back to this issue, I can confirm the problem. The problem happens here because TAG is the generic parameter (the outer tag), while we intended to test the T::TAG value. Using self.inner.tag() is a (complicated) way to obtain the same value because DynTagged is implemented for all T: Tagged and returns T::TAG.

I'll commit a fix soon.

Now, this still is a workaround, since the real fix would be to query the constructed bit from the inner type, but that would probably require the introduction of a new trait.

chifflier commented 8 months ago

Applied, thanks!