search

wbond / asn1crypto

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

Graceful degradation and(or) extension mechanism for unspecified/further/national extensions parsing with .native. #170

Open yzelensky opened 4 years ago

yzelensky commented 4 years ago

How mentioned in #120, it would be nice to avoid an errors during the parsing of the structures, in particular by calling .native property, even if these structures are (partly) "unknown" for the parser and are the results of the extensions of (or deviations from) the current international standards.

Just as an example, there are a lot of such custom extensions defined in STB 34.101.27 (national standard of the republic of Belarus) and "described" in dumpasn1by.cfg as a custom configuration for dumpasn1tool. Another example is gost3410-2001 already mentioned in #120.

I'm pretty sure there are other national crypto standards exist, not only in Russian spoken countries, I'm just point to these examples, because I have faced with the samples of the corresponded certificates in practice.

In a particular, there were two cases where I faced with fatal errors until implement some (pretty dirty) workarounds (they are dirty because they are break the encapsulation) .

First was in : keys.PublicKeyInfo - it was failed to recognise a custom algorithm represented by oid "1.2.112.0.2.0.34.101.45.2.1" (according to .cfg file mentioned above it corresponds to bign-with-hbelt, some form of Eliptic Curves Based digital signature algorithm). But in my opinion it is too strict behaviour to throw an Error here - if we just do not known the internal structure of the key of the unrecognised algorithm, we can always represent it as a raw BitString, and still have an ability to see other (recognised) fields of the certificate.

My (ugly) workaround was:

# noinspection PyProtectedMember
def _my_public_key_spec(self):
    algorithm = self['algorithm']['algorithm'].native
    if algorithm == '1.2.112.0.2.0.34.101.45.2.1':
        return (core.OctetBitString, None)
    else:
        return keys.PublicKeyInfo._public_key_spec(self)

# noinspection PyProtectedMember
keys.PublicKeyInfo._spec_callbacks = {'public_key': _my_public_key_spec}

Another example is related to crl.CRLReason class. Some of the CRL list entities I faced in the wild, contains unstandardised codes 17 and 18, and now it leads to fatal Error if want to try to parse such a CRL. But for most reasonable use cases it would be absolutely ok, if we will see something like "unknown reason (code 17)" or "unknown reason (code 18)". Again, in my opinion fatal Error is too strict behaviour here. May be we need to introduce strict mode or something, but in the same time would be nice to have the ability for more tolerant processing of small deviations from specification here. Or at least, more friendly and/or specified ways to implement the extensions.

For now, my another ugly workaround looks like:

# noinspection PyProtectedMember
crl.CRLReason._map.update({
17: 'hardware_key_became_unusable', 
18: 'certificate_attributes_are_incorrect'})

P.S. Thank you all contributors for the amazing and useful library P.P.S. Btw, it was a challenge to figure out (just for the sake of curiosity) that code 17 and 18 actually means. :)

joernheissler commented 4 years ago

Hello, it still should possible to parse those with asn1crypto. But with less comfort, I agree:

from asn1crypto.crl import CertificateList
crl = CertificateList.load(bytes.fromhex('30783062020101300b06092a864886f70d01010b300e310c300a06035504030c0358595a170d3139313131333133353731345a170d3139313131333133353731345a3022302002017b170d3139313131333133353731345a300c300a0603551d1504030a0112300b06092a864886f70d01010b03050054657374'))
print(int(crl['tbs_cert_list']['revoked_certificates'][0]['crl_entry_extensions'][0]['extn_value'].parsed))
==> 18

In particular, .native doesn't work. Perhaps .native should be changed to output a meaningful expression in case a structure is unknown.

wbond commented 4 years ago

In general, the approach with asn1crypto and oscrypto is to not guess, or allow things to silently pass. This is because we are dealing with security, and it seems like not immediately telling the developer that something is not right is a bad idea.

I would probably be pretty strongly against allowing unknown CRLReason values to pass without throwing an error. If there are other common values, they should be added. As a last resort we can either document how to add support for more values, or enhance the code to make the enhancement easier.

