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 <sys/wait.h>
 #include <unistd.h>
 
+#include <vector>
+
 #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<Certificate> 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<Certificate>& 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<Certificate>& certs) {
+    std::unique_ptr<FILE, decltype(&fclose)> 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<RSAPublicKey>(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<RSAPublicKey>(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<RSAPublicKey>(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<RSAPublicKey>(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<ECPublicKey>(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 <memory>
+#include <vector>
+
 #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<RSAPublicKey>&& rsa_,
+            std::unique_ptr<ECPublicKey>&& 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<RSAPublicKey> rsa;
+    std::unique_ptr<ECPublicKey> 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<Certificate>& keys);
 
-Certificate* load_keys(const char* filename, int* numKeys);
+bool load_keys(const char* filename, std::vector<Certificate>& 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 <sys/types.h>
 #include <sys/stat.h>
 
+#include <memory>
+#include <vector>
+
 #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 <keys>] <package>\n", argv[0]);
         return 2;
     }
-    Certificate* certs = NULL;
-    int num_keys = 0;
 
+    std::vector<Certificate> 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<ECPublicKey>(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<RSAPublicKey>(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<RSAPublicKey>(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<RSAPublicKey>(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;