Dexus / pem

Create private keys and certificates with node.js
Other
570 stars 129 forks source link

Bug in readPkcs12 prevents properly reading key #309

Closed guillaume86 closed 2 years ago

guillaume86 commented 2 years ago

Hi! ๐Ÿ‘‹

Firstly, thanks for your work on this project! ๐Ÿ™‚

Today I used patch-package to patch pem@1.14.4 for the project I'm working on.

Here is the diff that solved my problem:

diff --git a/node_modules/pem/lib/pem.js b/node_modules/pem/lib/pem.js
index 4250dff..956e122 100644
--- a/node_modules/pem/lib/pem.js
+++ b/node_modules/pem/lib/pem.js
@@ -816,8 +816,8 @@ function readPkcs12 (bufferOrPath, options, callback) {
         var certs = readFromString(stdout, CERT_START, CERT_END)
         keybundle.cert = certs.shift()
         keybundle.ca = certs
-        keybundle.key = readFromString(stdout, KEY_START, KEY_END).pop()

+        keybundle.key = readFromString(stdout, RSA_KEY_START, RSA_KEY_END).pop()
         if (keybundle.key) {
         // convert to RSA key
           return openssl.exec(['rsa', '-in', '--TMPFILE--'], 'RSA PRIVATE KEY', [keybundle.key], function (err, key) {
@@ -830,7 +830,7 @@ function readPkcs12 (bufferOrPath, options, callback) {
         if (options.clientKeyPassword) {
           keybundle.key = readFromString(stdout, ENCRYPTED_KEY_START, ENCRYPTED_KEY_END).pop()
         } else {
-          keybundle.key = readFromString(stdout, RSA_KEY_START, RSA_KEY_END).pop()
+          keybundle.key = readFromString(stdout, KEY_START, KEY_END).pop()
         }
       }

This issue body was partially generated by patch-package.

Dexus commented 2 years ago

would you be able to create a PR that will work on all Versions of OPENSSL/LibreSSL?

guillaume86 commented 2 years ago

I just fixed an easy to find bug (logic was inverted), it did fix my issue that's all I know sorry.

Dexus commented 2 years ago

@guillaume86 can you please provide fixtures and how you detect the bug? because I'm not sure if the test missing this or something else is need.

Thanks, Josef

guillaume86 commented 2 years ago

I can't do this right now but let me try again to explain:

https://github.com/Dexus/pem/blob/715e95db6128d01451d43890cd2cb835bc6440d2/lib/pem.js#L867-L882

In the first line, you try to extract a non encryted key, if it succeeds, you go into the first if case, which tries to decode the key with RSA, which do not work since the key is not encrypted. Now in the case of an RSA encrypted key, we enter in the last else case, you extract the RSA encrypted key, and returns it without decrypting it, which is also a problem. Hope this helps.

Dexus commented 2 years ago

Can you please tell me on which openssl version you work? because its looks only for openssl 3 to match

guillaume86 commented 2 years ago

Yes you're correct it's OpenSSL v3:

OpenSSL 3.0.1 14 Dec 2021 (Library: OpenSSL 3.0.1 14 Dec 2021)

Dexus commented 2 years ago

this issue is invalid