The PublicKeyInfo issue seems like there is a reasonable expectation there that we don't try to parse the value if we don't recognize the OID. I've never run into these issues, nor do I have any examples to work from, but if someone wants to propose a solution, I think we could discuss it and likely get a PR merged.

yzelensky commented 4 years ago

@joernheissler, you are right, I had to be more precise, and say that namely .native property became broken (I've edited the issue, thank you).

This behaviour It is especially inconvenient, when you trying to utilise asn1crypto for a kind of exploratory analysis of some certificate data, just by loading it, and calling .native to the outmost structure, and it doesn't work, if it faced with any minor problem in any of the "leafs" of the whole structure.

wbond commented 4 years ago

This behaviour It is especially inconvenient, when you trying to utilise asn1crypto for a kind of exploratory analysis of some certificate data, just by loading it, and calling .native to the outmost structure, and it doesn't work, if it faced with any minor problem in any of the "leafs" of the whole structure.

Yeah, asn1crypto definitely was not designed with the exploration in mind. Making things easy for exploration and reasonably secure and noisy when things are unrecognized seems like goals that are somewhat at odds.

Ideally I'd prefer to accept PRs with missing OIDs and ASN.1 structures. Or someone can propose a solution to making it easier to explore an ASN.1 structure without silently letting errors or unknown data pass. I'm sure there are ways to improve the status quo.

yzelensky commented 4 years ago

Dear, @wbond , thank you for your prompt replies. Your clarifications about "exploration was not a design goal" is understandable, and I can agree that it could contradict with reliability in the sense that any implementation of https://en.wikipedia.org/wiki/Fail-fast could contradict to implementation of https://en.wikipedia.org/wiki/Fail-safe

I think that as a "greatest common divisor" of these goals we could agree that more advanced extensibility mechanisms, could help to achieve a both goals without any trade-offs. One who want a fail-safe behaviour would be able to make a call e.g like this:

asn1crypto.cms.ContentInfo.load(
    der_data, callbacks={
        keys.PublicKeyInfo.UNKNOWN_ALG_OID:lambda oid:core.OctetBitString
   })

One who just want to implement extension (before proposing a PR, he-he) and still preserve fail-fast behaviour, could write something like this:

BIGN_HBELT_ALGO_OID = '1.2.112.0.2.0.34.101.45.2.1'
BIGNHBELTECPointBitString
asn1crypto.cms.ContentInfo.load(
    der_data, callbacks={
        keys.PublicKeyInfo.UNKNOWN_ALG_OID:
        lambda oid: BIGNHBELTECPointBitString if oid==BIGN_HBELT_ALGO_OID else None
   })

The one idea is to look for a named callback which could prevent a fail using an extra knowledge about data structure, or just prevent a fail by interpreting unknown structure in some generalised way (e.g. OctetBitString). But there are other approaches possible, of course. I personally not feel myself quite comfortable with internals of your nice library yet, to propose such a massive PR, but I will definitely try later.

As for PR's for exact OIDs, I am in contact with an authors of local national crypto standards, and I will welcome them to propose such changes "from the first hands".

As about examples to work from, if you (or other potential authors) are really curious about, here are some links:

CA root certificates, and CRL of the national PKI of the Republic of Belarus (hundred of thousands certificates issued, which are in use on monthly basis): https://nces.by/wp-content/uploads/certificates/pki/kuc.crl https://nces.by/wp-content/uploads/certificates/pki/ruc.crl https://nces.by/wp-content/uploads/certificates/pki/kuc.cer https://nces.by/wp-content/uploads/certificates/pki/ruc.cer

CA root certificate of the Ukraine national PKI (around a one million of certificates are in use): https://www.uakey.com.ua/files/download.php?file=599998 BTW, the Ukrainian oid's are even registered on oid-info.com : http://oid-info.com/get/1.2.804.2.1.1.1.1

CA certificates of the PKI of the national crypto-provider from Russia (millions of certificates are in use): http://cpca20.cryptopro.ru/cacer.p7b BTW, I could not parse it with asn1crypto at all. Although output of openssl asn1parse looks reasonable.

It would be great if the other users of asn1crypto would leave here an examples of other (national) PKIs CA certificates (which are indeed will be public) if they (certificates) contains some national crypto extensions which are not supported by asn1crypto yet.