Verify signed .crx extension installations

BUG=12114
R=erikkay,wtc

Committed: http://src.chromium.org/viewvc/18189

[rafaelw, 2009-05-26 21:34:47]

Erik, please review all of the changes here except the usage of wtc's
verification code.

Wtc, please review my use of the signature_verifier code in
chrome/common/extensions/extension_unpacker.cc

[Erik does not do reviews, 2009-05-26 23:23:29]

http://codereview.chromium.org/115682/diff/2001/3001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115682/diff/2001/3001#newcode581
Line 581: Extension *extension, std::string current_version, bool from_external)
{
string should be a const ref

http://codereview.chromium.org/115682/diff/2001/3001#newcode596
Line 596: if (!root.get())
swap the order of these two tests.  if the second would fail, the first would
have crashed first.

http://codereview.chromium.org/115682/diff/2001/3001#newcode602
Line 602: // without being signed. Before shipping, this needs to return false.
file a bug and reference here

http://codereview.chromium.org/115682/diff/2001/3001#newcode623
Line 623: CompareToInstalledVersion(const std::string id,
move the function name back to the previous line.  see the style guide section
on function declarations for details.

http://codereview.chromium.org/115682/diff/2001/3001#newcode623
Line 623: CompareToInstalledVersion(const std::string id,
const ref

http://codereview.chromium.org/115682/diff/2001/3001#newcode624
Line 624: const std::string new_version_str,
const ref

http://codereview.chromium.org/115682/diff/2001/3001#newcode625
Line 625: std::string *current_version_str) {
add CHECK(current_version_str);

http://codereview.chromium.org/115682/diff/2001/3001#newcode643
Line 643: bool ExtensionsServiceBackend::NeedsReinstall(std::string id,
const ref

http://codereview.chromium.org/115682/diff/2001/3001#newcode644
Line 644: std::string current_version) {
const ref

http://codereview.chromium.org/115682/diff/2001/3001#newcode775
Line 775: if (install_type == Extension::DOWNGRADE)
report an error?

http://codereview.chromium.org/115682/diff/2001/3001#newcode792
Line 792: !VerifySignatureForInstall(&extension, current_version,
from_external))
perhaps this should be renamed VerifySignatureForUpgrade?

http://codereview.chromium.org/115682/diff/2001/3001#newcode1032
Line 1032: NeedsReinstall(id, current_version));
I'm not sure this is correct for external extensions.  They have the ability to
be uninstalled.  I'm not sure how reinstall works, but I'm pretty sure they
shouldn't just be automatically reinstalled at this point.  Maybe ask aa what
the current implementation expects.

http://codereview.chromium.org/115682/diff/2001/3002
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/115682/diff/2001/3002#newcode198
Line 198: // Notify the frontend that the extension had already been installed.
update the comment

http://codereview.chromium.org/115682/diff/2001/3002#newcode206
Line 206: Extension::InstallType ExtensionsServiceBackend::
remove ExtensionsServiceBackend::

http://codereview.chromium.org/115682/diff/2001/3003
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/115682/diff/2001/3003#newcode406
Line 406: TEST_F(ExtensionsServiceTest, UpgradeSignedBad) {
we also need a test where the upgrade key doesn't match the install key

http://codereview.chromium.org/115682/diff/2001/3004
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/115682/diff/2001/3004#newcode38
Line 38: const wchar_t* Extension::kSignatureKey = L"signature";
should there be a matching signature error string here?

http://codereview.chromium.org/115682/diff/2001/3004#newcode133
Line 133: const size_t Extension::kIdSize = 20;  // SHA1 (160 bits) == 20 bytes
this is now incorrect

http://codereview.chromium.org/115682/diff/2001/3004#newcode135
Line 135: // PUBLIC KEY MARKERS
wrap these in an anonymous namespace

http://codereview.chromium.org/115682/diff/2001/3004#newcode140
Line 140: Extension::Extension(const Extension& rhs)
you need to explicitly declare this constructor in the header and get rid of the
DISALLOW_ declaration in the header, but add a private operator= declaration.
Also, since copy constructors are rare in our code, it would be good to add a
comment about why this is needed.

http://codereview.chromium.org/115682/diff/2001/3004#newcode144
Line 144: version_(new Version(*rhs.version_)),
rhs.version.get()

http://codereview.chromium.org/115682/diff/2001/3004#newcode149
Line 149: plugins_dir_(rhs.plugins_dir_),
missing background_url_, toolstrips_ and public_key_

http://codereview.chromium.org/115682/diff/2001/3004#newcode223
Line 223: bool Extension::GetPublicKeyBytes(std::string input, std::string*
output) {
CHECK(output)
const ref input

http://codereview.chromium.org/115682/diff/2001/3004#newcode228
Line 228: input = WideToASCII(CollapseWhitespace(ASCIIToWide(input), true));
these conversions are unfortunate.  could you create a CollapseWhitespaceASCII
instead?  You'll likely want to follow the template pattern of TrimWhitespace.

http://codereview.chromium.org/115682/diff/2001/3004#newcode230
Line 230: std::string(kKeyBeginHeaderMarker).length());
4 char indent

http://codereview.chromium.org/115682/diff/2001/3004#newcode250
Line 250: bool Extension::GenerateIdFromPublicKey(std::string input,
const ref

http://codereview.chromium.org/115682/diff/2001/3004#newcode251
Line 251: std::string* output) {
CHECK(output)

http://codereview.chromium.org/115682/diff/2001/3004#newcode255
Line 255: const unsigned char* ubuf = reinterpret_cast<const unsigned char*>(
use uint8

http://codereview.chromium.org/115682/diff/2001/3004#newcode259
Line 259: SHA256_Update(&ctx, reinterpret_cast<const unsigned
char*>(input.data()),
didn't you mean to use ubuf?

http://codereview.chromium.org/115682/diff/2001/3005
File chrome/common/extensions/extension.h (left):

http://codereview.chromium.org/115682/diff/2001/3005#oldcode60
Line 60: static const wchar_t* kZipHashKey;
Hmm.  Do we want to support the hash key for unsigned extensions?  I guess if
the plan is to remove support for unsigned crx files, then this is OK.

http://codereview.chromium.org/115682/diff/2001/3005
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/115682/diff/2001/3005#newcode52
Line 52: static const wchar_t* kKeyKey;
How about kPublicKeyKey for a tiny bit more clarity

http://codereview.chromium.org/115682/diff/2001/3005#newcode151
Line 151: // the raw bytes of the public key.
It might be worth expanding on this a bit, explaining that these ids are
expected to be unique, so we've implicitly created a 64-bit namespace for
extensions, and that we've chosen 64-bits as the tradeoff of short ids (url bar,
etc.) vs. low-collision likelihood.

http://codereview.chromium.org/115682/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/2001/3006#newcode158
Line 158: // be signed, but Themes may be unsigned.
File and refer to a bug here.  Note that themes may only be unsigned if they
don't have an autoupdate URL.

http://codereview.chromium.org/115682/diff/2001/3006#newcode181
Line 181: const uint8 signature_algorithm[15] = {
a comment about where this comes from would be useful

http://codereview.chromium.org/115682/diff/2001/3006#newcode200
Line 200: SetError("signature verification failed");
use an error constant if you can.  It seems like this error should be different
from the next one since this is a problem with the format of the data, and the
next one is a problem with the actual signature.

[wtc, 2009-05-28 00:29:28]

Your code in extension_unpacker.cc looks good to me.  I
only have some nits below.

http://codereview.chromium.org/115682/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/2001/3006#newcode131
Line 131: char buf[1 << 16];
Note: Coverity warns about using such a large buffer (64KB)
on the stack.  You may want to reduce it.  I suggest 4K.

http://codereview.chromium.org/115682/diff/2001/3006#newcode163
Line 163: SetError("'signature' found without public 'key'");
Nit: it is a little unusual that you quote 'signature'
and 'key' in the error messages in this function.  In
particular "public 'key'" looks a little strange.

http://codereview.chromium.org/115682/diff/2001/3006#newcode178
Line 178: net::Base64Decode(signature_string, &signature);
net::Base64Decode returns bool, so we should check its return
value.

http://codereview.chromium.org/115682/diff/2001/3006#newcode182
Line 182: 0x30, 0x0d,  // a SEQUENCE of length 13 (0xd)
You can omit the comments I added and just list the
bytes in the array initializer.

It is important to note in a comment that the default
signature algorithm is sha1WithRSAEncryption.

http://codereview.chromium.org/115682/diff/2001/3006#newcode206
Line 206: verifier.VerifyUpdate( ubuf, len);
Nit: remove the space after the opening parenthesis.

[rafaelw, 2009-06-04 19:00:08]

Hey guys,

I'm still waiting on aa's commit for generating signed extensions from
chrome.exe, so i can generate crx's suitable for fixing up the unittests here,
but since we're trying to get this all done by friday, can you review my changes
here (modulo the unittests).

http://codereview.chromium.org/115682/diff/2001/3001
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115682/diff/2001/3001#newcode581
Line 581: Extension *extension, std::string current_version, bool from_external)
{
On 2009/05/26 23:23:29, Erik Kay wrote:
> string should be a const ref

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode596
Line 596: if (!root.get())
On 2009/05/26 23:23:29, Erik Kay wrote:
> swap the order of these two tests.  if the second would fail, the first would
> have crashed first.

Indeed. Thanks for the catch. Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode602
Line 602: // without being signed. Before shipping, this needs to return false.
On 2009/05/26 23:23:29, Erik Kay wrote:
> file a bug and reference here

Done. http://code.google.com/p/chromium/issues/detail?id=12689

http://codereview.chromium.org/115682/diff/2001/3001#newcode623
Line 623: CompareToInstalledVersion(const std::string id,
On 2009/05/26 23:23:29, Erik Kay wrote:
> move the function name back to the previous line.  see the style guide section
> on function declarations for details.

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode623
Line 623: CompareToInstalledVersion(const std::string id,
On 2009/05/26 23:23:29, Erik Kay wrote:
> const ref

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode624
Line 624: const std::string new_version_str,
On 2009/05/26 23:23:29, Erik Kay wrote:
> const ref

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode625
Line 625: std::string *current_version_str) {
On 2009/05/26 23:23:29, Erik Kay wrote:
> add CHECK(current_version_str);

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode643
Line 643: bool ExtensionsServiceBackend::NeedsReinstall(std::string id,
On 2009/05/26 23:23:29, Erik Kay wrote:
> const ref

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode644
Line 644: std::string current_version) {
On 2009/05/26 23:23:29, Erik Kay wrote:
> const ref

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode775
Line 775: if (install_type == Extension::DOWNGRADE)
On 2009/05/26 23:23:29, Erik Kay wrote:
> report an error?

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode792
Line 792: !VerifySignatureForInstall(&extension, current_version,
from_external))
On 2009/05/26 23:23:29, Erik Kay wrote:
> perhaps this should be renamed VerifySignatureForUpgrade?

Done.

http://codereview.chromium.org/115682/diff/2001/3001#newcode1032
Line 1032: NeedsReinstall(id, current_version));
On 2009/05/26 23:23:29, Erik Kay wrote:
> I'm not sure this is correct for external extensions.  They have the ability
to
> be uninstalled.  I'm not sure how reinstall works, but I'm pretty sure they
> shouldn't just be automatically reinstalled at this point.  Maybe ask aa what
> the current implementation expects.

Per offline discussion. The check to not auto-reinstall user-uninstalled
external extensions is done with a separate mechanism.

http://codereview.chromium.org/115682/diff/2001/3002
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/115682/diff/2001/3002#newcode198
Line 198: // Notify the frontend that the extension had already been installed.
On 2009/05/26 23:23:29, Erik Kay wrote:
> update the comment

Done.

http://codereview.chromium.org/115682/diff/2001/3002#newcode206
Line 206: Extension::InstallType ExtensionsServiceBackend::
On 2009/05/26 23:23:29, Erik Kay wrote:
> remove ExtensionsServiceBackend::

Done.

http://codereview.chromium.org/115682/diff/2001/3003
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/115682/diff/2001/3003#newcode406
Line 406: TEST_F(ExtensionsServiceTest, UpgradeSignedBad) {
On 2009/05/26 23:23:29, Erik Kay wrote:
> we also need a test where the upgrade key doesn't match the install key

Not done. As discussed offline. Changing the public_key would result in the
extensions_service thinking it was a different extension

http://codereview.chromium.org/115682/diff/2001/3004
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/115682/diff/2001/3004#newcode38
Line 38: const wchar_t* Extension::kSignatureKey = L"signature";
On 2009/05/26 23:23:29, Erik Kay wrote:
> should there be a matching signature error string here?

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode133
Line 133: const size_t Extension::kIdSize = 20;  // SHA1 (160 bits) == 20 bytes
On 2009/05/26 23:23:29, Erik Kay wrote:
> this is now incorrect

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode135
Line 135: // PUBLIC KEY MARKERS
On 2009/05/26 23:23:29, Erik Kay wrote:
> wrap these in an anonymous namespace

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode140
Line 140: Extension::Extension(const Extension& rhs)
On 2009/05/26 23:23:29, Erik Kay wrote:
> you need to explicitly declare this constructor in the header and get rid of
the
> DISALLOW_ declaration in the header, but add a private operator= declaration.
> Also, since copy constructors are rare in our code, it would be good to add a
> comment about why this is needed.

I think this was a merge error. Not sure how it happened. I've removed the
entire constructor.

http://codereview.chromium.org/115682/diff/2001/3004#newcode144
Line 144: version_(new Version(*rhs.version_)),
On 2009/05/26 23:23:29, Erik Kay wrote:
> rhs.version.get()

merge error. removed.

http://codereview.chromium.org/115682/diff/2001/3004#newcode149
Line 149: plugins_dir_(rhs.plugins_dir_),
On 2009/05/26 23:23:29, Erik Kay wrote:
> missing background_url_, toolstrips_ and public_key_

merge error. removed.

http://codereview.chromium.org/115682/diff/2001/3004#newcode223
Line 223: bool Extension::GetPublicKeyBytes(std::string input, std::string*
output) {
On 2009/05/26 23:23:29, Erik Kay wrote:
> CHECK(output)
> const ref input

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode228
Line 228: input = WideToASCII(CollapseWhitespace(ASCIIToWide(input), true));
On 2009/05/26 23:23:29, Erik Kay wrote:
> these conversions are unfortunate.  could you create a CollapseWhitespaceASCII
> instead?  You'll likely want to follow the template pattern of TrimWhitespace.

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode230
Line 230: std::string(kKeyBeginHeaderMarker).length());
On 2009/05/26 23:23:29, Erik Kay wrote:
> 4 char indent

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode250
Line 250: bool Extension::GenerateIdFromPublicKey(std::string input,
On 2009/05/26 23:23:29, Erik Kay wrote:
> const ref

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode251
Line 251: std::string* output) {
On 2009/05/26 23:23:29, Erik Kay wrote:
> CHECK(output)

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode255
Line 255: const unsigned char* ubuf = reinterpret_cast<const unsigned char*>(
On 2009/05/26 23:23:29, Erik Kay wrote:
> use uint8

Done.

http://codereview.chromium.org/115682/diff/2001/3004#newcode259
Line 259: SHA256_Update(&ctx, reinterpret_cast<const unsigned
char*>(input.data()),
On 2009/05/26 23:23:29, Erik Kay wrote:
> didn't you mean to use ubuf?

Yes. Thank you. Done.

http://codereview.chromium.org/115682/diff/2001/3005
File chrome/common/extensions/extension.h (left):

http://codereview.chromium.org/115682/diff/2001/3005#oldcode60
Line 60: static const wchar_t* kZipHashKey;
On 2009/05/26 23:23:29, Erik Kay wrote:
> Hmm.  Do we want to support the hash key for unsigned extensions?  I guess if
> the plan is to remove support for unsigned crx files, then this is OK.

All extensions & themes must now be signed.

http://codereview.chromium.org/115682/diff/2001/3005
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/115682/diff/2001/3005#newcode52
Line 52: static const wchar_t* kKeyKey;
On 2009/05/26 23:23:29, Erik Kay wrote:
> How about kPublicKeyKey for a tiny bit more clarity

Done.

http://codereview.chromium.org/115682/diff/2001/3005#newcode151
Line 151: // the raw bytes of the public key.
On 2009/05/26 23:23:29, Erik Kay wrote:
> It might be worth expanding on this a bit, explaining that these ids are
> expected to be unique, so we've implicitly created a 64-bit namespace for
> extensions, and that we've chosen 64-bits as the tradeoff of short ids (url
bar,
> etc.) vs. low-collision likelihood.

Done.

http://codereview.chromium.org/115682/diff/2001/3006
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/2001/3006#newcode131
Line 131: char buf[1 << 16];
On 2009/05/28 00:29:29, wtc wrote:
> Note: Coverity warns about using such a large buffer (64KB)
> on the stack.  You may want to reduce it.  I suggest 4K.

Done.

http://codereview.chromium.org/115682/diff/2001/3006#newcode158
Line 158: // be signed, but Themes may be unsigned.
On 2009/05/26 23:23:29, Erik Kay wrote:
> File and refer to a bug here.  Note that themes may only be unsigned if they
> don't have an autoupdate URL.

Done. http://code.google.com/p/chromium/issues/detail?id=12696

http://codereview.chromium.org/115682/diff/2001/3006#newcode178
Line 178: net::Base64Decode(signature_string, &signature);
On 2009/05/28 00:29:29, wtc wrote:
> net::Base64Decode returns bool, so we should check its return
> value.

Done.

http://codereview.chromium.org/115682/diff/2001/3006#newcode181
Line 181: const uint8 signature_algorithm[15] = {
On 2009/05/26 23:23:29, Erik Kay wrote:
> a comment about where this comes from would be useful

Done.

http://codereview.chromium.org/115682/diff/2001/3006#newcode182
Line 182: 0x30, 0x0d,  // a SEQUENCE of length 13 (0xd)
On 2009/05/28 00:29:29, wtc wrote:
> You can omit the comments I added and just list the
> bytes in the array initializer.
> 
> It is important to note in a comment that the default
> signature algorithm is sha1WithRSAEncryption.

Done.

http://codereview.chromium.org/115682/diff/2001/3006#newcode200
Line 200: SetError("signature verification failed");
On 2009/05/26 23:23:29, Erik Kay wrote:
> use an error constant if you can.  It seems like this error should be
different
> from the next one since this is a problem with the format of the data, and the
> next one is a problem with the actual signature.

Done. Created error constants for all errors in this file.

http://codereview.chromium.org/115682/diff/2001/3006#newcode206
Line 206: verifier.VerifyUpdate( ubuf, len);
On 2009/05/28 00:29:29, wtc wrote:
> Nit: remove the space after the opening parenthesis.

Done.

[rafaelw, 2009-06-04 22:03:08]

Per Erik's request, I have split out the string_util CollapseWhitespaceASCII
into http://codereview.chromium.org/119199

[Erik does not do reviews, 2009-06-04 23:11:53]

http://codereview.chromium.org/115682/diff/70/71
File chrome/browser/extensions/extension_view_unittest.cc (right):

http://codereview.chromium.org/115682/diff/70/71#newcode28
Line 28: const char* kExtensionId = "pmgpglkggjdpkpgh";
awesome.  I love the look of these ids.

http://codereview.chromium.org/115682/diff/70/72
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115682/diff/70/72#newcode841
Line 841: ReportExtensionInstallError(extension_path,
indent

http://codereview.chromium.org/115682/diff/70/75
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/115682/diff/70/75#newcode189
Line 189: bool Extension::GetPublicKeyBytes(const std::string& input,
std::string* output) {
80 columns

http://codereview.chromium.org/115682/diff/70/75#newcode190
Line 190: CHECK(output);
DCHECK and then return false (sorry, I know I'm changing my mind on CHECK vs.
DCHECK)

http://codereview.chromium.org/115682/diff/70/75#newcode191
Line 191: if (input.length() == 0)
should this be a DCHECK as well?

http://codereview.chromium.org/115682/diff/70/75#newcode212
Line 212: if (!net::Base64Decode(working, output)) {
remove {}

http://codereview.chromium.org/115682/diff/70/75#newcode218
Line 218: static std::string HexAlphaEncode(const void* bytes, size_t size) {
this definitely needs a comment explaining why we do it.

http://codereview.chromium.org/115682/diff/70/75#newcode542
Line 542: // Generate a random ID
indent

http://codereview.chromium.org/115682/diff/70/75#newcode544
Line 544: std::string rand_id = StringPrintf("%x", counter);
I think you need a better name here since this is definitely not random. ;)

http://codereview.chromium.org/115682/diff/70/78
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/70/78#newcode64
Line 64: const char* kCouldNotCreateDirectoryError = "Could not create directory
for unzipping.";
80 cols

http://codereview.chromium.org/115682/diff/70/78#newcode198
Line 198: base::SignatureVerifier verifier;
move declaration closer to use

http://codereview.chromium.org/115682/diff/70/89
File
chrome/test/data/extensions/ui/create_extension_detail_value_expected_output/good-extension2.json
(right):

http://codereview.chromium.org/115682/diff/70/89#newcode2
Line 2: "id": "ofoknjclcmghjfmb",
I thought we shouldn't have the id in the manifest any more

[rafaelw, 2009-06-09 03:07:08]

http://codereview.chromium.org/115682/diff/70/72
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115682/diff/70/72#newcode841
Line 841: ReportExtensionInstallError(extension_path,
On 2009/06/04 23:11:53, Erik Kay wrote:
> indent

Done.

http://codereview.chromium.org/115682/diff/70/75
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/115682/diff/70/75#newcode189
Line 189: bool Extension::GetPublicKeyBytes(const std::string& input,
std::string* output) {
On 2009/06/04 23:11:53, Erik Kay wrote:
> 80 columns

Done.

http://codereview.chromium.org/115682/diff/70/75#newcode190
Line 190: CHECK(output);
On 2009/06/04 23:11:53, Erik Kay wrote:
> DCHECK and then return false (sorry, I know I'm changing my mind on CHECK vs.
> DCHECK)

Done.

http://codereview.chromium.org/115682/diff/70/75#newcode191
Line 191: if (input.length() == 0)
On 2009/06/04 23:11:53, Erik Kay wrote:
> should this be a DCHECK as well?

No. I think it'd rather have it be a failure. I could imagine someone setting a
"key": "" in a manifest. I suppose we could catch that earlier, but it seems
consistent that it's a failure to parse pem bytes.

http://codereview.chromium.org/115682/diff/70/75#newcode212
Line 212: if (!net::Base64Decode(working, output)) {
On 2009/06/04 23:11:53, Erik Kay wrote:
> remove {}

Done.

http://codereview.chromium.org/115682/diff/70/75#newcode218
Line 218: static std::string HexAlphaEncode(const void* bytes, size_t size) {
On 2009/06/04 23:11:53, Erik Kay wrote:
> this definitely needs a comment explaining why we do it.   

Done.

http://codereview.chromium.org/115682/diff/70/75#newcode542
Line 542: // Generate a random ID
On 2009/06/04 23:11:53, Erik Kay wrote:
> indent

Done.

http://codereview.chromium.org/115682/diff/70/75#newcode544
Line 544: std::string rand_id = StringPrintf("%x", counter);
On 2009/06/04 23:11:53, Erik Kay wrote:
> I think you need a better name here since this is definitely not random. ;)

Done.

http://codereview.chromium.org/115682/diff/70/78
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/70/78#newcode64
Line 64: const char* kCouldNotCreateDirectoryError = "Could not create directory
for unzipping.";
On 2009/06/04 23:11:53, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/115682/diff/70/78#newcode198
Line 198: base::SignatureVerifier verifier;
On 2009/06/04 23:11:53, Erik Kay wrote:
> move declaration closer to use

Done.

[Erik does not do reviews, 2009-06-09 16:37:06]

LGTM

http://codereview.chromium.org/115682/diff/175/183
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/175/183#newcode295
Line 295: // The public key ('key' in the manifest) in only present in the
header version
80 cols

http://codereview.chromium.org/115682/diff/175/183#newcode299
Line 299: bool result = header_manifest->GetString(Extension::kPublicKeyKey,
&public_key);
80 cols

[rafaelw, 2009-06-09 22:40:14]

Ok. This is done, but still has the issue with the Crypt API calls failing when
run from the child process.

http://codereview.chromium.org/115682/diff/175/183
File chrome/common/extensions/extension_unpacker.cc (right):

http://codereview.chromium.org/115682/diff/175/183#newcode295
Line 295: // The public key ('key' in the manifest) in only present in the
header version
On 2009/06/09 16:37:06, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/115682/diff/175/183#newcode299
Line 299: bool result = header_manifest->GetString(Extension::kPublicKeyKey,
&public_key);
On 2009/06/09 16:37:06, Erik Kay wrote:
> 80 cols

Done.

[rafaelw, 2009-06-11 00:20:51]

Ok, new crx header parsing code included. Unittests not yet fixed up.

[Aaron Boodman, 2009-06-11 01:02:04]

Looks pretty good to me. One major thing is that there's a good chunk of code
that can be removed.

http://codereview.chromium.org/115682/diff/4030/5029
File chrome/browser/extensions/extension_creator.h (right):

http://codereview.chromium.org/115682/diff/4030/5029#newcode25
Line 25: static const size_t kMaxPublicKeySize = 1 << 16;
Good idea.

http://codereview.chromium.org/115682/diff/4030/5030
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115682/diff/4030/5030#newcode1120
Line 1120: if (install_type == Extension::UPGRADE &&
I do not think that we need this check, or the corresponding method at all. 

By the time we get here, the we have validated the signature against the public
key, and the id is _generated_ from that public key. So it doesn't make sense to
go back and double-check that the existing extension with that same ID has the
same public key. It must.

http://codereview.chromium.org/115682/diff/4030/5033
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/115682/diff/4030/5033#newcode177
Line 177: const size_t Extension::kIdSize = 20;  // first 20 bytes of SHA256
hash of public key
line length. Also, crap, I forgot we were using sha-256 here. In that case, I'll
think more carefully about the length. I was just saying 40 because that is the
size of sha-1.

Anyway, doesn't matter for this CL.

http://codereview.chromium.org/115682/diff/4030/5033#newcode244
Line 244: bool Extension::DecodeIdBytes(const std::string& input,
std::vector<uint8>* output) {
Line length

[rafaelw, 2009-06-11 03:53:09]

http://codereview.chromium.org/115682/diff/4030/5030
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/115682/diff/4030/5030#newcode1120
Line 1120: if (install_type == Extension::UPGRADE &&
On 2009/06/11 01:02:05, Aaron Boodman wrote:
> I do not think that we need this check, or the corresponding method at all. 
> 
> By the time we get here, the we have validated the signature against the
public
> key, and the id is _generated_ from that public key. So it doesn't make sense
to
> go back and double-check that the existing extension with that same ID has the
> same public key. It must.

Right as usual. Done.

http://codereview.chromium.org/115682/diff/4030/5033
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/115682/diff/4030/5033#newcode177
Line 177: const size_t Extension::kIdSize = 20;  // first 20 bytes of SHA256
hash of public key
On 2009/06/11 01:02:05, Aaron Boodman wrote:
> line length. Also, crap, I forgot we were using sha-256 here. In that case,
I'll
> think more carefully about the length. I was just saying 40 because that is
the
> size of sha-1.
> 
> Anyway, doesn't matter for this CL.

Done.

http://codereview.chromium.org/115682/diff/4030/5033#newcode244
Line 244: bool Extension::DecodeIdBytes(const std::string& input,
std::vector<uint8>* output) {
On 2009/06/11 01:02:05, Aaron Boodman wrote:
> Line length

Done.

[Aaron Boodman, 2009-06-11 04:02:17]

LGTM!