crypto.publicDecrypt and crypto.publicEncrypt doesn't detect PEM encoded public key in parameters

[rinne]

• Version:
• Platform:
• Subsystem:

crypto.publicDecrypt and crypto.publicEncrypt doesn't detect PEM encoded public key in parameters.

$ node -p process.versions
{ http_parser: '2.7.0',
  node: '8.1.0',
  v8: '5.8.283.41',
  uv: '1.12.0',
  zlib: '1.2.11',
  ares: '1.10.1-DEV',
  modules: '57',
  openssl: '1.0.2l',
  icu: '58.2',
  unicode: '9.0',
  cldr: '30.0.3',
  tz: '2016j' }

var pub = `
-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAh7g2wJMkaKwLoVanYikW
E3XwgZoFI+NAiyuhhMpMch/l0Izyk7KQQ55/t/hQSU7NP82p+jo/FRg6yPbMF8XN
ESz9FRx4UqNdkGFb2k3v36+LuMteg02eOhK6XQejtBrKwXKpew9062u4sIKw1mhx
m4wKGNw6b9LlIVLlQXdyiQQV4O2sO0H+3GqdHYWBzK1ombw10tsbpu1zoAlVRG+V
o23TVaJoJKAE72rcFInUfUfc2Pz/+X6M6lpnehv/12sRHyT6wG3EK7RJzFDvj3ZA
y0c1dHo8xTgDC+rA18za1M4Skyw22USDQG/dlCy3tC+2ZR71benOg7zihXys8IW4
ewIDAQAB
-----END PUBLIC KEY-----
`;

var prv = `
-----BEGIN PRIVATE KEY-----
MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCHuDbAkyRorAuh
VqdiKRYTdfCBmgUj40CLK6GEykxyH+XQjPKTspBDnn+3+FBJTs0/zan6Oj8VGDrI
9swXxc0RLP0VHHhSo12QYVvaTe/fr4u4y16DTZ46ErpdB6O0GsrBcql7D3Tra7iw
grDWaHGbjAoY3Dpv0uUhUuVBd3KJBBXg7aw7Qf7cap0dhYHMrWiZvDXS2xum7XOg
CVVEb5WjbdNVomgkoATvatwUidR9R9zY/P/5fozqWmd6G//XaxEfJPrAbcQrtEnM
UO+PdkDLRzV0ejzFOAML6sDXzNrUzhKTLDbZRINAb92ULLe0L7ZlHvVt6c6DvOKF
fKzwhbh7AgMBAAECggEASPDSTFFIYhEE9yLxNqpKOQ9LwPucA5uhFqrOVeW7jCJk
it8ViBeRvSW0EpWb4Ko/WSkZT2h6esXY4iTxr10ZRz/cjLoOWKuAH5aTnRIB90AL
Ybe7BepzPVbPXiw66RL1IV6Ug5TdC6GLUyIKFFFxrV1oF9BUf8DicDRzDeo6XjRO
/LU2ZgK8A49erVuqsukCZqetveSp2psJEXK2PNTkZ2MaFVkDwvBmuG7BLEcHipp6
6lfrqwJq7JoAoPNneKG/7w0rLktQ5R6HsC8suE2IRJRkOrvaziUkTdlfqkxrX5PC
YKn7RMYz/V+fe3OatHmAF5l2Mcjd8jytmBaMnoTtIQKBgQDaoJScsx7NUH49qPjt
G2CvK/nvV/PrUBXZbSFKvWwm2PAXaYPJ9XipPcLbjzH4AYouHBqNevTxqi+Iqu7d
2dY97xCV+PQfZYkyd6rT2I8otqSWbvwBSalUjzeczCcel5Ada9msK4UgWx6nWOZw
IEdQKqXo769Mt7C68sTZOoJ0MwKBgQCe63utoo5tm9SiHR/6rPDfv8IKM2zANgM6
dXScL2T97bdjoAr06JQXuvpWwfVZf7HWw9dmRuf91+ug0bwFsB9pHC+W60ZOnSaf
YslWfSIi+2jrom1YLWy8ZCZyYmZb75DjUHyf/4EFDVL6o5+L2sGvUuWuOu6V5JDY
Ao9S0uKimQKBgH0zdsfiQCJ+FT2EdcF7azwF6CTr7nD0tP6F44nkvnnkxGHz+BgB
Lm9lQiDweUI4x4QubfpVzs5SktQmZ5K+/FUNgicQoeUVBaPUKg0VuK4tIkZQGps2
LvWQ6t0tgL2hOFPQ/p/9cEieRgi5/YV6xrwfIFIsaOx7SYdWHer0+d5HAoGBAJmt
/xwaZsF4QFfE8nfnZcf6GBrlP/VgRh7yFqIy8ubcSsv8qJvNjeik2BGt3yV9ZuzY
1iQBzbacZzBNohWeC8IJj7vSKVs8fW0Eis8okyphFUVI/ZSX2N8VulhC79lYAjTQ
ULQo0QuhpuzZ7h/AnCx/bbzfIHmzXp6FWzQs2x2BAoGARU6Ozo5JFMWAPd3/fLPc
/Buwk7G5BYo/qmsKuRWV77fp6w746/zBe/Go0PVlpvLd5Y1+wr61FBbqES4W2yyW
W+AAVHIJHKF4TnH2plLC8+4igyHDw71/WcPxEy2mDxZ/Bq8/UtBTYvbZM2HcSZKN
jB64rblDNHa9ZNci7s8E9BI=
-----END PRIVATE KEY-----
`;

