Fix various bits of error-handling in NSSDecryptor.

Fix various bits of error-handling in NSSDecryptor.

String may be empty. Base64-decode may fail.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287851

[gab, 2014-08-04 14:57:41]

Looks okay, but I'll defer to Ilya as I really know nothing about NSS.

Cheers,
Gab

[Ilya Sherman, 2014-08-04 21:35:37]

I also know very little about NSS, but these changes seem reasonable.  Is it
possible to add test coverage?

[gab, 2014-08-04 22:18:22]

Hmm.. Who knows NSS then? We should find someone and add a file-specific
owner here.
Le 2014-08-01 18:06, <davidben@chromium.org> a écrit :

> Reviewers: gab,
>
> Description:
> Fix various bits of error-handling in NSSDecryptor.
>
> String may be empty. Base64-decode may fail.
>
> BUG=none
>
> Please review this at https://codereview.chromium.org/427303004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+7, -2 lines):
>   M chrome/utility/importer/nss_decryptor.cc
>
>
> Index: chrome/utility/importer/nss_decryptor.cc
> diff --git a/chrome/utility/importer/nss_decryptor.cc
> b/chrome/utility/importer/nss_decryptor.cc
> index 77a10a0bc044d1901b9f260cb9b980c81d9bc1fa..
> 656b73ff8443a4e76c59c3ade82c62b67c7f879d 100644
> --- a/chrome/utility/importer/nss_decryptor.cc
> +++ b/chrome/utility/importer/nss_decryptor.cc
> @@ -66,12 +66,16 @@ base::string16 NSSDecryptor::Decrypt(const
> std::string& crypt) const {
>    if (!is_nss_initialized_)
>      return base::string16();
>
> +  if (crypt.empty())
> +    return base::string16();
> +
>    // The old style password is encoded in base64. They are identified
>    // by a leading '~'. Otherwise, we should decrypt the text.
>    std::string plain;
>    if (crypt[0] != '~') {
>      std::string decoded_data;
> -    base::Base64Decode(crypt, &decoded_data);
> +    if (!base::Base64Decode(crypt, &decoded_data))
> +      return base::string16();
>      PK11SlotInfo* slot = GetKeySlotForDB();
>      SECStatus result = PK11_Authenticate(slot, PR_TRUE, NULL);
>      if (result != SECSuccess) {
> @@ -98,7 +102,8 @@ base::string16 NSSDecryptor::Decrypt(const std::string&
> crypt) const {
>      FreeSlot(slot);
>    } else {
>      // Deletes the leading '~' before decoding.
> -    base::Base64Decode(crypt.substr(1), &plain);
> +    if (!base::Base64Decode(crypt.substr(1), &plain))
> +      return base::string16();
>    }
>
>    return base::UTF8ToUTF16(plain);
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[davidben, 2014-08-04 22:44:41]

wtc, rsleevi, and I are familiar with NSS to varying degrees (them more than
me). But none of these changes are terribly NSS-specific. It's branching on the
first character of a string without checking the length, and base64 decodes may
fail because some input may not be valid base64.

I can drop in some unit tests perhaps...

[Ilya Sherman, 2014-08-04 22:51:03]

On 2014/08/04 22:44:41, David Benjamin wrote:
> I can drop in some unit tests perhaps...

Yes, please do.  Thanks :)

[wtc, 2014-08-04 23:40:56]

Patch set 1 LGTM. (David, you probably should consider this as a drive-by
review.)

[davidben, 2014-08-05 23:55:01]

Added tests

[Ilya Sherman, 2014-08-06 01:29:16]

LGTM % a nit.  Thanks :)

https://codereview.chromium.org/427303004/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/427303004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:50:
EXPECT_EQ(base::string16(), decryptor_proxy.Decrypt(""));
nit: Prefer std::string() to "".

[wtc, 2014-08-06 17:43:31]

Patch set 2 LGTM.

[davidben, 2014-08-06 17:47:59]

https://codereview.chromium.org/427303004/diff/20001/chrome/utility/importer/...
File chrome/utility/importer/firefox_importer_unittest.cc (right):

https://codereview.chromium.org/427303004/diff/20001/chrome/utility/importer/...
chrome/utility/importer/firefox_importer_unittest.cc:50:
EXPECT_EQ(base::string16(), decryptor_proxy.Decrypt(""));
On 2014/08/06 01:29:16, Ilya Sherman wrote:
> nit: Prefer std::string() to "".

Done.

[davidben, 2014-08-06 17:48:05]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-08-06 17:48:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/427303004/40001

[commit-bot: I haz the power, 2014-08-06 20:48:48]

Change committed as 287851