wbond / oscrypto

Compiler-free Python crypto library backed by the OS, supporting CPython and PyPy
MIT License
321 stars 71 forks source link

Uncaught KeyError in load_private_key #85

Open geitda opened 2 months ago

geitda commented 2 months ago

If you pass ASN.1 bytes that look too much like a PrivateKeyInfo structure (but aren't) to oscrypto.asymmetric.load_private_key, then you may end up with a KeyError that passes through everything. load_private_key (on Windows, at least) tries to unarmor the bytes (if necessary), then tries to interpret those bytes as PrivateKeyInfo, EncryptedPrivateKeyInfo, RSAPrivateKey, DSAPrivateKey, or ECPrivateKey in turn. It tries to get the native representation "since asn1crypto is lazy," and if it gets a ValueError it knows that the data isn't the given type. But if you pass data that looks similar enough to a PrivateKeyInfo object it gets far enough to try to determine the asymmetric key type (e.g. RSA, DSA, ECDSA) and gets a (dictionary-)KeyError because the OID it finds isn't actually for an asymmetric key type.

I encountered this specifically when trying to pass PKCS#12 data to load_private_key:

Traceback (most recent call last):
  File "<pyshell#142>", line 1, in <module>
    priv_key = asymmetric.load_private_key(der_bytes, password=b'testpassword')
  File "C:\Python312\Lib\site-packages\oscrypto\_win\asymmetric.py", line 2100, in load_private_key
    private_object = parse_private(source, password)
  File "C:\Python312\Lib\site-packages\oscrypto\_asymmetric.py", line 582, in parse_private
    pki.native
  File "C:\Python312\Lib\site-packages\asn1crypto\core.py", line 4070, in native
    self._parse_children(recurse=True)
  File "C:\Python312\Lib\site-packages\asn1crypto\core.py", line 3921, in _parse_children
    cls._precomputed_specs[field] or self._determine_spec(field))
  File "C:\Python312\Lib\site-packages\asn1crypto\core.py", line 3759, in _determine_spec
    spec_override = callback(self)
  File "C:\Python312\Lib\site-packages\asn1crypto\keys.py", line 710, in _private_key_spec
    return {
KeyError: '1.2.840.113549.1.7.1'

A PrivateKeyInfo is a sequence of an Integer (version) followed by a PrivateKeyAlgorithm sequence that starts with an OID that identifies the key type. PKCS#12 PFX PDUs look identical at their start, with an Integer (version) followed by a ContentInfo sequence that also starts with an OID, this time to identify if its unencrypted, symmetric cipher encrypted, or asymmetric cipher "enveloped" data. _determine_spec thinks the PKCS#12 OID should be a key type OID, and we get a KeyError.

I'm opening the issue here in oscrypto, not in asn1crypto because I'm not convinced that the problem (and therefore the fix) is in that package. parse_private has a try - except block around the pki.native call (this is where program flow moves over to asn1crypto) but only catches ValueErrors, not KeyErrors, but it's quite clear that this is possible. It's not asn1crypto's fault: oscrypto is taking a shot-in-dark by trying to pass unknown data - it should be responsible for any weird variety of errors that asn1crypto returns based on weird or unknown data.

Fix (from my Windows perspective) is to add KeyError to the except lines (584, 596, 604 & 620 to cover the PrivateKeyInfo, EncryptedPrivateKeyInfo, RSAPrivateKey, & ECPrivateKey cases, respectively: you can skip DSAPrivateKey as it has only Integers and shouldn't ever raise KeyError because of it, but the others have OIDs in places that can cause KeyErrors). Or maybe some sort of restructuring of this fall-through (since the excepts are all just pass and fall into the final ValueError). OR you can decide "asn1crypto shouldn't ever raise a KeyError from any of it's 'official' APIs" (like native) and rather should catch it somewhere in _parse_children or _determine_spec or some such, and re-raise as ValueError. If so, close this issue in oscrypto and open one in asn1crypto. Getting a KeyError from asn1crypto is pleasantly instructive as to what's going on, to make debugging (precisely like what I'm doing right now) a bit easier, so I'm inclined to making oscrypto more robust, catching it and raising "The data specified does not appear to be a known private key format" ValueError. If you catch and re-raise in asn1crypto you could add a sufficiently detailed traceback message, though, so there are definitely options, here.