From cbe93e6506df0d89007d504f47d60a7a37e02475 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Fri, 19 Oct 2018 17:23:21 -0700 Subject: Remove the load_keys function This function is used to parse the result of dumpKeys. It's no longer needed as we are now parsing the public keys from the zipfile. Bug: 116655889 Test: unit tests pass Change-Id: I817906e451664058c644f4329ff499bbe4587ebb --- tests/component/verifier_test.cpp | 84 ++----------- verifier.cpp | 249 -------------------------------------- verifier.h | 2 - 3 files changed, 9 insertions(+), 326 deletions(-) diff --git a/tests/component/verifier_test.cpp b/tests/component/verifier_test.cpp index 14b6060c3..480f3c96c 100644 --- a/tests/component/verifier_test.cpp +++ b/tests/component/verifier_test.cpp @@ -238,8 +238,9 @@ class VerifierTest : public testing::TestWithParam> { } for (auto it = ++args.cbegin(); it != args.cend(); ++it) { - std::string public_key_file = from_testdata_base("testkey_" + *it + ".txt"); - ASSERT_TRUE(load_keys(public_key_file.c_str(), certs)); + std::string public_key_file = from_testdata_base("testkey_" + *it + ".x509.pem"); + certs.emplace_back(0, Certificate::KEY_TYPE_RSA, nullptr, nullptr); + LoadKeyFromFile(public_key_file, &certs.back()); } } @@ -253,70 +254,10 @@ class VerifierSuccessTest : public VerifierTest { class VerifierFailureTest : public VerifierTest { }; -TEST(VerifierTest, load_keys_multiple_keys) { - std::string testkey_v4; - ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v4.txt"), &testkey_v4)); - - std::string testkey_v3; - ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v3.txt"), &testkey_v3)); - - std::string keys = testkey_v4 + "," + testkey_v3 + "," + testkey_v4; - TemporaryFile key_file1; - ASSERT_TRUE(android::base::WriteStringToFile(keys, key_file1.path)); - std::vector certs; - ASSERT_TRUE(load_keys(key_file1.path, certs)); - ASSERT_EQ(3U, certs.size()); -} - -TEST(VerifierTest, load_keys_invalid_keys) { - std::vector certs; - ASSERT_FALSE(load_keys("/doesntexist", certs)); - - // Empty file. - TemporaryFile key_file1; - ASSERT_FALSE(load_keys(key_file1.path, certs)); - - // Invalid contents. - ASSERT_TRUE(android::base::WriteStringToFile("invalid", key_file1.path)); - ASSERT_FALSE(load_keys(key_file1.path, certs)); - - std::string testkey_v4; - ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v4.txt"), &testkey_v4)); - - // Invalid key version: "v4 ..." => "v6 ...". - std::string invalid_key2(testkey_v4); - invalid_key2[1] = '6'; - TemporaryFile key_file2; - ASSERT_TRUE(android::base::WriteStringToFile(invalid_key2, key_file2.path)); - ASSERT_FALSE(load_keys(key_file2.path, certs)); - - // Invalid key content: inserted extra bytes ",2209831334". - std::string invalid_key3(testkey_v4); - invalid_key3.insert(invalid_key2.size() - 2, ",2209831334"); - TemporaryFile key_file3; - ASSERT_TRUE(android::base::WriteStringToFile(invalid_key3, key_file3.path)); - ASSERT_FALSE(load_keys(key_file3.path, certs)); - - // Invalid key: the last key must not end with an extra ','. - std::string invalid_key4 = testkey_v4 + ","; - TemporaryFile key_file4; - ASSERT_TRUE(android::base::WriteStringToFile(invalid_key4, key_file4.path)); - ASSERT_FALSE(load_keys(key_file4.path, certs)); - - // Invalid key separator. - std::string invalid_key5 = testkey_v4 + ";" + testkey_v4; - TemporaryFile key_file5; - ASSERT_TRUE(android::base::WriteStringToFile(invalid_key5, key_file5.path)); - ASSERT_FALSE(load_keys(key_file5.path, certs)); -} - TEST(VerifierTest, BadPackage_AlteredFooter) { - std::string testkey_v3; - ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v3.txt"), &testkey_v3)); - TemporaryFile key_file1; - ASSERT_TRUE(android::base::WriteStringToFile(testkey_v3, key_file1.path)); std::vector certs; - ASSERT_TRUE(load_keys(key_file1.path, certs)); + certs.emplace_back(0, Certificate::KEY_TYPE_RSA, nullptr, nullptr); + LoadKeyFromFile(from_testdata_base("testkey_v3.x509.pem"), &certs.back()); std::string package; ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("otasigned_v3.zip"), &package)); @@ -330,12 +271,9 @@ TEST(VerifierTest, BadPackage_AlteredFooter) { } TEST(VerifierTest, BadPackage_AlteredContent) { - std::string testkey_v3; - ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v3.txt"), &testkey_v3)); - TemporaryFile key_file1; - ASSERT_TRUE(android::base::WriteStringToFile(testkey_v3, key_file1.path)); std::vector certs; - ASSERT_TRUE(load_keys(key_file1.path, certs)); + certs.emplace_back(0, Certificate::KEY_TYPE_RSA, nullptr, nullptr); + LoadKeyFromFile(from_testdata_base("testkey_v3.x509.pem"), &certs.back()); std::string package; ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("otasigned_v3.zip"), &package)); @@ -356,13 +294,9 @@ TEST(VerifierTest, BadPackage_AlteredContent) { } TEST(VerifierTest, BadPackage_SignatureStartOutOfBounds) { - std::string testkey_v3; - ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v3.txt"), &testkey_v3)); - - TemporaryFile key_file; - ASSERT_TRUE(android::base::WriteStringToFile(testkey_v3, key_file.path)); std::vector certs; - ASSERT_TRUE(load_keys(key_file.path, certs)); + certs.emplace_back(0, Certificate::KEY_TYPE_RSA, nullptr, nullptr); + LoadKeyFromFile(from_testdata_base("testkey_v3.x509.pem"), &certs.back()); // Signature start is 65535 (0xffff) while comment size is 0 (Bug: 31914369). std::string package = "\x50\x4b\x05\x06"s + std::string(12, '\0') + "\xff\xff\xff\xff\x00\x00"s; diff --git a/verifier.cpp b/verifier.cpp index 2dfc20808..44bd4e180 100644 --- a/verifier.cpp +++ b/verifier.cpp @@ -308,144 +308,6 @@ int verify_file(const unsigned char* addr, size_t length, const std::vector parse_rsa_key(FILE* file, uint32_t exponent) { - // Read key length in words and n0inv. n0inv is a precomputed montgomery - // parameter derived from the modulus and can be used to speed up - // verification. n0inv is 32 bits wide here, assuming the verification logic - // uses 32 bit arithmetic. However, BoringSSL may use a word size of 64 bits - // internally, in which case we don't have a valid n0inv. Thus, we just - // ignore the montgomery parameters and have BoringSSL recompute them - // internally. If/When the speedup from using the montgomery parameters - // becomes relevant, we can add more sophisticated code here to obtain a - // 64-bit n0inv and initialize the montgomery parameters in the key object. - uint32_t key_len_words = 0; - uint32_t n0inv = 0; - if (fscanf(file, " %i , 0x%x", &key_len_words, &n0inv) != 2) { - return nullptr; - } - - if (key_len_words > 8192 / 32) { - LOG(ERROR) << "key length (" << key_len_words << ") too large"; - return nullptr; - } - - // Read the modulus. - std::unique_ptr modulus(new uint32_t[key_len_words]); - if (fscanf(file, " , { %u", &modulus[0]) != 1) { - return nullptr; - } - for (uint32_t i = 1; i < key_len_words; ++i) { - if (fscanf(file, " , %u", &modulus[i]) != 1) { - return nullptr; - } - } - - // Cconvert from little-endian array of little-endian words to big-endian - // byte array suitable as input for BN_bin2bn. - std::reverse((uint8_t*)modulus.get(), - (uint8_t*)(modulus.get() + key_len_words)); - - // The next sequence of values is the montgomery parameter R^2. Since we - // generally don't have a valid |n0inv|, we ignore this (see comment above). - uint32_t rr_value; - if (fscanf(file, " } , { %u", &rr_value) != 1) { - return nullptr; - } - for (uint32_t i = 1; i < key_len_words; ++i) { - if (fscanf(file, " , %u", &rr_value) != 1) { - return nullptr; - } - } - if (fscanf(file, " } } ") != 0) { - return nullptr; - } - - // Initialize the key. - std::unique_ptr key(RSA_new()); - if (!key) { - return nullptr; - } - - key->n = BN_bin2bn((uint8_t*)modulus.get(), - key_len_words * sizeof(uint32_t), NULL); - if (!key->n) { - return nullptr; - } - - key->e = BN_new(); - if (!key->e || !BN_set_word(key->e, exponent)) { - return nullptr; - } - - return key; -} - -struct BNDeleter { - void operator()(BIGNUM* bn) const { - BN_free(bn); - } -}; - -std::unique_ptr parse_ec_key(FILE* file) { - uint32_t key_len_bytes = 0; - if (fscanf(file, " %i", &key_len_bytes) != 1) { - return nullptr; - } - - std::unique_ptr group( - EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1), EC_GROUP_free); - if (!group) { - return nullptr; - } - - // Verify that |key_len| matches the group order. - if (key_len_bytes != BN_num_bytes(EC_GROUP_get0_order(group.get()))) { - return nullptr; - } - - // Read the public key coordinates. Note that the byte order in the file is - // little-endian, so we convert to big-endian here. - std::unique_ptr bytes(new uint8_t[key_len_bytes]); - std::unique_ptr point[2]; - for (int i = 0; i < 2; ++i) { - unsigned int byte = 0; - if (fscanf(file, " , { %u", &byte) != 1) { - return nullptr; - } - bytes[key_len_bytes - 1] = byte; - - for (size_t i = 1; i < key_len_bytes; ++i) { - if (fscanf(file, " , %u", &byte) != 1) { - return nullptr; - } - bytes[key_len_bytes - i - 1] = byte; - } - - point[i].reset(BN_bin2bn(bytes.get(), key_len_bytes, nullptr)); - if (!point[i]) { - return nullptr; - } - - if (fscanf(file, " }") != 0) { - return nullptr; - } - } - - if (fscanf(file, " } ") != 0) { - return nullptr; - } - - // Create and initialize the key. - std::unique_ptr key(EC_KEY_new()); - if (!key || !EC_KEY_set_group(key.get(), group.get()) || - !EC_KEY_set_public_key_affine_coordinates(key.get(), point[0].get(), - point[1].get())) { - return nullptr; - } - - return key; -} - static std::vector IterateZipEntriesAndSearchForKeys(const ZipArchiveHandle& handle) { void* cookie; ZipString suffix("x509.pem"); @@ -603,114 +465,3 @@ bool LoadCertificateFromBuffer(const std::vector& pem_content, Certific return true; } - -// Reads a file containing one or more public keys as produced by -// DumpPublicKey: this is an RSAPublicKey struct as it would appear -// as a C source literal, eg: -// -// "{64,0xc926ad21,{1795090719,...,-695002876},{-857949815,...,1175080310}}" -// -// For key versions newer than the original 2048-bit e=3 keys -// supported by Android, the string is preceded by a version -// identifier, eg: -// -// "v2 {64,0xc926ad21,{1795090719,...,-695002876},{-857949815,...,1175080310}}" -// -// (Note that the braces and commas in this example are actual -// characters the parser expects to find in the file; the ellipses -// indicate more numbers omitted from this example.) -// -// The file may contain multiple keys in this format, separated by -// commas. The last key must not be followed by a comma. -// -// A Certificate is a pair of an RSAPublicKey and a particular hash -// (we support SHA-1 and SHA-256; we store the hash length to signify -// which is being used). The hash used is implied by the version number. -// -// 1: 2048-bit RSA key with e=3 and SHA-1 hash -// 2: 2048-bit RSA key with e=65537 and SHA-1 hash -// 3: 2048-bit RSA key with e=3 and SHA-256 hash -// 4: 2048-bit RSA key with e=65537 and SHA-256 hash -// 5: 256-bit EC key using the NIST P-256 curve parameters and SHA-256 hash -// -// Returns true on success, and appends the found keys (at least one) to certs. -// Otherwise returns false if the file failed to parse, or if it contains zero -// keys. The contents in certs would be unspecified on failure. -bool load_keys(const char* filename, std::vector& certs) { - std::unique_ptr f(fopen(filename, "re"), fclose); - if (!f) { - PLOG(ERROR) << "error opening " << filename; - return false; - } - - while (true) { - certs.emplace_back(0, Certificate::KEY_TYPE_RSA, nullptr, nullptr); - Certificate& cert = certs.back(); - uint32_t exponent = 0; - - char start_char; - if (fscanf(f.get(), " %c", &start_char) != 1) return false; - if (start_char == '{') { - // a version 1 key has no version specifier. - cert.key_type = Certificate::KEY_TYPE_RSA; - exponent = 3; - cert.hash_len = SHA_DIGEST_LENGTH; - } else if (start_char == 'v') { - int version; - if (fscanf(f.get(), "%d {", &version) != 1) return false; - switch (version) { - case 2: - cert.key_type = Certificate::KEY_TYPE_RSA; - exponent = 65537; - cert.hash_len = SHA_DIGEST_LENGTH; - break; - case 3: - cert.key_type = Certificate::KEY_TYPE_RSA; - exponent = 3; - cert.hash_len = SHA256_DIGEST_LENGTH; - break; - case 4: - cert.key_type = Certificate::KEY_TYPE_RSA; - exponent = 65537; - cert.hash_len = SHA256_DIGEST_LENGTH; - break; - case 5: - cert.key_type = Certificate::KEY_TYPE_EC; - cert.hash_len = SHA256_DIGEST_LENGTH; - break; - default: - return false; - } - } - - if (cert.key_type == Certificate::KEY_TYPE_RSA) { - cert.rsa = parse_rsa_key(f.get(), exponent); - if (!cert.rsa) { - return false; - } - - LOG(INFO) << "read key e=" << exponent << " hash=" << cert.hash_len; - } else if (cert.key_type == Certificate::KEY_TYPE_EC) { - cert.ec = parse_ec_key(f.get()); - if (!cert.ec) { - return false; - } - } else { - LOG(ERROR) << "Unknown key type " << cert.key_type; - return false; - } - - // if the line ends in a comma, this file has more keys. - int ch = fgetc(f.get()); - if (ch == ',') { - // more keys to come. - continue; - } else if (ch == EOF) { - break; - } else { - LOG(ERROR) << "unexpected character between keys"; - return false; - } - } - return true; -} diff --git a/verifier.h b/verifier.h index 944823293..df9a4b648 100644 --- a/verifier.h +++ b/verifier.h @@ -70,8 +70,6 @@ struct Certificate { int verify_file(const unsigned char* addr, size_t length, const std::vector& keys, const std::function& set_progress = nullptr); -bool load_keys(const char* filename, std::vector& certs); - // Checks that the RSA key has a modulus of 2048 bits long, and public exponent is 3 or 65537. bool CheckRSAKey(const std::unique_ptr& rsa); -- cgit v1.2.3