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

crypto/encryptor_mac.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "crypto/encryptor.h"
 
 #include <CommonCrypto/CommonCryptor.h>
 
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "crypto/symmetric_key.h"
 
 namespace crypto {
 
 Encryptor::Encryptor()
     : key_(NULL),
       mode_(CBC) {
 }
 
 Encryptor::~Encryptor() {
 }
 
 bool Encryptor::Init(SymmetricKey* key,
                      Mode mode,
                      const base::StringPiece& iv) {
-  DCHECK(key);
-  DCHECK_EQ(CBC, mode) << "Unsupported mode of operation";
+  if (!key || mode != CBC)
+    return false;
+
   CSSM_DATA raw_key = key->cssm_data();
   if (raw_key.Length != kCCKeySizeAES128 &&
       raw_key.Length != kCCKeySizeAES192 &&
       raw_key.Length != kCCKeySizeAES256)
     return false;
   if (iv.size() != kCCBlockSizeAES128)
     return false;
 
   key_ = key;
   mode_ = mode;
   iv.CopyToString(&iv_);
   return true;
 }
 
 bool Encryptor::Crypt(int /*CCOperation*/ op,
                       const base::StringPiece& input,
                       std::string* output) {
-  DCHECK(key_);
+  output->clear();
+  if (!key_)
+    return false;
+
   CSSM_DATA raw_key = key_->cssm_data();
   // CommonCryptor.h: "A general rule for the size of the output buffer which
   // must be provided by the caller is that for block ciphers, the output
   // length is never larger than the input length plus the block size."
 
+  std::string result;
   size_t output_size = input.size() + iv_.size();
+  if (output_size == 0 || output_size + 1 < input.size())

[wtc, 2011/11/15 02:33:58]

I don't think we need to check output_size + 1 < input.size().
The purpose of the test is very subtle.  I believe you are
detecting size_t overflow of output_size+1 on line 65.
If so, it seems that you should also detect input.size() + iv_.size()
overflowing size_t.

+    return false;
   CCCryptorStatus err = CCCrypt(op,
                                 kCCAlgorithmAES128,
                                 kCCOptionPKCS7Padding,
                                 raw_key.Data, raw_key.Length,
                                 iv_.data(),
                                 input.data(), input.size(),
-                                WriteInto(output, output_size+1),
+                                WriteInto(&result, output_size+1),
                                 output_size,
                                 &output_size);
   if (err) {
-    output->resize(0);
     LOG(ERROR) << "CCCrypt returned " << err;
     return false;
   }
-  output->resize(output_size);
+  result.resize(output_size);
+  output->swap(result);
   return true;
 }
 
 bool Encryptor::Encrypt(const base::StringPiece& plaintext,
                         std::string* ciphertext) {
+  if (plaintext.empty() && mode_ != CBC)
+    return false;
   return Crypt(kCCEncrypt, plaintext, ciphertext);
 }
 
 bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
                         std::string* plaintext) {
+  if (ciphertext.empty())

[wtc, 2011/11/15 02:33:58]

Why don't you also check mode_ != CBC here as you did above>

[Ryan Sleevi, 2011/12/14 06:07:53]

Empty ciphertext should always be treated as an error. While it can be treated
as empty plaintext, there's a subtle difference between a valid, empty CBC block
and a 'no data' block of |ciphertext.tmpy()|

For encryption, encrypting empty CBC blocks is fine (and can be used to confound
some known attacks on IV - eg: BEAST), as they will result in non-empty
ciphertext. Encrypting empty blocks under other types isn't.

+    return false;
   return Crypt(kCCDecrypt, ciphertext, plaintext);
 }
 
 }  // namespace crypto