mouse07410 / asn1c

The ASN.1 Compiler
http://lionet.info/asn1c/
BSD 2-Clause "Simplified" License
94 stars 70 forks source link

JER BIT STRING quoted hex #133

Closed v0-e closed 11 months ago

v0-e commented 1 year ago

JER BIT STRING currently encodes to:

This PR fixes both of these issues.

Current limitation: this contribution assumes a BIT STRING of fixed size. X.697 provides a variant encoding for BIT STRING of variable size, however, from my understanding, the function in question (BIT_STRING_encode_jer()) may not have enough context to know if the BIT STRING element is constrained in size or not.

mouse07410 commented 1 year ago

My apologies - by now I managed to forget what the intended effect of this PR would be, compared with the current behavior. Could you please refresh my memory, and maybe give a couple of examples?

v0-e commented 1 year ago

Sorry about that, I could have added more details. Consider the following definition:

Test ::= SEQUENCE {
    number INTEGER,
    bsFixed BIT STRING (SIZE(16)),
    bsVar BIT STRING
}

With number=122 and bsFixed=bsVar=1001111001000100, this currently encodes to:

{
"Test":{

    "number": 122,
    "bsFixed":
        1001111001000100
    ,
    "bsVar":
        1001111001000100
    }
}

However, X.697 (Section 24 - Encoding of bitstring values) mandates to use an hex encoding for the BIT STRING. Moreover, if you set any of these BIT STRING fields to 000... the final field encoding will be something like "bsFixed":00000, which is an invalid JSON according to RFC 8259.

This PR fixes both of these issues. The encoding of the above becomes:

{
"Test":{

    "number": 122,
    "bsFixed": "9e44"
    ,
    "bsVar": "9e44"
    }
}

A bit awkward with the displaced comma, but still a valid JSON while partially following X.697. I say partially because both BIT STRING fields are encoded in the same way while one's length is fixed and the other is variable.

I am unsure if the function responsible for encoding a JER BIT STRING (BIT_STRING_encode_jer()) is aware if the field is constrained in size or not to implement the above distinction.

Let me know if there is anything I can do to improve this PR.

kentkost commented 1 year ago

Pretty sure that this provides a more precise implementation of x.697. But I am not sure if using sprintf is the correct fastest way to go converting binary to hex. If both of you don't mind. Then I would like to test the behavior this coming weekend. I'll also try to add some tests.

v0-e commented 1 year ago

Indeed, sprintf is not the best approach. I'll try to change it before the weekend.

v0-e commented 1 year ago

I've switched to the lookup approach instead of using sprintf. I've also removed code related to adding line breaks in the middle of the encoded BIT STRING.

Looks like the encoding of a JER OCTET STRING for large enough lengths also produces an invalid JSON due to the line breaks. For example,

Test ::= SEQUENCE {
    os OCTET STRING
}

with os=000... 32 bytes in length, encodes to:

{
"Test":{

    "os":
        00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
        "00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00"
    }
}

Not sure if this PR can be used to fix this or should a new one be created.

mouse07410 commented 1 year ago

Not sure if this PR can be used to fix this or should a new one be created.

A separate PR, please. One issue - one PR. ;-)

Thanks!

mouse07410 commented 1 year ago

Looks alright to me.

This line though seems a bit odd to me. What's the reason for this roundabout way of creating an array with a size of 52? And why was it reduced from 128?


char scratch[16 * 3 + 4];

I've no clue. :-(

v0-e commented 1 year ago

Looks alright to me.

This line though seems a bit odd to me. What's the reason for this roundabout way of creating an array with a size of 52? And why was it reduced from 128?

char scratch[16 * 3 + 4];

I copied some code of the JER OCTET STRING encoder implementation: https://github.com/mouse07410/asn1c/blob/vlm_master/skeletons/OCTET_STRING_jer.c#L17 I just left it because we don't need 128 bytes here.

kentkost commented 1 year ago

I am unsure if the function responsible for encoding a JER BIT STRING (BIT_STRING_encode_jer()) is aware if the field is constrained in size or not to implement the above distinction.

I don't see how it would be possible to comply with X.697 in regards to variable lengths. Per @v0-e example.

Test ::= SEQUENCE {
    number INTEGER,
    bsFixed BIT STRING (SIZE(16)),
    bsVar BIT STRING
}

Should actually encode to

{
"Test":{
    "number": 122,
    "bsFixed": "9e44",
    "bsVar": {
        "length": 14,
        "value": "9e44"
    }
}

I just don't see how that is possible under the current implementation. The constraints "belong" to the PDU itself. But the encoding has long since happened. There is just no way to check beforehand if the BIT STRING that is being encoded has any size constraints. It would require that as you encode know where you are in the tree ie. which PDU is being encoded and subsequent primitives. It would also require the name of the element being encoded. And then you would have to be able to have some kind of lookup on the PDU itself to get the result of whether the element currently being encoded has any SIZE constraints.

I think it is best to just deviate from the specification in this case. At least until this has been properly discussed on how this should be solved.