const crypto = require('crypto');
const constants = require('constants');
const padding = constants.RSA_PKCS1_PADDING;

var msg = new Buffer('foo');
console.log(msg);
var sig = crypto.privateEncrypt({ key: prv, padding: padding  }, msg)
console.log(sig);

var clr = crypto.publicDecrypt({ key: pub, padding: padding  }, sig)
// change prv to previous line instead of pub and it returns data as expected.
// this one however throws error of not finding key start line

console.log(clr);

[gibfahn]

cc/ @nodejs/crypto

[rinne]

And this seems to be the case in all versions I tried (>= node.js 4.6).

[seishun]

@rinne both of those keys are technically invalid because they start with a newline. Node.js looks at the beginning of the key to figure out what kind of a key it is and assumes PrivateKey if all else fails. (see here) That works when the key actually is a private key (I guess OpenSSL is lenient about whitespace) and doesn't if it's a public key.

1. Do we want to explicitly support leading whitespace in keys?
2. If not, do we want to explicitly not support leading whitespace and throw an error instead of assuming PrivateKey?

cc @nodejs/crypto

[gibfahn]

Do we want to explicitly support leading whitespace in keys?

This seems reasonable at first glance.

[seishun]

Then the next question is whether OpenSSL has any function that automatically detects key type, or if a long chain of if/else with comparisons is really the only way.

[rinne]

Oh my. The problem indeed was the newline in the beginning of the key. Weird that it wasn't a problem with the private key though. And isn't the beginning and the end markers with base64 in between just something to get around the possible trash around the actual payload?

Anyways, I leave it to you, whether you want just to close the ticket or do something about it.

One thing that at least needs fixing, is the documentation of the crypto session of node.js. Even in public key encrypt/decrypt it talks about:

crypto.publicEncrypt(public_key, buffer)

Added in: v0.11.14
public_key <Object> | <string>
key <string> A PEM encoded private key.
...

It does say later that the private key can be used instead of the public key, but still it's clearly wrong. And a comment about the whitespace would be welcome :).

[seishun]

And isn't the beginning and the end markers with base64 in between just something to get around the possible trash around the actual payload?

Indeed, if I insert garbage in the beginning of the private key, it still works just fine. Maybe instead of inspecting the beginning of the key, Node.js should just try calling each of OpenSSL's read functions until one works?

One thing that at least needs fixing, is the documentation of the crypto session of node.js.

