Unify the error checking of crypto::Encryptor and add WARN_UNUSED_RESULT to prevent misuse.

crypto/encryptor_nss.cc

Index: crypto/encryptor_nss.cc
diff --git a/crypto/encryptor_nss.cc b/crypto/encryptor_nss.cc
index cf4fa2a78d4ca04486940ca5d5ec8c27d85df9ef..3ae4c0d45aeef193baee21681cb3911c4e173478 100644
--- a/crypto/encryptor_nss.cc
+++ b/crypto/encryptor_nss.cc
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "base/string_util.h"
 #include "crypto/nss_util.h"
 #include "crypto/symmetric_key.h"
 
@@ -44,8 +45,9 @@ Encryptor::~Encryptor() {
 bool Encryptor::Init(SymmetricKey* key,
                      Mode mode,
                      const base::StringPiece& iv) {
-  DCHECK(key);
-  DCHECK(CBC == mode || CTR == mode) << "Unsupported mode of operation";
+  DCHECK(CBC == mode || CTR == mode);
+  if (!key)
+    return false;
 
   key_ = key;
   mode_ = mode;
@@ -77,13 +79,19 @@ bool Encryptor::Init(SymmetricKey* key,
 
 bool Encryptor::Encrypt(const base::StringPiece& plaintext,
                         std::string* ciphertext) {
-  CHECK(!plaintext.empty() || (mode_ == CBC));
+  if (plaintext.empty() && mode_ != CBC) {
+    ciphertext->clear();

[wtc, 2011/12/15 02:03:58]

The clear() calls before returning false can all be omitted.

+    return false;
+  }
+
   ScopedPK11Context context(PK11_CreateContextBySymKey(GetMechanism(mode_),
                                                        CKA_ENCRYPT,
                                                        key_->key(),
                                                        param_.get()));
-  if (!context.get())
+  if (!context.get()) {
+    ciphertext->clear();
     return false;
+  }
 
   return (mode_ == CTR) ?
       CryptCTR(context.get(), plaintext, ciphertext) :
@@ -92,12 +100,18 @@ bool Encryptor::Encrypt(const base::StringPiece& plaintext,
 
 bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
                         std::string* plaintext) {
-  CHECK(!ciphertext.empty());
+  if (ciphertext.empty()) {
+    plaintext->clear();
+    return false;
+  }
+
   ScopedPK11Context context(PK11_CreateContextBySymKey(
       GetMechanism(mode_), (mode_ == CTR ? CKA_ENCRYPT : CKA_DECRYPT),
       key_->key(), param_.get()));
-  if (!context.get())
+  if (!context.get()) {
+    plaintext->clear();
     return false;
+  }
 
   return (mode_ == CTR) ?
       CryptCTR(context.get(), ciphertext, plaintext) :
@@ -107,47 +121,46 @@ bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
 bool Encryptor::Crypt(PK11Context* context,
                       const base::StringPiece& input,
                       std::string* output) {
-  size_t output_len = input.size() + AES_BLOCK_SIZE;
-  CHECK_GT(output_len, input.size());
+  std::string result;
+  output->swap(result);
 
-  output->resize(output_len);
-  uint8* output_data =
-      reinterpret_cast<uint8*>(const_cast<char*>(output->data()));
+  size_t output_len = input.size() + AES_BLOCK_SIZE;
+  if (output_len == 0 || output_len < input.size() ||
+      output_len + 1 < input.size()) {
+    return false;  // size_t overflow.
+  }
 
   int input_len = input.size();
   uint8* input_data =
       reinterpret_cast<uint8*>(const_cast<char*>(input.data()));
+  uint8* result_data =
+      reinterpret_cast<uint8*>(WriteInto(&result, output_len + 1));
 
   int op_len;
-  SECStatus rv = PK11_CipherOp(context,
-                               output_data,
-                               &op_len,
-                               output_len,
-                               input_data,
-                               input_len);
-
-  if (SECSuccess != rv) {
-    output->clear();
+  SECStatus rv = PK11_CipherOp(context, result_data, &op_len, output_len,
+                               input_data, input_len);
+  if (SECSuccess != rv)
     return false;
-  }
 
   unsigned int digest_len;
-  rv = PK11_DigestFinal(context,
-                        output_data + op_len,
-                        &digest_len,
+  rv = PK11_DigestFinal(context, result_data + op_len, &digest_len,
                         output_len - op_len);
-  if (SECSuccess != rv) {
-    output->clear();
+  if (SECSuccess != rv)
     return false;
-  }
 
-  output->resize(op_len + digest_len);
+  result.resize(op_len + digest_len);
+  output->swap(result);
   return true;
 }
 
 bool Encryptor::CryptCTR(PK11Context* context,
                          const base::StringPiece& input,
                          std::string* output) {
+  // Work on the result in a local variable, and then only transfer it to
+  // |output| on success to ensure no partial data is returned.
+  std::string result;
+  output->swap(result);
+
   if (!counter_.get()) {
     LOG(ERROR) << "Counter value not set in CTR mode.";
     return false;
@@ -155,42 +168,38 @@ bool Encryptor::CryptCTR(PK11Context* context,
 
   size_t output_len = ((input.size() + AES_BLOCK_SIZE - 1) / AES_BLOCK_SIZE) *
       AES_BLOCK_SIZE;
-  CHECK_GE(output_len, input.size());
-  output->resize(output_len);
-  uint8* output_data =
-      reinterpret_cast<uint8*>(const_cast<char*>(output->data()));
+  if (output_len == 0 || output_len < input.size() ||
+      output_len + 1 < input.size()) {
+    return false;  // size_t overflow.
+  }
+
+  uint8* result_data =
+      reinterpret_cast<uint8*>(WriteInto(&result, output_len + 1));
 
   size_t mask_len;
-  bool ret = GenerateCounterMask(input.size(), output_data, &mask_len);
+  bool ret = GenerateCounterMask(input.size(), result_data, &mask_len);
   if (!ret)
     return false;
 
   CHECK_EQ(mask_len, output_len);
   int op_len;
-  SECStatus rv = PK11_CipherOp(context,
-                               output_data,
-                               &op_len,
-                               output_len,
-                               output_data,
-                               mask_len);
+  SECStatus rv = PK11_CipherOp(context, result_data, &op_len, output_len,
+                               result_data, mask_len);
   if (SECSuccess != rv)
     return false;
   CHECK_EQ(static_cast<int>(mask_len), op_len);
 
   unsigned int digest_len;
-  rv = PK11_DigestFinal(context,
-                        NULL,
-                        &digest_len,
-                        0);
+  rv = PK11_DigestFinal(context, NULL, &digest_len, 0);
   if (SECSuccess != rv)
     return false;
   CHECK(!digest_len);
 
-  // Use |output_data| to mask |input|.
-  MaskMessage(
-      reinterpret_cast<uint8*>(const_cast<char*>(input.data())),
-      input.length(), output_data, output_data);
-  output->resize(input.length());
+  // Use |result_data| to mask |input|.
+  MaskMessage(reinterpret_cast<uint8*>(const_cast<char*>(input.data())),
+              input.size(), result_data, result_data);
+  result.resize(input.size());
+  output->swap(result);
   return true;
 }