wstrange / asn1lib

Dart ASN1 Encoder / Decoder
BSD 2-Clause "Simplified" License
30 stars 31 forks source link

asn1lib 1.2.0 breaks decoding of snmp message sequences #62

Closed point-source closed 2 years ago

point-source commented 2 years ago

I am the author of dart_snmp and recently came across this issue. Upon investigation, I realized that it is rejecting tag 162 with the error The tag 162 does not look like a sequence type.

I am passing this Uint8List: [48, 74, 2, 1, 1, 4, 5, 115, 101, 118, 101, 110, 162, 62, 2, 4, 3, 235, 147, 43, 2, 1, 0, 2, 1, 0, 48, 48, 48, 46, 6, 8, 43, 6, 1, 2, 1, 1, 1, 0, 4, 34, 69, 100, 103, 101, 79, 83, 32, 118, 52, 46, 52, 46, 53, 54, 46, 53, 52, 52, 57, 48, 54, 50, 46, 50, 49, 49, 48, 50, 48, 46, 48, 56, 51, 49] to ASN1Sequence.fromBytes() which should decode to an SNMP message sequence.

wstrange commented 2 years ago

Ouch.

I introduced a fix for an older bug, but it looks like this is a regression. I will take a look.

wstrange commented 2 years ago

I need some feedback here..

Tag 162 is binary 10100010

Which the asn1 spec says:

It's the the last part that is confusing me. It should be a sequence type (0x10, or binary 0001 0000) - not an integer type.

If we see the constructed bit set, AND context specific, do we just assume it is a sequence?

The regression here is that the library is explicitly checking for a sequence type (0x10).

wstrange commented 2 years ago

Can you give 1.2.2 a try. It relaxes the check for an ASN1Sequence

point-source commented 2 years ago

Unfortunately, I am not an expert on asn1. I do know a fair bit about snmp structure so I can tell you that the byte sequence I posted is considered an SNMP "message". A v1 or v2 SNMP message (such as this one) consists of a version number, a community string, and a PDU.

The byte sequence I posted decodes as follows:

48: SNMP sequence type (context specific, in this case it's a "message") 74: Length 2: Integer type 1: Length 1: SNMP version = v2c 4: Octet string type 5: Length 115, 101, 118, 101, 110: seven 162: Beginning of the PDU of type "GetResponse" (there are at least 9 types from byte 160-168, see below) 62: Length 2: Integer type 4: Length 3, 235, 147, 43: Request ID 2: Integer type 1: Length 0: No error detected 2: Integer type 1: Length 0: No error type (because there's no error) 48: SNMP sequence type (context specific, in this case it's a "varbind" instead of a message) 48: Length 48: Varbind type = SNMP sequence 46: Length 6: Object Identifier type 8: Length 43, 6, 1, 2, 1, 1, 1, 0: translates to OID 1.3.6.1.2.1.1.1.0 = "sysDesc" = system description 4: Octet string type 34: Length 69, 100, 103, 101, 79, 83, 32, 118, 52, 46, 52, 46, 53, 54, 46, 53, 52, 52, 57, 48, 54, 50, 46, 50, 49, 49, 48, 50, 48, 46, 48, 56, 51, 49: EdgeOS v4.4.56.5449062.211020.0831

So basically any time byte is 48, this is an snmp sequence which could be any of: SNMP Message, SNMP Varbind, or SNMP Varbind type (contents)

The PDU types I support are these:

    160: 'GetRequest',
    161: 'GetNextRequest',
    162: 'GetResponse',
    163: 'SetRequest',
    164: 'Trap',
    165: 'GetBulkRequest',
    166: 'InformRequest',
    167: 'TrapV2',
    168: 'Report'

Here is more info on snmp encoding / decoding: https://www.ranecommercial.com/legacy/note161.html

point-source commented 2 years ago

Can you give 1.2.2 a try. It relaxes the check for an ASN1Sequence

This worked. Hopefully my previous answer can give more insight into how to support snmp without relaxing the checks too much. Thanks!

gabrielgarciagava commented 2 years ago

@wstrange I can see on this commit https://github.com/wstrange/asn1lib/commit/9a4c1d5517e63b79b737753233b85de850de2ae1 that you added the method isContextSpecific. However, I cannot see it being used inside nextObject.

I have a certificate that has the tag 0xa0 (which is context specific). That tag used to convert to ASN1Object in version 1.2.1. Now it converts to ASN1Sequence in version 1.2.2. This is happening because of the change in line 58

-   } else if (isApplication(tag) &&
-        isSequence(tag) &&
+    } else if (!isPrimitive(tag) &&
+        isConstructed(tag) &&

Not sure what is correct, to be honest, but the third party library that I'm using to consume the ASN1 object to convert to a x509 certiticate is expecting it to be an ASN1Object :(

wstrange commented 2 years ago

Not sure what is correct, to be honest,

Yes - this is my challenge as well. If we parse all context-specific tags as an ASN1Object that would seem to break the originally reported bug.

The spec is hard to follow, and the context specific tags seem like a free for all. If we can get more context (no pun intended) around what the library is doing that would be helpful. Perhaps the library should be re-casting the sequence?

If you can you provide a test case of bytes with the expected result, I'll take a look. My concern is breaking other libraries that might depend on these tags.

wstrange commented 2 years ago

FWIW, 0xa0 is context-specific with the "constructed" bit set, which normally means some kind of sequence or set type.

However the type bits are 0

gabrielgarciagava commented 2 years ago

After a quick check in the docs, I also could not get to a proper conclusion in relation to the first 2 bits. I could not find anything saying that context-specific should make it a ASN1Object.

I will try to reserve some time today to investigate it further. Maybe also take a look on some very stable implementation, like a C++ or JS library.

wstrange commented 2 years ago

After reading through Let's Encrypt I'm coming to the conclusion that the best thing to do is to return context-specific tags as ASN1Objects, and let the caller interpret and re encode as required. For example:

    // "recast" ASN1Object as a sequence type.
    var seq = ASN1Sequence.fromBytes(someObject.encodedBytes);

@point-source Will this work for the SNMP library? IIRC, you already re-cast the Object to a sequence. I think the fix I added to relax the sequence type check still works.

point-source commented 2 years ago

Yes I should be able to work around that. Preferably if you make it a semantic breaking change so I can more easily force the right versions.

wstrange commented 2 years ago

I uploaded version 1.3.0 (semver bump). See the README - but the gist is that there is now a ASN1ContextSpecific class (which is also an ASN1Object) which I think will do the trick.

gabrielgarciagava commented 2 years ago

That is good to hear. I will let you know if everything goes well as soon as I try it. Thank you