Private Key might leak

[sklemer1]

Because of

https://github.com/IdentityPython/pyXMLSecurity/blob/2a34bcac61cbd85a772354752a95f3c1bc1e5e5f/src/xmlsec/__init__.py#L479

we might leak the private key if e.g. cert_spec=None . This is very dangerous. I'm not completely sure how this interacts with pkcs11 -- will post a follow up soon.

[sklemer1]

@leifj @fredrikt @c00kiemon5ter this is pretty grave -- even if you don't like my approach to fix it, we should do something.

This is for example triggered by a pyff-config like:

[...]
             - sign:
                 key: /etc/ssl/private/privkey.priv
             - publish: foobar.xml

(i.e. withouth a cert-statement in the sign-clause)

[fredrikt]

Thanks for the ping. Agree that it looks bad. Not that I don't trust you, but have you actually verified that you can get the private key into an XML document with this? You can typically extract the public key from a private key PEM, and I don't know what

DS.KeyInfo(DS.X509Data(DS.X509Certificate(pem2b64(cert_src.cert_pem))))

would actually result in, given a private key.

[sklemer1]

Oh, I'm pretty sure as the line you referenced does something with cert_src.cert_pem

which at the moment we fill with the private key: https://github.com/sklemer1/pyXMLSecurity/commit/23f99b603ee6c304748f2807c92f606e90a5fa26#diff-e90de4a0ab2b87b5dd036d640b4e917aL134 . The rest is only about putting this string into the 'right' xml tags.

This only happens after the change to pyca/crypto and wasn't the case with the old backend (as it never filled cert_pem with a pem-representation of the private part). So it's not a problem in any released version but only with git master.

In the end I stumbled exactly about the problem @c00kiemon5ter saw coming when he wrote that we should disentangle private keys and certs and not have them in the same class. @leifj brought a good argument why this is necessary, but at least inside the class/object we shouldn't mix things up, e.g. use the attribute key to hold the private part OR the public part. This is very dangerous. I have a good idea what I want to do with this but don't have time for another 2 or 3 weeks.

[fredrikt]

No offense intended, just trying to communicate clearly:

Not to neglect importance at all, but "pretty sure" is not the level of confidence I was looking for to triage this.

Consider this:

Generate a self-signed certificate with

openssl req -x509 -newkey rsa:512 -keyout key.pem -out cert.pem -days 365 -nodes

Now, you can extract the public key from both cert.pem and key.pem using these two commands:

openssl rsa -in key.pem -pubout

openssl x509 -pubkey -noout -in cert.pem

So my question was do you know the private key will get inserted in the XML, or are you just assuming it will?

As for your suggested way forward, I am all for having two distinct classes for private keys and public keys. I like dynamic languages, and I also like type checking ;)

[sklemer1]

I am sure that there are case where the private key will end up in the xml and that I added a line that shouldn't be there because the private part of the key is never needed as pem/text and which is removed by #48 .

Did you actually look at the PR? The method in question is called private_bytes() and gives you the private part of the key: https://cryptography.io/en/latest/hazmat/primitives/asymmetric/rsa/#key-serialization

[c00kiemon5ter]

DS.KeyInfo(DS.X509Data(DS.X509Certificate(pem2b64(cert_src.cert_pem))))

That line will create the corresponding nested elements and add whatever is passed to the inner most element (X509Certificate) -the result of pem2b64()- as a text node. pem2b64() strips the first and last line of whatever is passed to it. There are no checks made to validate the created node.

What's in cert_src.cert_pem? cert_src is private, private is an rsa.RSAPrivateKey.

self.key = serialization.load_pem_private_key(...)
self.cert_pem = self.key.private_bytes(...)

So, this would result in the private key, serialised as bytes, inserted under the X509Certificate element.

btw, sorry I didn't see this earlier, as I wasn't watching the repo.

[fredrikt]

No I didn't look close enough. Thanks for pointing out the call to private_bytes(). As I understand it is your code, I trust you completely when you say it was made in error. I support merging #48.

[sklemer1]

I didn't say it, but of course I also run the patch through the unit tests, through pyff unit tests and our pyff scripts and mdq-server (the latter one is where I first noticed that something was wrong after pulling a recent master).

[sklemer1]

No I didn't look close enough. Thanks for pointing out the call to private_bytes().

Never mind ;).

btw, sorry I didn't see this earlier, as I wasn't watching the repo.

All fine :D.

But good thing would be that we won't wait much longer to fix it one way or another :).

[c00kiemon5ter]

for cert_src in (public, private):
    if cert_src is not None and cert_src.cert_pem:
        # Insert cert_data as b64-encoded X.509 certificate into XML document
        sv_elt = si.getnext()
        sv_elt.addnext(DS.KeyInfo(DS.X509Data(DS.X509Certificate(pem2b64(cert_src.cert_pem)))))
        break  # add the first we find, no more

The relevant code (modified a bit by other commits but the main structure is there) was added by 0243e545b8502831f3d8aaa307512b444f4bbd46 with the following comment:

possible solution for leifj/pyFF#63

The patch is:

-        if public is not None:
+        for cert_src in (public, private):
+            if cert_src is not None and 'data' in cert_src:
                 # Insert cert_data as b64-encoded X.509 certificate into XML document
                 sv_elt = si.getnext()
-            sv_elt.addnext(DS.KeyInfo(DS.X509Data(DS.X509Certificate(pem2b64(public['data'])))))
+                sv_elt.addnext(DS.KeyInfo(DS.X509Data(DS.X509Certificate(pem2b64(cert_src['data'])))))

Before the patch, the code would only look into the public key. I am not sure why we need to peek into the private key to get the cert data.

[sklemer1]

I think this comes from pkcs11 case that @leifj mentioned (private-key in token, certificate in file) and it's filled here): https://github.com/IdentityPython/pyXMLSecurity/blob/2a34bcac61cbd85a772354752a95f3c1bc1e5e5f/src/xmlsec/crypto.py#L164

(notice that the super-object is private and still the key- and cert_pem-attributes contains the cert. Whereas for XMLSecCryptoFile objects with private=true the key-attribute contains the private key. That's what I meant when I wrote it's a bit messy ;) )

With all my patches I tried to be as non-invasive as possible and to stay as close to the old backend as possible ... until some major overhaul of the XMlSecCrypto object. I only wanted to change the backend, not the inner workings that might have a ton of quirks (or not). If you follow this approach this would be what the PR is doing.

Leaving for the weekend now.

[leifj]

I merged that PR on faith but I agree we need to refactor this stuff asap.