From 2dc4cabe0639c71014d729dd92eff19289429c89 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 15 Nov 2018 17:45:46 -0800 Subject: [PATCH] adb: stop using adbkey.pub. An adbkey/adbkey.pub pair that doesn't match up results in a hard-to-diagnose scenario where "Always allow from this computer" doesn't work. The private key contains all of the information that's in the public key, so just extract the public key out of the private key instead of storing them separately. Bug: http://b/119634232 Test: rm ~/.android/adbkey.pub; adb kill-server; adb shell true Test: rm ~/.android/adbkey*; adb kill-server; adb shell true Change-Id: I0ae9033dbcd119c12cfb2b3977f1f1954ac800c1 --- adb/adb_auth.h | 1 + adb/client/auth.cpp | 114 ++++++++++++++++--------------------- adb/client/commandline.cpp | 17 +++--- 3 files changed, 57 insertions(+), 75 deletions(-) diff --git a/adb/adb_auth.h b/adb/adb_auth.h index 715e04f2c..2fc84789a 100644 --- a/adb/adb_auth.h +++ b/adb/adb_auth.h @@ -36,6 +36,7 @@ void adb_auth_init(); int adb_auth_keygen(const char* filename); +int adb_auth_pubkey(const char* filename); std::string adb_auth_get_userkey(); std::deque> adb_auth_get_private_keys(); diff --git a/adb/client/auth.cpp b/adb/client/auth.cpp index 71c19b82d..bcb829b1c 100644 --- a/adb/client/auth.cpp +++ b/adb/client/auth.cpp @@ -43,6 +43,7 @@ #include "adb.h" #include "adb_auth.h" +#include "adb_io.h" #include "adb_utils.h" #include "sysdeps.h" #include "transport.h" @@ -52,30 +53,7 @@ static std::map>& g_keys = *new std::map>; static std::map& g_monitored_paths = *new std::map; -static std::string get_user_info() { - LOG(INFO) << "get_user_info..."; - - std::string hostname; - if (getenv("HOSTNAME")) hostname = getenv("HOSTNAME"); -#if !defined(_WIN32) - char buf[64]; - if (hostname.empty() && gethostname(buf, sizeof(buf)) != -1) hostname = buf; -#endif - if (hostname.empty()) hostname = "unknown"; - - std::string username; - if (getenv("LOGNAME")) username = getenv("LOGNAME"); -#if !defined _WIN32 && !defined ADB_HOST_ON_TARGET - if (username.empty() && getlogin()) username = getlogin(); -#endif - if (username.empty()) hostname = "unknown"; - - return " " + username + "@" + hostname; -} - -static bool write_public_keyfile(RSA* private_key, const std::string& private_key_path) { - LOG(INFO) << "write_public_keyfile..."; - +static bool calculate_public_key(std::string* out, RSA* private_key) { uint8_t binary_key_data[ANDROID_PUBKEY_ENCODED_SIZE]; if (!android_pubkey_encode(private_key, binary_key_data, sizeof(binary_key_data))) { LOG(ERROR) << "Failed to convert to public key"; @@ -88,20 +66,10 @@ static bool write_public_keyfile(RSA* private_key, const std::string& private_ke return false; } - std::string content; - content.resize(expected_length); - size_t actual_length = EVP_EncodeBlock(reinterpret_cast(&content[0]), binary_key_data, + out->resize(expected_length); + size_t actual_length = EVP_EncodeBlock(reinterpret_cast(out->data()), binary_key_data, sizeof(binary_key_data)); - content.resize(actual_length); - - content += get_user_info(); - - std::string path(private_key_path + ".pub"); - if (!android::base::WriteStringToFile(content, path)) { - PLOG(ERROR) << "Failed to write public key to '" << path << "'"; - return false; - } - + out->resize(actual_length); return true; } @@ -140,11 +108,6 @@ static int generate_key(const std::string& file) { goto out; } - if (!write_public_keyfile(rsa, file)) { - D("Failed to write public key"); - goto out; - } - ret = 1; out: @@ -170,36 +133,41 @@ static std::string hash_key(RSA* key) { return result; } -static bool read_key_file(const std::string& file) { - LOG(INFO) << "read_key_file '" << file << "'..."; - +static std::shared_ptr read_key_file(const std::string& file) { std::unique_ptr fp(fopen(file.c_str(), "r"), fclose); if (!fp) { PLOG(ERROR) << "Failed to open '" << file << "'"; - return false; + return nullptr; } RSA* key = RSA_new(); if (!PEM_read_RSAPrivateKey(fp.get(), &key, nullptr, nullptr)) { LOG(ERROR) << "Failed to read key"; RSA_free(key); + return nullptr; + } + + return std::shared_ptr(key, RSA_free); +} + +static bool load_key(const std::string& file) { + std::shared_ptr key = read_key_file(file); + if (!key) { return false; } std::lock_guard lock(g_keys_mutex); - std::string fingerprint = hash_key(key); + std::string fingerprint = hash_key(key.get()); if (g_keys.find(fingerprint) != g_keys.end()) { LOG(INFO) << "ignoring already-loaded key: " << file; - RSA_free(key); } else { - g_keys[fingerprint] = std::shared_ptr(key, RSA_free); + g_keys[fingerprint] = std::move(key); } - return true; } -static bool read_keys(const std::string& path, bool allow_dir = true) { - LOG(INFO) << "read_keys '" << path << "'..."; +static bool load_keys(const std::string& path, bool allow_dir = true) { + LOG(INFO) << "load_keys '" << path << "'..."; struct stat st; if (stat(path.c_str(), &st) != 0) { @@ -208,7 +176,7 @@ static bool read_keys(const std::string& path, bool allow_dir = true) { } if (S_ISREG(st.st_mode)) { - return read_key_file(path); + return load_key(path); } else if (S_ISDIR(st.st_mode)) { if (!allow_dir) { // inotify isn't recursive. It would break expectations to load keys in nested @@ -237,7 +205,7 @@ static bool read_keys(const std::string& path, bool allow_dir = true) { continue; } - result |= read_key_file((path + OS_PATH_SEPARATOR + name)); + result |= load_key((path + OS_PATH_SEPARATOR + name)); } return result; } @@ -250,7 +218,7 @@ static std::string get_user_key_path() { return adb_get_android_dir_path() + OS_PATH_SEPARATOR + "adbkey"; } -static bool get_user_key() { +static bool generate_userkey() { std::string path = get_user_key_path(); if (path.empty()) { PLOG(ERROR) << "Error getting user key filename"; @@ -266,7 +234,7 @@ static bool get_user_key() { } } - return read_key_file(path); + return load_key(path); } static std::set get_vendor_keys() { @@ -320,26 +288,42 @@ static std::string adb_auth_sign(RSA* key, const char* token, size_t token_size) return result; } +static bool pubkey_from_privkey(std::string* out, const std::string& path) { + std::shared_ptr privkey = read_key_file(path); + if (!privkey) { + return false; + } + return calculate_public_key(out, privkey.get()); +} + std::string adb_auth_get_userkey() { std::string path = get_user_key_path(); if (path.empty()) { PLOG(ERROR) << "Error getting user key filename"; return ""; } - path += ".pub"; - std::string content; - if (!android::base::ReadFileToString(path, &content)) { - PLOG(ERROR) << "Can't load '" << path << "'"; + std::string result; + if (!pubkey_from_privkey(&result, path)) { return ""; } - return content; + return result; } int adb_auth_keygen(const char* filename) { return (generate_key(filename) == 0); } +int adb_auth_pubkey(const char* filename) { + std::string pubkey; + if (!pubkey_from_privkey(&pubkey, filename)) { + return 1; + } + pubkey.push_back('\n'); + + return WriteFdExactly(STDOUT_FILENO, pubkey.data(), pubkey.size()) ? 0 : 1; +} + #if defined(__linux__) static void adb_auth_inotify_update(int fd, unsigned fd_event, void*) { LOG(INFO) << "adb_auth_inotify_update called"; @@ -380,7 +364,7 @@ static void adb_auth_inotify_update(int fd, unsigned fd_event, void*) { LOG(INFO) << "ignoring new directory at '" << path << "'"; } else { LOG(INFO) << "observed new file at '" << path << "'"; - read_keys(path, false); + load_keys(path, false); } } else { LOG(WARNING) << "unmonitored event for " << path << ": 0x" << std::hex @@ -420,8 +404,8 @@ static void adb_auth_inotify_init(const std::set& paths) { void adb_auth_init() { LOG(INFO) << "adb_auth_init..."; - if (!get_user_key()) { - LOG(ERROR) << "Failed to get user key"; + if (!generate_userkey()) { + LOG(ERROR) << "Failed to generate user key"; return; } @@ -432,7 +416,7 @@ void adb_auth_init() { #endif for (const std::string& path : key_paths) { - read_keys(path.c_str()); + load_keys(path.c_str()); } } diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp index b5bed2881..c11052d10 100644 --- a/adb/client/commandline.cpp +++ b/adb/client/commandline.cpp @@ -185,7 +185,6 @@ static void help() { " enable-verity re-enable dm-verity checking on userdebug builds\n" " keygen FILE\n" " generate adb public/private key; private key stored in FILE,\n" - " public key stored in FILE.pub (existing files overwritten)\n" "\n" "scripting:\n" " wait-for[-TRANSPORT]-STATE\n" @@ -1756,14 +1755,14 @@ int adb_commandline(int argc, const char** argv) { // Always print key generation information for keygen command. adb_trace_enable(AUTH); return adb_auth_keygen(argv[1]); - } - else if (!strcmp(argv[0], "jdwp")) { + } else if (!strcmp(argv[0], "pubkey")) { + if (argc != 2) error_exit("pubkey requires an argument"); + return adb_auth_pubkey(argv[1]); + } else if (!strcmp(argv[0], "jdwp")) { return adb_connect_command("jdwp"); - } - else if (!strcmp(argv[0], "track-jdwp")) { + } else if (!strcmp(argv[0], "track-jdwp")) { return adb_connect_command("track-jdwp"); - } - else if (!strcmp(argv[0], "track-devices")) { + } else if (!strcmp(argv[0], "track-devices")) { return adb_connect_command("host:track-devices"); } else if (!strcmp(argv[0], "raw")) { if (argc != 2) { @@ -1772,13 +1771,11 @@ int adb_commandline(int argc, const char** argv) { return adb_connect_command(argv[1]); } - /* "adb /?" is a common idiom under Windows */ else if (!strcmp(argv[0], "--help") || !strcmp(argv[0], "help") || !strcmp(argv[0], "/?")) { help(); return 0; - } - else if (!strcmp(argv[0], "--version") || !strcmp(argv[0], "version")) { + } else if (!strcmp(argv[0], "--version") || !strcmp(argv[0], "version")) { fprintf(stdout, "%s", adb_version().c_str()); return 0; } else if (!strcmp(argv[0], "features")) {