You're welcome to submit a PR for that. 😉

[rinne]

OK, I did. #13633
(I almost lost my interest, when I noticed I can get around this feature by trimming the input. :)

[codeman869]

I think this should probably be fixed, if you read RFC 7468, on page 4:

Data before the encapsulation boundaries are
permitted, and parsers MUST NOT malfunction when processing such
data. Furthermore, parsers SHOULD ignore whitespace and other non-
base64 characters and MUST handle different newline conventions.

If there's no objections, I'd like to take on this change

[bnoordhuis]

Pull request welcome. Node.js probably needs to start using PEM_bytes_read_bio() to find the -----BEGIN and -----END markers, then dispatch based on the key type. A convenience function for private keys exists:

node/deps/openssl/openssl/crypto/pem/pem_pkey.c

Lines 78 to 148 in b3e5367

	EVP_PKEY *PEM_read_bio_PrivateKey(BIO *bp, EVP_PKEY **x, pem_password_cb *cb,
	void *u)
	{
	char *nm = NULL;
	const unsigned char *p = NULL;
	unsigned char *data = NULL;
	long len;
	int slen;
	EVP_PKEY *ret = NULL;
	if (!PEM_bytes_read_bio(&data, &len, &nm, PEM_STRING_EVP_PKEY, bp, cb, u))
	return NULL;
	p = data;
	if (strcmp(nm, PEM_STRING_PKCS8INF) == 0) {
	PKCS8_PRIV_KEY_INFO *p8inf;
	p8inf = d2i_PKCS8_PRIV_KEY_INFO(NULL, &p, len);
	if (!p8inf)
	goto p8err;
	ret = EVP_PKCS82PKEY(p8inf);
	if (x) {
	if (*x)
	EVP_PKEY_free((EVP_PKEY *)*x);
	*x = ret;
	}
	PKCS8_PRIV_KEY_INFO_free(p8inf);
	} else if (strcmp(nm, PEM_STRING_PKCS8) == 0) {
	PKCS8_PRIV_KEY_INFO *p8inf;
	X509_SIG *p8;
	int klen;
	char psbuf[PEM_BUFSIZE];
	p8 = d2i_X509_SIG(NULL, &p, len);
	if (!p8)
	goto p8err;
	if (cb)
	klen = cb(psbuf, PEM_BUFSIZE, 0, u);
	else
	klen = PEM_def_callback(psbuf, PEM_BUFSIZE, 0, u);
	if (klen <= 0) {
	PEMerr(PEM_F_PEM_READ_BIO_PRIVATEKEY, PEM_R_BAD_PASSWORD_READ);
	X509_SIG_free(p8);
	goto err;
	}
	p8inf = PKCS8_decrypt(p8, psbuf, klen);
	X509_SIG_free(p8);
	OPENSSL_cleanse(psbuf, klen);
	if (!p8inf)
	goto p8err;
	ret = EVP_PKCS82PKEY(p8inf);
	if (x) {
	if (*x)
	EVP_PKEY_free((EVP_PKEY *)*x);
	*x = ret;
	}
	PKCS8_PRIV_KEY_INFO_free(p8inf);
	} else if ((slen = pem_check_suffix(nm, "PRIVATE KEY")) > 0) {
	const EVP_PKEY_ASN1_METHOD *ameth;
	ameth = EVP_PKEY_asn1_find_str(NULL, nm, slen);
	if (!ameth || !ameth->old_priv_decode)
	goto p8err;
	ret = d2i_PrivateKey(ameth->pkey_id, x, &p, len);
	}
	p8err:
	if (ret == NULL)
	PEMerr(PEM_F_PEM_READ_BIO_PRIVATEKEY, ERR_R_ASN1_LIB);
	err:
	OPENSSL_free(nm);
	OPENSSL_cleanse(data, len);
	OPENSSL_free(data);
	return (ret);
	}

Consolidate the public/private key handling in src/node_crypto.cc while you're at it.

[Nolaan]

Up, this issue is still present in node v8.11