crypto: throw only in direct C++ methods

Do not throw in internal C++ methods, that clobbers logic and may lead
to the situations, where both exception was thrown and the value was
returned (via `args.GetReturnValue().Set()`). That doesn't play nicely
with v8.

fix #6912

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index b6eabc5c429d6ecb862baa782ad3f27475182ead..cf1b6379fbf6160974c9f77b5c2ccea9ccb3750c 100644 (file)
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -2692,6 +2692,46 @@ void Hash::HashDigest(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+void SignBase::CheckThrow(SignBase::Error error) {
+  HandleScope scope(node_isolate);
+
+  switch (error) {
+    case kSignUnknownDigest:
+      return ThrowError("Unknown message digest");
+
+    case kSignNotInitialised:
+      return ThrowError("Not initialised");
+
+    case kSignInit:
+    case kSignUpdate:
+    case kSignPrivateKey:
+    case kSignPublicKey:
+      {
+        unsigned long err = ERR_get_error();
+        if (err)
+          return ThrowCryptoError(err);
+        switch (error) {
+          case kSignInit:
+            return ThrowError("EVP_SignInit_ex failed");
+          case kSignUpdate:
+            return ThrowError("EVP_SignUpdate failed");
+          case kSignPrivateKey:
+            return ThrowError("PEM_read_bio_PrivateKey failed");
+          case kSignPublicKey:
+            return ThrowError("PEM_read_bio_PUBKEY failed");
+          default:
+            abort();
+        }
+      }
+
+    case kSignOk:
+      return;
+  }
+}
+
+
+
+
 void Sign::Initialize(Environment* env, v8::Handle<v8::Object> target) {
   Local<FunctionTemplate> t = FunctionTemplate::New(New);
 
@@ -2712,17 +2752,18 @@ void Sign::New(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-void Sign::SignInit(const char* sign_type) {
-  HandleScope scope(node_isolate);
-
+SignBase::Error Sign::SignInit(const char* sign_type) {
   assert(md_ == NULL);
   md_ = EVP_get_digestbyname(sign_type);
-  if (!md_) {
-    return ThrowError("Uknown message digest");
-  }
+  if (!md_)
+    return kSignUnknownDigest;
+
   EVP_MD_CTX_init(&mdctx_);
-  EVP_SignInit_ex(&mdctx_, md_, NULL);
+  if (!EVP_SignInit_ex(&mdctx_, md_, NULL))
+    return kSignInit;
   initialised_ = true;
+
+  return kSignOk;
 }
 
 
@@ -2736,15 +2777,16 @@ void Sign::SignInit(const FunctionCallbackInfo<Value>& args) {
   }
 
   const String::Utf8Value sign_type(args[0]);
-  sign->SignInit(*sign_type);
+  CheckThrow(sign->SignInit(*sign_type));
 }
 
 
-bool Sign::SignUpdate(const char* data, int len) {
+SignBase::Error Sign::SignUpdate(const char* data, int len) {
   if (!initialised_)
-    return false;
-  EVP_SignUpdate(&mdctx_, data, len);
-  return true;
+    return kSignNotInitialised;
+  if (!EVP_SignUpdate(&mdctx_, data, len))
+    return kSignUpdate;
+  return kSignOk;
 }
 
 
@@ -2756,7 +2798,7 @@ void Sign::SignUpdate(const FunctionCallbackInfo<Value>& args) {
   ASSERT_IS_STRING_OR_BUFFER(args[0]);
 
   // Only copy the data if we have to, because it's a string
-  int r;
+  Error err;
   if (args[0]->IsString()) {
     Local<String> string = args[0].As<String>();
     enum encoding encoding = ParseEncoding(args[1], BINARY);
@@ -2765,29 +2807,25 @@ void Sign::SignUpdate(const FunctionCallbackInfo<Value>& args) {
     size_t buflen = StringBytes::StorageSize(string, encoding);
     char* buf = new char[buflen];
     size_t written = StringBytes::Write(buf, buflen, string, encoding);
-    r = sign->SignUpdate(buf, written);
+    err = sign->SignUpdate(buf, written);
     delete[] buf;
   } else {
     char* buf = Buffer::Data(args[0]);
     size_t buflen = Buffer::Length(args[0]);
-    r = sign->SignUpdate(buf, buflen);
+    err = sign->SignUpdate(buf, buflen);
   }
 
-  if (!r) {
-    return ThrowTypeError("SignUpdate fail");
-  }
+  CheckThrow(err);
 }
 
 
-bool Sign::SignFinal(const char* key_pem,
-                     int key_pem_len,
-                     const char* passphrase,
-                     unsigned char** sig,
-                     unsigned int *sig_len) {
-  if (!initialised_) {
-    ThrowError("Sign not initalised");
-    return false;
-  }
+SignBase::Error Sign::SignFinal(const char* key_pem,
+                                int key_pem_len,
+                                const char* passphrase,
+                                unsigned char** sig,
+                                unsigned int *sig_len) {
+  if (!initialised_)
+    return kSignNotInitialised;
 
   BIO* bp = NULL;
   EVP_PKEY* pkey = NULL;
@@ -2820,17 +2858,10 @@ bool Sign::SignFinal(const char* key_pem,
 
   EVP_MD_CTX_cleanup(&mdctx_);
 
-  if (fatal) {
-    unsigned long err = ERR_get_error();
-    if (err) {
-      ThrowCryptoError(err);
-    } else {
-      ThrowError("PEM_read_bio_PrivateKey");
-    }
-    return false;
-  }
+  if (fatal)
+    return kSignPrivateKey;
 
-  return true;
+  return kSignOk;
 }
 
 
@@ -2857,15 +2888,17 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) {
   md_len = 8192;  // Maximum key size is 8192 bits
   md_value = new unsigned char[md_len];
 
-  bool r = sign->SignFinal(buf,
-                           buf_len,
-                           len >= 3 && !args[2]->IsNull() ? *passphrase : NULL,
-                           &md_value,
-                           &md_len);
-  if (!r) {
+  Error err = sign->SignFinal(
+      buf,
+      buf_len,
+      len >= 3 && !args[2]->IsNull() ? *passphrase : NULL,
+      &md_value,
+      &md_len);
+  if (err != kSignOk) {
     delete[] md_value;
     md_value = NULL;
     md_len = 0;
+    return CheckThrow(err);
   }
 
   Local<Value> rc = StringBytes::Encode(
@@ -2896,18 +2929,18 @@ void Verify::New(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-void Verify::VerifyInit(const char* verify_type) {
-  HandleScope scope(node_isolate);
-
+SignBase::Error Verify::VerifyInit(const char* verify_type) {
   assert(md_ == NULL);
   md_ = EVP_get_digestbyname(verify_type);
-  if (md_ == NULL) {
-    return ThrowError("Unknown message digest");
-  }
+  if (md_ == NULL)
+    return kSignUnknownDigest;
 
   EVP_MD_CTX_init(&mdctx_);
-  EVP_VerifyInit_ex(&mdctx_, md_, NULL);
+  if (!EVP_VerifyInit_ex(&mdctx_, md_, NULL))
+    return kSignInit;
   initialised_ = true;
+
+  return kSignOk;
 }
 
 
@@ -2921,15 +2954,18 @@ void Verify::VerifyInit(const FunctionCallbackInfo<Value>& args) {
   }
 
   const String::Utf8Value verify_type(args[0]);
-  verify->VerifyInit(*verify_type);
+  CheckThrow(verify->VerifyInit(*verify_type));
 }
 
 
-bool Verify::VerifyUpdate(const char* data, int len) {
+SignBase::Error Verify::VerifyUpdate(const char* data, int len) {
   if (!initialised_)
-    return false;
-  EVP_VerifyUpdate(&mdctx_, data, len);
-  return true;
+    return kSignNotInitialised;
+
+  if (!EVP_VerifyUpdate(&mdctx_, data, len))
+    return kSignUpdate;
+
+  return kSignOk;
 }
 
 
@@ -2941,7 +2977,7 @@ void Verify::VerifyUpdate(const FunctionCallbackInfo<Value>& args) {
   ASSERT_IS_STRING_OR_BUFFER(args[0]);
 
   // Only copy the data if we have to, because it's a string
-  bool r;
+  Error err;
   if (args[0]->IsString()) {
     Local<String> string = args[0].As<String>();
     enum encoding encoding = ParseEncoding(args[1], BINARY);
@@ -2950,30 +2986,25 @@ void Verify::VerifyUpdate(const FunctionCallbackInfo<Value>& args) {
     size_t buflen = StringBytes::StorageSize(string, encoding);
     char* buf = new char[buflen];
     size_t written = StringBytes::Write(buf, buflen, string, encoding);
-    r = verify->VerifyUpdate(buf, written);
+    err = verify->VerifyUpdate(buf, written);
     delete[] buf;
   } else {
     char* buf = Buffer::Data(args[0]);
     size_t buflen = Buffer::Length(args[0]);
-    r = verify->VerifyUpdate(buf, buflen);
+    err = verify->VerifyUpdate(buf, buflen);
   }
 
-  if (!r) {
-    return ThrowTypeError("VerifyUpdate fail");
-  }
+  CheckThrow(err);
 }
 
 
-bool Verify::VerifyFinal(const char* key_pem,
-                         int key_pem_len,
-                         const char* sig,
-                         int siglen) {
-  HandleScope scope(node_isolate);
-
-  if (!initialised_) {
-    ThrowError("Verify not initalised");
-    return false;
-  }
+SignBase::Error Verify::VerifyFinal(const char* key_pem,
+                                    int key_pem_len,
+                                    const char* sig,
+                                    int siglen,
+                                    bool* verify_result) {
+  if (!initialised_)
+    return kSignNotInitialised;
 
   ClearErrorOnReturn clear_error_on_return;
   (void) &clear_error_on_return;  // Silence compiler warning.
@@ -3036,13 +3067,11 @@ bool Verify::VerifyFinal(const char* key_pem,
   EVP_MD_CTX_cleanup(&mdctx_);
   initialised_ = false;
 
-  if (fatal) {
-    unsigned long err = ERR_get_error();
-    ThrowCryptoError(err);
-    return false;
-  }
+  if (fatal)
+    return kSignPublicKey;
 
-  return r == 1;
+  *verify_result = r == 1;
+  return kSignOk;
 }
 
 
@@ -3074,11 +3103,13 @@ void Verify::VerifyFinal(const FunctionCallbackInfo<Value>& args) {
     hbuf = Buffer::Data(args[1]);
   }
 
-  bool rc = verify->VerifyFinal(kbuf, klen, hbuf, hlen);
-  if (args[1]->IsString()) {
+  bool verify_result;
+  Error err = verify->VerifyFinal(kbuf, klen, hbuf, hlen, &verify_result);
+  if (args[1]->IsString())
     delete[] hbuf;
-  }
-  args.GetReturnValue().Set(rc);
+  if (err != kSignOk)
+    return CheckThrow(err);
+  args.GetReturnValue().Set(verify_result);
 }
 
 

diff --git a/src/node_crypto.h b/src/node_crypto.h
index 7f29e8959046cc85a74621fcf6dec4a7d8ea0af2..aa670dbf10fc3aa1b18f1c5256bfbc8859dcc788 100644 (file)
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -446,23 +446,50 @@ class Hash : public BaseObject {
   bool initialised_;
 };
 
-class Sign : public BaseObject {
+class SignBase : public BaseObject {
  public:
-  ~Sign() {
+  typedef enum {
+    kSignOk,
+    kSignUnknownDigest,
+    kSignInit,
+    kSignNotInitialised,
+    kSignUpdate,
+    kSignPrivateKey,
+    kSignPublicKey
+  } Error;
+
+  SignBase(Environment* env, v8::Local<v8::Object> wrap)
+      : BaseObject(env, wrap),
+        md_(NULL),
+        initialised_(false) {
+  }
+
+  ~SignBase() {
     if (!initialised_)
       return;
     EVP_MD_CTX_cleanup(&mdctx_);
   }
 
+ protected:
+  static void CheckThrow(Error error);
+
+  EVP_MD_CTX mdctx_; /* coverity[member_decl] */
+  const EVP_MD* md_; /* coverity[member_decl] */
+  bool initialised_;
+};
+
+class Sign : public SignBase {
+ public:
+
   static void Initialize(Environment* env, v8::Handle<v8::Object> target);
 
-  void SignInit(const char* sign_type);
-  bool SignUpdate(const char* data, int len);
-  bool SignFinal(const char* key_pem,
-                 int key_pem_len,
-                 const char* passphrase,
-                 unsigned char** sig,
-                 unsigned int *sig_len);
+  Error SignInit(const char* sign_type);
+  Error SignUpdate(const char* data, int len);
+  Error SignFinal(const char* key_pem,
+                  int key_pem_len,
+                  const char* passphrase,
+                  unsigned char** sig,
+                  unsigned int *sig_len);
 
  protected:
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -470,35 +497,22 @@ class Sign : public BaseObject {
   static void SignUpdate(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SignFinal(const v8::FunctionCallbackInfo<v8::Value>& args);
 
-  Sign(Environment* env, v8::Local<v8::Object> wrap)
-      : BaseObject(env, wrap),
-        md_(NULL),
-        initialised_(false) {
+  Sign(Environment* env, v8::Local<v8::Object> wrap) : SignBase(env, wrap) {
     MakeWeak<Sign>(this);
   }
-
- private:
-  EVP_MD_CTX mdctx_; /* coverity[member_decl] */
-  const EVP_MD* md_; /* coverity[member_decl] */
-  bool initialised_;
 };
 
-class Verify : public BaseObject {
+class Verify : public SignBase {
  public:
-  ~Verify() {
-    if (!initialised_)
-      return;
-    EVP_MD_CTX_cleanup(&mdctx_);
-  }
-
   static void Initialize(Environment* env, v8::Handle<v8::Object> target);
 
-  void VerifyInit(const char* verify_type);
-  bool VerifyUpdate(const char* data, int len);
-  bool VerifyFinal(const char* key_pem,
-                   int key_pem_len,
-                   const char* sig,
-                   int siglen);
+  Error VerifyInit(const char* verify_type);
+  Error VerifyUpdate(const char* data, int len);
+  Error VerifyFinal(const char* key_pem,
+                    int key_pem_len,
+                    const char* sig,
+                    int siglen,
+                    bool* verify_result);
 
  protected:
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -506,17 +520,9 @@ class Verify : public BaseObject {
   static void VerifyUpdate(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void VerifyFinal(const v8::FunctionCallbackInfo<v8::Value>& args);
 
-  Verify(Environment* env, v8::Local<v8::Object> wrap)
-      : BaseObject(env, wrap),
-        md_(NULL),
-        initialised_(false) {
+  Verify(Environment* env, v8::Local<v8::Object> wrap) : SignBase(env, wrap) {
     MakeWeak<Verify>(this);
   }
-
- private:
-  EVP_MD_CTX mdctx_; /* coverity[member_decl] */
-  const EVP_MD* md_; /* coverity[member_decl] */
-  bool initialised_;
 };
 
 class DiffieHellman : public BaseObject {