From 71e3e09ec2ac4f022e8f9213657746d8cad5dd97 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Tue, 2 Feb 2016 14:02:27 -0800 Subject: recovery: Refactor verifier and verifier_test. Move to using std::vector and std::unique_ptr to manage key certificates to stop memory leaks. Bug: 26908001 Change-Id: Ia5f799bc8dcc036a0ffae5eaa8d9f6e09abd031c --- install.cpp | 13 ++- verifier.cpp | 265 +++++++++++++++++++++++++----------------------------- verifier.h | 23 +++-- verifier_test.cpp | 60 +++++-------- 4 files changed, 168 insertions(+), 193 deletions(-) diff --git a/install.cpp b/install.cpp index 7d88ed72a..c0d007709 100644 --- a/install.cpp +++ b/install.cpp @@ -23,6 +23,8 @@ #include #include +#include + #include "common.h" #include "install.h" #include "mincrypt/rsa.h" @@ -221,19 +223,16 @@ really_install_package(const char *path, bool* wipe_cache, bool needs_mount) return INSTALL_CORRUPT; } - int numKeys; - Certificate* loadedKeys = load_keys(PUBLIC_KEYS_FILE, &numKeys); - if (loadedKeys == NULL) { + std::vector loadedKeys; + if (!load_keys(PUBLIC_KEYS_FILE, loadedKeys)) { LOGE("Failed to load keys\n"); return INSTALL_CORRUPT; } - LOGI("%d key(s) loaded from %s\n", numKeys, PUBLIC_KEYS_FILE); + LOGI("%zu key(s) loaded from %s\n", loadedKeys.size(), PUBLIC_KEYS_FILE); ui->Print("Verifying update package...\n"); - int err; - err = verify_file(map.addr, map.length, loadedKeys, numKeys); - free(loadedKeys); + int err = verify_file(map.addr, map.length, loadedKeys); LOGI("verify_file returned %d\n", err); if (err != VERIFY_SUCCESS) { LOGE("signature verification failed\n"); diff --git a/verifier.cpp b/verifier.cpp index 61e5adf0b..9a2d60c66 100644 --- a/verifier.cpp +++ b/verifier.cpp @@ -113,7 +113,7 @@ static bool read_pkcs7(uint8_t* pkcs7_der, size_t pkcs7_der_len, uint8_t** sig_d // or no key matches the signature). int verify_file(unsigned char* addr, size_t length, - const Certificate* pKeys, unsigned int numKeys) { + const std::vector& keys) { ui->SetProgress(0.0); // An archive with a whole-file signature will end in six bytes: @@ -176,8 +176,7 @@ int verify_file(unsigned char* addr, size_t length, return VERIFY_FAILURE; } - size_t i; - for (i = 4; i < eocd_size-3; ++i) { + for (size_t i = 4; i < eocd_size-3; ++i) { if (eocd[i ] == 0x50 && eocd[i+1] == 0x4b && eocd[i+2] == 0x05 && eocd[i+3] == 0x06) { // if the sequence $50 $4b $05 $06 appears anywhere after @@ -193,8 +192,8 @@ int verify_file(unsigned char* addr, size_t length, bool need_sha1 = false; bool need_sha256 = false; - for (i = 0; i < numKeys; ++i) { - switch (pKeys[i].hash_len) { + for (const auto& key : keys) { + switch (key.hash_len) { case SHA_DIGEST_SIZE: need_sha1 = true; break; case SHA256_DIGEST_SIZE: need_sha256 = true; break; } @@ -225,7 +224,7 @@ int verify_file(unsigned char* addr, size_t length, const uint8_t* sha1 = SHA_final(&sha1_ctx); const uint8_t* sha256 = SHA256_final(&sha256_ctx); - uint8_t* sig_der = NULL; + uint8_t* sig_der = nullptr; size_t sig_der_length = 0; size_t signature_size = signature_start - FOOTER_SIZE; @@ -240,9 +239,10 @@ int verify_file(unsigned char* addr, size_t length, * any key can match, we need to try each before determining a verification * failure has happened. */ - for (i = 0; i < numKeys; ++i) { + size_t i = 0; + for (const auto& key : keys) { const uint8_t* hash; - switch (pKeys[i].hash_len) { + switch (key.hash_len) { case SHA_DIGEST_SIZE: hash = sha1; break; case SHA256_DIGEST_SIZE: hash = sha256; break; default: continue; @@ -250,15 +250,15 @@ int verify_file(unsigned char* addr, size_t length, // The 6 bytes is the "(signature_start) $ff $ff (comment_size)" that // the signing tool appends after the signature itself. - if (pKeys[i].key_type == Certificate::RSA) { + if (key.key_type == Certificate::RSA) { if (sig_der_length < RSANUMBYTES) { // "signature" block isn't big enough to contain an RSA block. LOGI("signature is too short for RSA key %zu\n", i); continue; } - if (!RSA_verify(pKeys[i].rsa, sig_der, RSANUMBYTES, - hash, pKeys[i].hash_len)) { + if (!RSA_verify(key.rsa.get(), sig_der, RSANUMBYTES, + hash, key.hash_len)) { LOGI("failed to verify against RSA key %zu\n", i); continue; } @@ -266,8 +266,8 @@ int verify_file(unsigned char* addr, size_t length, LOGI("whole-file signature verified against RSA key %zu\n", i); free(sig_der); return VERIFY_SUCCESS; - } else if (pKeys[i].key_type == Certificate::EC - && pKeys[i].hash_len == SHA256_DIGEST_SIZE) { + } else if (key.key_type == Certificate::EC + && key.hash_len == SHA256_DIGEST_SIZE) { p256_int r, s; if (!dsa_sig_unpack(sig_der, sig_der_length, &r, &s)) { LOGI("Not a DSA signature block for EC key %zu\n", i); @@ -276,7 +276,7 @@ int verify_file(unsigned char* addr, size_t length, p256_int p256_hash; p256_from_bin(hash, &p256_hash); - if (!p256_ecdsa_verify(&(pKeys[i].ec->x), &(pKeys[i].ec->y), + if (!p256_ecdsa_verify(&(key.ec->x), &(key.ec->y), &p256_hash, &r, &s)) { LOGI("failed to verify against EC key %zu\n", i); continue; @@ -286,8 +286,9 @@ int verify_file(unsigned char* addr, size_t length, free(sig_der); return VERIFY_SUCCESS; } else { - LOGI("Unknown key type %d\n", pKeys[i].key_type); + LOGI("Unknown key type %d\n", key.key_type); } + i++; } free(sig_der); LOGE("failed to verify whole-file signature\n"); @@ -323,140 +324,122 @@ int verify_file(unsigned char* addr, size_t length, // 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 NULL if the file failed to parse, or if it contain zero keys. -Certificate* -load_keys(const char* filename, int* numKeys) { - Certificate* out = NULL; - *numKeys = 0; - - FILE* f = fopen(filename, "r"); - if (f == NULL) { +// 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, "r"), fclose); + if (!f) { LOGE("opening %s: %s\n", filename, strerror(errno)); - goto exit; + return false; } - { - int i; - bool done = false; - while (!done) { - ++*numKeys; - out = (Certificate*)realloc(out, *numKeys * sizeof(Certificate)); - Certificate* cert = out + (*numKeys - 1); - memset(cert, '\0', sizeof(Certificate)); - - char start_char; - if (fscanf(f, " %c", &start_char) != 1) goto exit; - if (start_char == '{') { - // a version 1 key has no version specifier. - cert->key_type = Certificate::RSA; - cert->rsa = (RSAPublicKey*)malloc(sizeof(RSAPublicKey)); - cert->rsa->exponent = 3; - cert->hash_len = SHA_DIGEST_SIZE; - } else if (start_char == 'v') { - int version; - if (fscanf(f, "%d {", &version) != 1) goto exit; - switch (version) { - case 2: - cert->key_type = Certificate::RSA; - cert->rsa = (RSAPublicKey*)malloc(sizeof(RSAPublicKey)); - cert->rsa->exponent = 65537; - cert->hash_len = SHA_DIGEST_SIZE; - break; - case 3: - cert->key_type = Certificate::RSA; - cert->rsa = (RSAPublicKey*)malloc(sizeof(RSAPublicKey)); - cert->rsa->exponent = 3; - cert->hash_len = SHA256_DIGEST_SIZE; - break; - case 4: - cert->key_type = Certificate::RSA; - cert->rsa = (RSAPublicKey*)malloc(sizeof(RSAPublicKey)); - cert->rsa->exponent = 65537; - cert->hash_len = SHA256_DIGEST_SIZE; - break; - case 5: - cert->key_type = Certificate::EC; - cert->ec = (ECPublicKey*)calloc(1, sizeof(ECPublicKey)); - cert->hash_len = SHA256_DIGEST_SIZE; - break; - default: - goto exit; - } + while (true) { + certs.emplace_back(0, Certificate::RSA, nullptr, nullptr); + Certificate& cert = certs.back(); + + 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::RSA; + cert.rsa = std::unique_ptr(new RSAPublicKey); + cert.rsa->exponent = 3; + cert.hash_len = SHA_DIGEST_SIZE; + } else if (start_char == 'v') { + int version; + if (fscanf(f.get(), "%d {", &version) != 1) return false; + switch (version) { + case 2: + cert.key_type = Certificate::RSA; + cert.rsa = std::unique_ptr(new RSAPublicKey); + cert.rsa->exponent = 65537; + cert.hash_len = SHA_DIGEST_SIZE; + break; + case 3: + cert.key_type = Certificate::RSA; + cert.rsa = std::unique_ptr(new RSAPublicKey); + cert.rsa->exponent = 3; + cert.hash_len = SHA256_DIGEST_SIZE; + break; + case 4: + cert.key_type = Certificate::RSA; + cert.rsa = std::unique_ptr(new RSAPublicKey); + cert.rsa->exponent = 65537; + cert.hash_len = SHA256_DIGEST_SIZE; + break; + case 5: + cert.key_type = Certificate::EC; + cert.ec = std::unique_ptr(new ECPublicKey); + cert.hash_len = SHA256_DIGEST_SIZE; + break; + default: + return false; } + } - if (cert->key_type == Certificate::RSA) { - RSAPublicKey* key = cert->rsa; - if (fscanf(f, " %i , 0x%x , { %u", - &(key->len), &(key->n0inv), &(key->n[0])) != 3) { - goto exit; - } - if (key->len != RSANUMWORDS) { - LOGE("key length (%d) does not match expected size\n", key->len); - goto exit; - } - for (i = 1; i < key->len; ++i) { - if (fscanf(f, " , %u", &(key->n[i])) != 1) goto exit; - } - if (fscanf(f, " } , { %u", &(key->rr[0])) != 1) goto exit; - for (i = 1; i < key->len; ++i) { - if (fscanf(f, " , %u", &(key->rr[i])) != 1) goto exit; - } - fscanf(f, " } } "); - - LOGI("read key e=%d hash=%d\n", key->exponent, cert->hash_len); - } else if (cert->key_type == Certificate::EC) { - ECPublicKey* key = cert->ec; - int key_len; - unsigned int byte; - uint8_t x_bytes[P256_NBYTES]; - uint8_t y_bytes[P256_NBYTES]; - if (fscanf(f, " %i , { %u", &key_len, &byte) != 2) goto exit; - if (key_len != P256_NBYTES) { - LOGE("Key length (%d) does not match expected size %d\n", key_len, P256_NBYTES); - goto exit; - } - x_bytes[P256_NBYTES - 1] = byte; - for (i = P256_NBYTES - 2; i >= 0; --i) { - if (fscanf(f, " , %u", &byte) != 1) goto exit; - x_bytes[i] = byte; - } - if (fscanf(f, " } , { %u", &byte) != 1) goto exit; - y_bytes[P256_NBYTES - 1] = byte; - for (i = P256_NBYTES - 2; i >= 0; --i) { - if (fscanf(f, " , %u", &byte) != 1) goto exit; - y_bytes[i] = byte; - } - fscanf(f, " } } "); - p256_from_bin(x_bytes, &key->x); - p256_from_bin(y_bytes, &key->y); - } else { - LOGE("Unknown key type %d\n", cert->key_type); - goto exit; + if (cert.key_type == Certificate::RSA) { + RSAPublicKey* key = cert.rsa.get(); + if (fscanf(f.get(), " %i , 0x%x , { %u", &(key->len), &(key->n0inv), + &(key->n[0])) != 3) { + return false; } - - // if the line ends in a comma, this file has more keys. - switch (fgetc(f)) { - case ',': - // more keys to come. - break; - - case EOF: - done = true; - break; - - default: - LOGE("unexpected character between keys\n"); - goto exit; + if (key->len != RSANUMWORDS) { + LOGE("key length (%d) does not match expected size\n", key->len); + return false; + } + for (int i = 1; i < key->len; ++i) { + if (fscanf(f.get(), " , %u", &(key->n[i])) != 1) return false; + } + if (fscanf(f.get(), " } , { %u", &(key->rr[0])) != 1) return false; + for (int i = 1; i < key->len; ++i) { + if (fscanf(f.get(), " , %u", &(key->rr[i])) != 1) return false; + } + fscanf(f.get(), " } } "); + + LOGI("read key e=%d hash=%d\n", key->exponent, cert.hash_len); + } else if (cert.key_type == Certificate::EC) { + ECPublicKey* key = cert.ec.get(); + int key_len; + unsigned int byte; + uint8_t x_bytes[P256_NBYTES]; + uint8_t y_bytes[P256_NBYTES]; + if (fscanf(f.get(), " %i , { %u", &key_len, &byte) != 2) return false; + if (key_len != P256_NBYTES) { + LOGE("Key length (%d) does not match expected size %d\n", key_len, P256_NBYTES); + return false; + } + x_bytes[P256_NBYTES - 1] = byte; + for (int i = P256_NBYTES - 2; i >= 0; --i) { + if (fscanf(f.get(), " , %u", &byte) != 1) return false; + x_bytes[i] = byte; + } + if (fscanf(f.get(), " } , { %u", &byte) != 1) return false; + y_bytes[P256_NBYTES - 1] = byte; + for (int i = P256_NBYTES - 2; i >= 0; --i) { + if (fscanf(f.get(), " , %u", &byte) != 1) return false; + y_bytes[i] = byte; } + fscanf(f.get(), " } } "); + p256_from_bin(x_bytes, &key->x); + p256_from_bin(y_bytes, &key->y); + } else { + LOGE("Unknown key type %d\n", cert.key_type); + return false; } - } - fclose(f); - return out; + // 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 { + LOGE("unexpected character between keys\n"); + return false; + } + } -exit: - if (f) fclose(f); - free(out); - *numKeys = 0; - return NULL; + return true; } diff --git a/verifier.h b/verifier.h index 15f8d98e4..4eafc7565 100644 --- a/verifier.h +++ b/verifier.h @@ -17,6 +17,9 @@ #ifndef _RECOVERY_VERIFIER_H #define _RECOVERY_VERIFIER_H +#include +#include + #include "mincrypt/p256.h" #include "mincrypt/rsa.h" @@ -25,17 +28,25 @@ typedef struct { p256_int y; } ECPublicKey; -typedef struct { +struct Certificate { typedef enum { RSA, EC, } KeyType; + Certificate(int hash_len_, KeyType key_type_, + std::unique_ptr&& rsa_, + std::unique_ptr&& ec_) : + hash_len(hash_len_), + key_type(key_type_), + rsa(std::move(rsa_)), + ec(std::move(ec_)) { } + int hash_len; // SHA_DIGEST_SIZE (SHA-1) or SHA256_DIGEST_SIZE (SHA-256) KeyType key_type; - RSAPublicKey* rsa; - ECPublicKey* ec; -} Certificate; + std::unique_ptr rsa; + std::unique_ptr ec; +}; /* addr and length define a an update package file that has been * loaded (or mmap'ed, or whatever) into memory. Verify that the file @@ -43,9 +54,9 @@ typedef struct { * one of the constants below. */ int verify_file(unsigned char* addr, size_t length, - const Certificate *pKeys, unsigned int numKeys); + const std::vector& keys); -Certificate* load_keys(const char* filename, int* numKeys); +bool load_keys(const char* filename, std::vector& certs); #define VERIFY_SUCCESS 0 #define VERIFY_FAILURE 1 diff --git a/verifier_test.cpp b/verifier_test.cpp index 21633dc20..2367e0052 100644 --- a/verifier_test.cpp +++ b/verifier_test.cpp @@ -23,6 +23,9 @@ #include #include +#include +#include + #include "common.h" #include "verifier.h" #include "ui.h" @@ -163,56 +166,43 @@ ui_print(const char* format, ...) { va_end(ap); } -static Certificate* add_certificate(Certificate** certsp, int* num_keys, - Certificate::KeyType key_type) { - int i = *num_keys; - *num_keys = *num_keys + 1; - *certsp = (Certificate*) realloc(*certsp, *num_keys * sizeof(Certificate)); - Certificate* certs = *certsp; - certs[i].rsa = NULL; - certs[i].ec = NULL; - certs[i].key_type = key_type; - certs[i].hash_len = SHA_DIGEST_SIZE; - return &certs[i]; -} - -int main(int argc, char **argv) { +int main(int argc, char** argv) { if (argc < 2) { fprintf(stderr, "Usage: %s [-sha256] [-ec | -f4 | -file ] \n", argv[0]); return 2; } - Certificate* certs = NULL; - int num_keys = 0; + std::vector certs; int argn = 1; while (argn < argc) { if (strcmp(argv[argn], "-sha256") == 0) { - if (num_keys == 0) { + if (certs.empty()) { fprintf(stderr, "May only specify -sha256 after key type\n"); return 2; } ++argn; - Certificate* cert = &certs[num_keys - 1]; - cert->hash_len = SHA256_DIGEST_SIZE; + certs.back().hash_len = SHA256_DIGEST_SIZE; } else if (strcmp(argv[argn], "-ec") == 0) { ++argn; - Certificate* cert = add_certificate(&certs, &num_keys, Certificate::EC); - cert->ec = &test_ec_key; + certs.emplace_back(SHA_DIGEST_SIZE, Certificate::EC, + nullptr, std::unique_ptr(new ECPublicKey(test_ec_key))); } else if (strcmp(argv[argn], "-e3") == 0) { ++argn; - Certificate* cert = add_certificate(&certs, &num_keys, Certificate::RSA); - cert->rsa = &test_key; + certs.emplace_back(SHA_DIGEST_SIZE, Certificate::RSA, + std::unique_ptr(new RSAPublicKey(test_key)), nullptr); } else if (strcmp(argv[argn], "-f4") == 0) { ++argn; - Certificate* cert = add_certificate(&certs, &num_keys, Certificate::RSA); - cert->rsa = &test_f4_key; + certs.emplace_back(SHA_DIGEST_SIZE, Certificate::RSA, + std::unique_ptr(new RSAPublicKey(test_f4_key)), nullptr); } else if (strcmp(argv[argn], "-file") == 0) { - if (certs != NULL) { + if (!certs.empty()) { fprintf(stderr, "Cannot specify -file with other certs specified\n"); return 2; } ++argn; - certs = load_keys(argv[argn], &num_keys); + if (!load_keys(argv[argn], certs)) { + fprintf(stderr, "Cannot load keys from %s\n", argv[argn]); + } ++argn; } else if (argv[argn][0] == '-') { fprintf(stderr, "Unknown argument %s\n", argv[argn]); @@ -227,17 +217,9 @@ int main(int argc, char **argv) { return 2; } - if (num_keys == 0) { - certs = (Certificate*) calloc(1, sizeof(Certificate)); - if (certs == NULL) { - fprintf(stderr, "Failure allocating memory for default certificate\n"); - return 1; - } - certs->key_type = Certificate::RSA; - certs->rsa = &test_key; - certs->ec = NULL; - certs->hash_len = SHA_DIGEST_SIZE; - num_keys = 1; + if (certs.empty()) { + certs.emplace_back(SHA_DIGEST_SIZE, Certificate::RSA, + std::unique_ptr(new RSAPublicKey(test_key)), nullptr); } ui = new FakeUI(); @@ -248,7 +230,7 @@ int main(int argc, char **argv) { return 4; } - int result = verify_file(map.addr, map.length, certs, num_keys); + int result = verify_file(map.addr, map.length, certs); if (result == VERIFY_SUCCESS) { printf("VERIFIED\n"); return 0; -- cgit v1.2.3