Skip to content

Commit

Permalink
src: split CryptoPemCallback into two functions
Browse files Browse the repository at this point in the history
Currently the function CryptoPemCallback is used for two things:
1. As a passphrase callback.
2. To avoid the default OpenSSL passphrase routine.

The default OpenSSL passphase routine would apply if both
the callback and the passphrase are null pointers and the typical
behaviour is to prompt for the passphase which is not appropriate in
node.

This commit suggests that the PasswordCallback function only handle
passphrases, and that an additional function named NoPasswordCallback
used for the second case to avoid OpenSSL's passphase routine.

PR-URL: #12827
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
danbev authored and addaleax committed May 18, 2017
1 parent 7e5ed8b commit 29d89c9
Showing 1 changed file with 22 additions and 11 deletions.
33 changes: 22 additions & 11 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,6 @@ static void crypto_lock_cb(int mode, int n, const char* file, int line) {
}


// This callback is used by OpenSSL when it needs to query for the passphrase
// which may be used for encrypted PEM structures.
static int PasswordCallback(char *buf, int size, int rwflag, void *u) {
if (u) {
size_t buflen = static_cast<size_t>(size);
Expand All @@ -244,6 +242,16 @@ static int PasswordCallback(char *buf, int size, int rwflag, void *u) {
}


// This callback is used to avoid the default passphrase callback in OpenSSL
// which will typically prompt for the passphrase. The prompting is designed
// for the OpenSSL CLI, but works poorly for Node.js because it involves
// synchronous interaction with the controlling terminal, something we never
// want, and use this function to avoid it.
static int NoPasswordCallback(char *buf, int size, int rwflag, void *u) {
return 0;
}


void ThrowCryptoError(Environment* env,
unsigned long err, // NOLINT(runtime/int)
const char* default_message = nullptr) {
Expand Down Expand Up @@ -613,7 +621,7 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
// that we are interested in
ERR_clear_error();

x = PEM_read_bio_X509_AUX(in, nullptr, PasswordCallback, nullptr);
x = PEM_read_bio_X509_AUX(in, nullptr, NoPasswordCallback, nullptr);

if (x == nullptr) {
SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_PEM_LIB);
Expand All @@ -631,7 +639,10 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
goto done;
}

while ((extra = PEM_read_bio_X509(in, nullptr, PasswordCallback, nullptr))) {
while ((extra = PEM_read_bio_X509(in,
nullptr,
NoPasswordCallback,
nullptr))) {
if (sk_X509_push(extra_certs, extra))
continue;

Expand Down Expand Up @@ -728,7 +739,7 @@ static X509_STORE* NewRootCertStore() {
if (root_certs_vector.empty()) {
for (size_t i = 0; i < arraysize(root_certs); i++) {
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
X509 *x509 = PEM_read_bio_X509(bp, nullptr, PasswordCallback, nullptr);
X509 *x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr);
BIO_free(bp);

// Parse errors from the built-in roots are fatal.
Expand Down Expand Up @@ -771,7 +782,7 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {

X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
while (X509* x509 =
PEM_read_bio_X509(bio, nullptr, PasswordCallback, nullptr)) {
PEM_read_bio_X509(bio, nullptr, NoPasswordCallback, nullptr)) {
if (cert_store == root_cert_store) {
cert_store = NewRootCertStore();
SSL_CTX_set_cert_store(sc->ctx_, cert_store);
Expand Down Expand Up @@ -803,7 +814,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
return;

X509_CRL* crl =
PEM_read_bio_X509_CRL(bio, nullptr, PasswordCallback, nullptr);
PEM_read_bio_X509_CRL(bio, nullptr, NoPasswordCallback, nullptr);

if (crl == nullptr) {
BIO_free_all(bio);
Expand Down Expand Up @@ -842,7 +853,7 @@ static unsigned long AddCertsFromFile( // NOLINT(runtime/int)
}

while (X509* x509 =
PEM_read_bio_X509(bio, nullptr, PasswordCallback, nullptr)) {
PEM_read_bio_X509(bio, nullptr, NoPasswordCallback, nullptr)) {
X509_STORE_add_cert(store, x509);
X509_free(x509);
}
Expand Down Expand Up @@ -4387,7 +4398,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
// Split this out into a separate function once we have more than one
// consumer of public keys.
if (strncmp(key_pem, PUBLIC_KEY_PFX, PUBLIC_KEY_PFX_LEN) == 0) {
pkey = PEM_read_bio_PUBKEY(bp, nullptr, PasswordCallback, nullptr);
pkey = PEM_read_bio_PUBKEY(bp, nullptr, NoPasswordCallback, nullptr);
if (pkey == nullptr)
goto exit;
} else if (strncmp(key_pem, PUBRSA_KEY_PFX, PUBRSA_KEY_PFX_LEN) == 0) {
Expand All @@ -4403,7 +4414,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
goto exit;
} else {
// X.509 fallback
x509 = PEM_read_bio_X509(bp, nullptr, PasswordCallback, nullptr);
x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr);
if (x509 == nullptr)
goto exit;

Expand Down Expand Up @@ -4530,7 +4541,7 @@ bool PublicKeyCipher::Cipher(const char* key_pem,
goto exit;
} else if (operation == kPublic &&
strncmp(key_pem, CERTIFICATE_PFX, CERTIFICATE_PFX_LEN) == 0) {
x509 = PEM_read_bio_X509(bp, nullptr, PasswordCallback, nullptr);
x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr);
if (x509 == nullptr)
goto exit;

Expand Down

0 comments on commit 29d89c9

Please sign in to comment.