wbond / asn1crypto

Python ASN.1 library with a focus on performance and a pythonic API
MIT License
335 stars 140 forks source link

Optional parameters #235

Closed m32 closed 2 years ago

m32 commented 2 years ago

In the algos.DigestValue class the parameter 'parameters' is marked as optional and if it is not provided DigestValue can produce values different from the reference values.

`import hashlib from asn1crypto import algos, core

smech = 'md5' data_digest = hashlib.md5(b"Hello, world!\n").digest() data1 = algos.DigestInfo( { "digest_algorithm": algos.DigestAlgorithm({ "algorithm": smech,

"parameters": algos.Null()

        }),
        "digest": data_digest,
    }

).dump() bb = core.load(data1) bb.debug()

data0 = bytes([int(x, 16) for x in '30 20 30 0c 06 08 2a 86 48 86 f7 0d 02 05 05 00 04 10'.split()]) bb = core.load(data0+data_digest) bb.debug() assert data1 == data0 `

MatthiasValvekens commented 2 years ago

Hmm, I'm a little confused. Are you referring to the parameters of DigestAlgorithm being optional? Because AFAIK DigestAlgorithm is typically just defined as AlgorithmIdentifier, which technically does have an optional parameters field (note: parameters has no default because it's an open type). Now, when a DigestAlgorithm is used in a DigestInfo value for the purposes of constructing an RSA signature, PKCS#1 imposes an extra rule requiring the parameters field to be explicitly set to NULL instead of them being omitted. Since the parameters field in AlgorithmIdentifier doesn't have a default, this produces different DER than just omitting the field.

I suppose that that's a confusing gotcha, but strictly speaking, I don't think anything unexpected is going on here from the ASN.1 encoding/decoding perspective. Are you suggesting that asn1crypto should impute that NULL automatically when serialising an algorithm identifier inside a DigestInfo value?

Maybe I'm completely missing the point, though.

assert data1 == data0

I think you mean assert data1 == data0 + data_digest, no? 🙂

m32 commented 2 years ago

When signing a large file with PKCS11, I decided to first calculate its hash and then sign it as in https://stackoverflow.com/questions/50273289/pkcs11interop-hash-with-sha256-and-sign-with-rsa-in-two-steps, when I had the program ready, I checked its operation with asn1crypto for various hash functions and with md5 I got different results After assigning the value "parameters": algos.Null (), the generated DER value matches what generates PKCS11 and bouncycastle.

Perhaps it is the constructor of the Any class. When the value of the (value) parameter is None, no data generation occurs when the DER is created.

data1:
Data: 0x300a06082a864886f70d--020504--10746308829575e17c3331bbcb00c0898b data0+data_digest: Data: 0x300c06082a864886f70d--0205050004--10746308829575e17c3331bbcb00c0898b

in bold is missing part

Traceback (most recent call last): File "test-da-der.py", line 21, in assert data1 == data0+data_digest

Are you suggesting that asn1crypto should impute that NULL automatically when serialising an algorithm identifier inside a DigestInfo value?

yes

MatthiasValvekens commented 2 years ago

Perhaps it is the constructor of the Any class. When the value of the (value) parameter is None, no data generation occurs when the DER is created.

To be totally fair, I don't think this is unexpected per se. In general Any values don't have a sensible default value, and

AlgorithmIdentifier ::= {
    algorithmId OBJECT IDENTIFIER,
    parameters ANY OPTIONAL
}

is not the same thing as

AlgorithmIdentifier ::= {
    algorithmId OBJECT IDENTIFIER,
    parameters ANY DEFAULT NULL
}

Not only is the encoding different, but there are even situations where distinctions like this change the meaning of the value completely: ECDomainParameters, for example, is usually defined as an untagged choice type where NULL is one of the choices. This is about algorithm identifiers for public keys, not digest functions, but syntactically they're pretty much the same thing.

So, I'm not saying that your request to autofill this NULL value as a default when adding a digest algorithm identifier to a DigestInfo is unreasonable (inb4 anyone mentions variable-output SHAKE256...), and I certainly wouldn't object to having it added to asn1crypto if it's easy to do. I don't think the current behaviour is wrong, though.

m32 commented 2 years ago

ok