Snap for 5096429 from 8787d4a1c804520dd4217ea2e4cbe4cb1d82d9ac to qt-release
Change-Id: Ia3cc5d1bca3709d842bcd358bd8bc33cc1787475
diff --git a/minui/graphics_drm.cpp b/minui/graphics_drm.cpp
index 81b49fd..f81fd9d 100644
--- a/minui/graphics_drm.cpp
+++ b/minui/graphics_drm.cpp
@@ -24,74 +24,37 @@
#include <sys/types.h>
#include <unistd.h>
+#include <memory>
+
+#include <android-base/macros.h>
+#include <android-base/stringprintf.h>
+#include <android-base/unique_fd.h>
#include <drm_fourcc.h>
#include <xf86drm.h>
#include <xf86drmMode.h>
#include "minui/minui.h"
-#define ARRAY_SIZE(A) (sizeof(A)/sizeof(*(A)))
-
-MinuiBackendDrm::MinuiBackendDrm()
- : GRSurfaceDrms(), main_monitor_crtc(nullptr), main_monitor_connector(nullptr), drm_fd(-1) {}
-
-void MinuiBackendDrm::DrmDisableCrtc(int drm_fd, drmModeCrtc* crtc) {
- if (crtc) {
- drmModeSetCrtc(drm_fd, crtc->crtc_id,
- 0, // fb_id
- 0, 0, // x,y
- nullptr, // connectors
- 0, // connector_count
- nullptr); // mode
- }
-}
-
-int MinuiBackendDrm::DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc, GRSurfaceDrm* surface) {
- int ret = drmModeSetCrtc(drm_fd, crtc->crtc_id, surface->fb_id, 0, 0, // x,y
- &main_monitor_connector->connector_id,
- 1, // connector_count
- &main_monitor_crtc->mode);
-
- if (ret) {
- printf("drmModeSetCrtc failed ret=%d\n", ret);
+GRSurfaceDrm::~GRSurfaceDrm() {
+ if (mmapped_buffer_) {
+ munmap(mmapped_buffer_, row_bytes * height);
}
- return ret;
-}
-
-void MinuiBackendDrm::Blank(bool blank) {
- if (blank) {
- DrmDisableCrtc(drm_fd, main_monitor_crtc);
- } else {
- DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[current_buffer]);
- }
-}
-
-void MinuiBackendDrm::DrmDestroySurface(GRSurfaceDrm* surface) {
- if (!surface) return;
-
- if (surface->mmapped_buffer_) {
- munmap(surface->mmapped_buffer_, surface->row_bytes * surface->height);
- }
-
- if (surface->fb_id) {
- int ret = drmModeRmFB(drm_fd, surface->fb_id);
- if (ret) {
- printf("drmModeRmFB failed ret=%d\n", ret);
+ if (fb_id) {
+ if (drmModeRmFB(drm_fd_, fb_id) != 0) {
+ perror("Failed to drmModeRmFB");
+ // Falling through to free other resources.
}
}
- if (surface->handle) {
+ if (handle) {
drm_gem_close gem_close = {};
- gem_close.handle = surface->handle;
+ gem_close.handle = handle;
- int ret = drmIoctl(drm_fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
- if (ret) {
- printf("DRM_IOCTL_GEM_CLOSE failed ret=%d\n", ret);
+ if (drmIoctl(drm_fd_, DRM_IOCTL_GEM_CLOSE, &gem_close) != 0) {
+ perror("Failed to DRM_IOCTL_GEM_CLOSE");
}
}
-
- delete surface;
}
static int drm_format_to_bpp(uint32_t format) {
@@ -111,10 +74,7 @@
}
}
-GRSurfaceDrm* MinuiBackendDrm::DrmCreateSurface(int width, int height) {
- GRSurfaceDrm* surface = new GRSurfaceDrm;
- *surface = {};
-
+std::unique_ptr<GRSurfaceDrm> GRSurfaceDrm::Create(int drm_fd, int width, int height) {
uint32_t format;
PixelFormat pixel_format = gr_pixel_format();
// PixelFormat comes in byte order, whereas DRM_FORMAT_* uses little-endian
@@ -137,12 +97,12 @@
create_dumb.bpp = drm_format_to_bpp(format);
create_dumb.flags = 0;
- int ret = drmIoctl(drm_fd, DRM_IOCTL_MODE_CREATE_DUMB, &create_dumb);
- if (ret) {
- printf("DRM_IOCTL_MODE_CREATE_DUMB failed ret=%d\n", ret);
- DrmDestroySurface(surface);
+ if (drmIoctl(drm_fd, DRM_IOCTL_MODE_CREATE_DUMB, &create_dumb) != 0) {
+ perror("Failed to DRM_IOCTL_MODE_CREATE_DUMB");
return nullptr;
}
+
+ std::unique_ptr<GRSurfaceDrm> surface = std::make_unique<GRSurfaceDrm>(drm_fd);
surface->handle = create_dumb.handle;
uint32_t handles[4], pitches[4], offsets[4];
@@ -151,20 +111,16 @@
pitches[0] = create_dumb.pitch;
offsets[0] = 0;
- ret =
- drmModeAddFB2(drm_fd, width, height, format, handles, pitches, offsets, &(surface->fb_id), 0);
- if (ret) {
- printf("drmModeAddFB2 failed ret=%d\n", ret);
- DrmDestroySurface(surface);
+ if (drmModeAddFB2(drm_fd, width, height, format, handles, pitches, offsets, &surface->fb_id, 0) !=
+ 0) {
+ perror("Failed to drmModeAddFB2");
return nullptr;
}
drm_mode_map_dumb map_dumb = {};
map_dumb.handle = create_dumb.handle;
- ret = drmIoctl(drm_fd, DRM_IOCTL_MODE_MAP_DUMB, &map_dumb);
- if (ret) {
- printf("DRM_IOCTL_MODE_MAP_DUMB failed ret=%d\n", ret);
- DrmDestroySurface(surface);
+ if (drmIoctl(drm_fd, DRM_IOCTL_MODE_MAP_DUMB, &map_dumb) != 0) {
+ perror("Failed to DRM_IOCTL_MODE_MAP_DUMB");
return nullptr;
}
@@ -175,14 +131,44 @@
auto mmapped = mmap(nullptr, surface->height * surface->row_bytes, PROT_READ | PROT_WRITE,
MAP_SHARED, drm_fd, map_dumb.offset);
if (mmapped == MAP_FAILED) {
- perror("mmap() failed");
- DrmDestroySurface(surface);
+ perror("Failed to mmap()");
return nullptr;
}
surface->mmapped_buffer_ = static_cast<uint8_t*>(mmapped);
return surface;
}
+void MinuiBackendDrm::DrmDisableCrtc(int drm_fd, drmModeCrtc* crtc) {
+ if (crtc) {
+ drmModeSetCrtc(drm_fd, crtc->crtc_id,
+ 0, // fb_id
+ 0, 0, // x,y
+ nullptr, // connectors
+ 0, // connector_count
+ nullptr); // mode
+ }
+}
+
+bool MinuiBackendDrm::DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc,
+ const std::unique_ptr<GRSurfaceDrm>& surface) {
+ if (drmModeSetCrtc(drm_fd, crtc->crtc_id, surface->fb_id, 0, 0, // x,y
+ &main_monitor_connector->connector_id,
+ 1, // connector_count
+ &main_monitor_crtc->mode) != 0) {
+ perror("Failed to drmModeSetCrtc");
+ return false;
+ }
+ return true;
+}
+
+void MinuiBackendDrm::Blank(bool blank) {
+ if (blank) {
+ DrmDisableCrtc(drm_fd, main_monitor_crtc);
+ } else {
+ DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[current_buffer]);
+ }
+}
+
static drmModeCrtc* find_crtc_for_connector(int fd, drmModeRes* resources,
drmModeConnector* connector) {
// Find the encoder. If we already have one, just use it.
@@ -264,7 +250,7 @@
do {
main_monitor_connector = find_used_connector_by_type(fd, resources, kConnectorPriority[i]);
i++;
- } while (!main_monitor_connector && i < ARRAY_SIZE(kConnectorPriority));
+ } while (!main_monitor_connector && i < arraysize(kConnectorPriority));
/* If we didn't find a connector, grab the first one that is connected. */
if (!main_monitor_connector) {
@@ -298,60 +284,53 @@
GRSurface* MinuiBackendDrm::Init() {
drmModeRes* res = nullptr;
+ drm_fd = -1;
/* Consider DRM devices in order. */
for (int i = 0; i < DRM_MAX_MINOR; i++) {
- char* dev_name;
- int ret = asprintf(&dev_name, DRM_DEV_NAME, DRM_DIR_NAME, i);
- if (ret < 0) continue;
+ auto dev_name = android::base::StringPrintf(DRM_DEV_NAME, DRM_DIR_NAME, i);
+ android::base::unique_fd fd(open(dev_name.c_str(), O_RDWR));
+ if (fd == -1) continue;
- drm_fd = open(dev_name, O_RDWR, 0);
- free(dev_name);
- if (drm_fd < 0) continue;
-
- uint64_t cap = 0;
/* We need dumb buffers. */
- ret = drmGetCap(drm_fd, DRM_CAP_DUMB_BUFFER, &cap);
- if (ret || cap == 0) {
- close(drm_fd);
+ if (uint64_t cap = 0; drmGetCap(fd.get(), DRM_CAP_DUMB_BUFFER, &cap) != 0 || cap == 0) {
continue;
}
- res = drmModeGetResources(drm_fd);
+ res = drmModeGetResources(fd.get());
if (!res) {
- close(drm_fd);
continue;
}
/* Use this device if it has at least one connected monitor. */
if (res->count_crtcs > 0 && res->count_connectors > 0) {
- if (find_first_connected_connector(drm_fd, res)) break;
+ if (find_first_connected_connector(fd.get(), res)) {
+ drm_fd = fd.release();
+ break;
+ }
}
drmModeFreeResources(res);
- close(drm_fd);
res = nullptr;
}
- if (drm_fd < 0 || res == nullptr) {
- perror("cannot find/open a drm device");
+ if (drm_fd == -1 || res == nullptr) {
+ perror("Failed to find/open a drm device");
return nullptr;
}
uint32_t selected_mode;
main_monitor_connector = FindMainMonitor(drm_fd, res, &selected_mode);
-
if (!main_monitor_connector) {
- printf("main_monitor_connector not found\n");
+ fprintf(stderr, "Failed to find main_monitor_connector\n");
drmModeFreeResources(res);
close(drm_fd);
return nullptr;
}
main_monitor_crtc = find_crtc_for_connector(drm_fd, res, main_monitor_connector);
-
if (!main_monitor_crtc) {
- printf("main_monitor_crtc not found\n");
+ fprintf(stderr, "Failed to find main_monitor_crtc\n");
drmModeFreeResources(res);
close(drm_fd);
return nullptr;
@@ -366,21 +345,20 @@
drmModeFreeResources(res);
- GRSurfaceDrms[0] = DrmCreateSurface(width, height);
- GRSurfaceDrms[1] = DrmCreateSurface(width, height);
+ GRSurfaceDrms[0] = GRSurfaceDrm::Create(drm_fd, width, height);
+ GRSurfaceDrms[1] = GRSurfaceDrm::Create(drm_fd, width, height);
if (!GRSurfaceDrms[0] || !GRSurfaceDrms[1]) {
- // GRSurfaceDrms and drm_fd should be freed in d'tor.
return nullptr;
}
current_buffer = 0;
// We will likely encounter errors in the backend functions (i.e. Flip) if EnableCrtc fails.
- if (DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[1]) != 0) {
+ if (!DrmEnableCrtc(drm_fd, main_monitor_crtc, GRSurfaceDrms[1])) {
return nullptr;
}
- return GRSurfaceDrms[0];
+ return GRSurfaceDrms[0].get();
}
static void page_flip_complete(__unused int fd,
@@ -393,12 +371,9 @@
GRSurface* MinuiBackendDrm::Flip() {
bool ongoing_flip = true;
-
- int ret = drmModePageFlip(drm_fd, main_monitor_crtc->crtc_id,
- GRSurfaceDrms[current_buffer]->fb_id,
- DRM_MODE_PAGE_FLIP_EVENT, &ongoing_flip);
- if (ret < 0) {
- printf("drmModePageFlip failed ret=%d\n", ret);
+ if (drmModePageFlip(drm_fd, main_monitor_crtc->crtc_id, GRSurfaceDrms[current_buffer]->fb_id,
+ DRM_MODE_PAGE_FLIP_EVENT, &ongoing_flip) != 0) {
+ perror("Failed to drmModePageFlip");
return nullptr;
}
@@ -408,9 +383,8 @@
.events = POLLIN
};
- ret = poll(&fds, 1, -1);
- if (ret == -1 || !(fds.revents & POLLIN)) {
- printf("poll() failed on drm fd\n");
+ if (poll(&fds, 1, -1) == -1 || !(fds.revents & POLLIN)) {
+ perror("Failed to poll() on drm fd");
break;
}
@@ -419,21 +393,18 @@
.page_flip_handler = page_flip_complete
};
- ret = drmHandleEvent(drm_fd, &evctx);
- if (ret != 0) {
- printf("drmHandleEvent failed ret=%d\n", ret);
+ if (drmHandleEvent(drm_fd, &evctx) != 0) {
+ perror("Failed to drmHandleEvent");
break;
}
}
current_buffer = 1 - current_buffer;
- return GRSurfaceDrms[current_buffer];
+ return GRSurfaceDrms[current_buffer].get();
}
MinuiBackendDrm::~MinuiBackendDrm() {
DrmDisableCrtc(drm_fd, main_monitor_crtc);
- DrmDestroySurface(GRSurfaceDrms[0]);
- DrmDestroySurface(GRSurfaceDrms[1]);
drmModeFreeCrtc(main_monitor_crtc);
drmModeFreeConnector(main_monitor_connector);
close(drm_fd);
diff --git a/minui/graphics_drm.h b/minui/graphics_drm.h
index f3aad6b..02db89f 100644
--- a/minui/graphics_drm.h
+++ b/minui/graphics_drm.h
@@ -18,6 +18,9 @@
#include <stdint.h>
+#include <memory>
+
+#include <android-base/macros.h>
#include <xf86drmMode.h>
#include "graphics.h"
@@ -25,6 +28,12 @@
class GRSurfaceDrm : public GRSurface {
public:
+ explicit GRSurfaceDrm(int drm_fd) : drm_fd_(drm_fd) {}
+ ~GRSurfaceDrm() override;
+
+ // Creates a GRSurfaceDrm instance.
+ static std::unique_ptr<GRSurfaceDrm> Create(int drm_fd, int width, int height);
+
uint8_t* data() override {
return mmapped_buffer_;
}
@@ -32,30 +41,33 @@
private:
friend class MinuiBackendDrm;
- uint32_t fb_id;
- uint32_t handle;
+ const int drm_fd_;
+
+ uint32_t fb_id{ 0 };
+ uint32_t handle{ 0 };
uint8_t* mmapped_buffer_{ nullptr };
+
+ DISALLOW_COPY_AND_ASSIGN(GRSurfaceDrm);
};
class MinuiBackendDrm : public MinuiBackend {
public:
+ MinuiBackendDrm() = default;
+ ~MinuiBackendDrm() override;
+
GRSurface* Init() override;
GRSurface* Flip() override;
void Blank(bool) override;
- ~MinuiBackendDrm() override;
- MinuiBackendDrm();
private:
void DrmDisableCrtc(int drm_fd, drmModeCrtc* crtc);
- int DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc, GRSurfaceDrm* surface);
- GRSurfaceDrm* DrmCreateSurface(int width, int height);
- void DrmDestroySurface(GRSurfaceDrm* surface);
+ bool DrmEnableCrtc(int drm_fd, drmModeCrtc* crtc, const std::unique_ptr<GRSurfaceDrm>& surface);
void DisableNonMainCrtcs(int fd, drmModeRes* resources, drmModeCrtc* main_crtc);
drmModeConnector* FindMainMonitor(int fd, drmModeRes* resources, uint32_t* mode_index);
- GRSurfaceDrm* GRSurfaceDrms[2];
- int current_buffer;
- drmModeCrtc* main_monitor_crtc;
- drmModeConnector* main_monitor_connector;
- int drm_fd;
+ std::unique_ptr<GRSurfaceDrm> GRSurfaceDrms[2];
+ int current_buffer{ 0 };
+ drmModeCrtc* main_monitor_crtc{ nullptr };
+ drmModeConnector* main_monitor_connector{ nullptr };
+ int drm_fd{ -1 };
};
diff --git a/tests/component/verifier_test.cpp b/tests/component/verifier_test.cpp
index d110c37..480f3c9 100644
--- a/tests/component/verifier_test.cpp
+++ b/tests/component/verifier_test.cpp
@@ -30,6 +30,9 @@
#include <android-base/test_utils.h>
#include <android-base/unique_fd.h>
#include <gtest/gtest.h>
+#include <openssl/bn.h>
+#include <openssl/ec.h>
+#include <openssl/nid.h>
#include <ziparchive/zip_writer.h>
#include "common/test_constants.h"
@@ -148,6 +151,35 @@
VerifyPackageWithSingleCertificate("otasigned_v5.zip", std::move(cert));
}
+TEST(VerifierTest, LoadCertificateFromBuffer_check_rsa_keys) {
+ std::unique_ptr<RSA, RSADeleter> rsa(RSA_new());
+ std::unique_ptr<BIGNUM, decltype(&BN_free)> exponent(BN_new(), BN_free);
+ BN_set_word(exponent.get(), 3);
+ RSA_generate_key_ex(rsa.get(), 2048, exponent.get(), nullptr);
+ ASSERT_TRUE(CheckRSAKey(rsa));
+
+ // Exponent is expected to be 3 or 65537
+ BN_set_word(exponent.get(), 17);
+ RSA_generate_key_ex(rsa.get(), 2048, exponent.get(), nullptr);
+ ASSERT_FALSE(CheckRSAKey(rsa));
+
+ // Modulus is expected to be 2048.
+ BN_set_word(exponent.get(), 3);
+ RSA_generate_key_ex(rsa.get(), 1024, exponent.get(), nullptr);
+ ASSERT_FALSE(CheckRSAKey(rsa));
+}
+
+TEST(VerifierTest, LoadCertificateFromBuffer_check_ec_keys) {
+ std::unique_ptr<EC_KEY, ECKEYDeleter> ec(EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
+ ASSERT_EQ(1, EC_KEY_generate_key(ec.get()));
+ ASSERT_TRUE(CheckECKey(ec));
+
+ // Expects 256-bit EC key with curve NIST P-256
+ ec.reset(EC_KEY_new_by_curve_name(NID_secp224r1));
+ ASSERT_EQ(1, EC_KEY_generate_key(ec.get()));
+ ASSERT_FALSE(CheckECKey(ec));
+}
+
TEST(VerifierTest, LoadKeysFromZipfile_empty_archive) {
TemporaryFile otacerts;
BuildCertificateArchive({}, otacerts.release());
@@ -206,8 +238,9 @@
}
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());
}
}
@@ -221,70 +254,10 @@
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<Certificate> certs;
- ASSERT_TRUE(load_keys(key_file1.path, certs));
- ASSERT_EQ(3U, certs.size());
-}
-
-TEST(VerifierTest, load_keys_invalid_keys) {
- std::vector<Certificate> 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<Certificate> 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));
@@ -298,12 +271,9 @@
}
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<Certificate> 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));
@@ -324,13 +294,9 @@
}
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<Certificate> 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 2101dcb..44bd4e1 100644
--- a/verifier.cpp
+++ b/verifier.cpp
@@ -308,144 +308,6 @@
return VERIFY_FAILURE;
}
-std::unique_ptr<RSA, RSADeleter> 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<uint32_t[]> 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<RSA, RSADeleter> 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<EC_KEY, ECKEYDeleter> parse_ec_key(FILE* file) {
- uint32_t key_len_bytes = 0;
- if (fscanf(file, " %i", &key_len_bytes) != 1) {
- return nullptr;
- }
-
- std::unique_ptr<EC_GROUP, void (*)(EC_GROUP*)> 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<uint8_t[]> bytes(new uint8_t[key_len_bytes]);
- std::unique_ptr<BIGNUM, BNDeleter> 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<EC_KEY, ECKEYDeleter> 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<Certificate> IterateZipEntriesAndSearchForKeys(const ZipArchiveHandle& handle) {
void* cookie;
ZipString suffix("x509.pem");
@@ -500,6 +362,48 @@
return result;
}
+bool CheckRSAKey(const std::unique_ptr<RSA, RSADeleter>& rsa) {
+ if (!rsa) {
+ return false;
+ }
+
+ const BIGNUM* out_n;
+ const BIGNUM* out_e;
+ RSA_get0_key(rsa.get(), &out_n, &out_e, nullptr /* private exponent */);
+ auto modulus_bits = BN_num_bits(out_n);
+ if (modulus_bits != 2048) {
+ LOG(ERROR) << "Modulus should be 2048 bits long, actual: " << modulus_bits;
+ return false;
+ }
+
+ BN_ULONG exponent = BN_get_word(out_e);
+ if (exponent != 3 && exponent != 65537) {
+ LOG(ERROR) << "Public exponent should be 3 or 65537, actual: " << exponent;
+ return false;
+ }
+
+ return true;
+}
+
+bool CheckECKey(const std::unique_ptr<EC_KEY, ECKEYDeleter>& ec_key) {
+ if (!ec_key) {
+ return false;
+ }
+
+ const EC_GROUP* ec_group = EC_KEY_get0_group(ec_key.get());
+ if (!ec_group) {
+ LOG(ERROR) << "Failed to get the ec_group from the ec_key";
+ return false;
+ }
+ auto degree = EC_GROUP_get_degree(ec_group);
+ if (degree != 256) {
+ LOG(ERROR) << "Field size of the ec key should be 256 bits long, actual: " << degree;
+ return false;
+ }
+
+ return true;
+}
+
bool LoadCertificateFromBuffer(const std::vector<uint8_t>& pem_content, Certificate* cert) {
std::unique_ptr<BIO, decltype(&BIO_free)> content(
BIO_new_mem_buf(pem_content.data(), pem_content.size()), BIO_free);
@@ -538,22 +442,20 @@
}
int key_type = EVP_PKEY_id(public_key.get());
- // TODO(xunchang) check the rsa key has exponent 3 or 65537 with RSA_get0_key; and ec key is
- // 256 bits.
if (key_type == EVP_PKEY_RSA) {
cert->key_type = Certificate::KEY_TYPE_RSA;
cert->ec.reset();
cert->rsa.reset(EVP_PKEY_get1_RSA(public_key.get()));
- if (!cert->rsa) {
- LOG(ERROR) << "Failed to get the rsa key info from public key";
+ if (!cert->rsa || !CheckRSAKey(cert->rsa)) {
+ LOG(ERROR) << "Failed to validate the rsa key info from public key";
return false;
}
} else if (key_type == EVP_PKEY_EC) {
cert->key_type = Certificate::KEY_TYPE_EC;
cert->rsa.reset();
cert->ec.reset(EVP_PKEY_get1_EC_KEY(public_key.get()));
- if (!cert->ec) {
- LOG(ERROR) << "Failed to get the ec key info from the public key";
+ if (!cert->ec || !CheckECKey(cert->ec)) {
+ LOG(ERROR) << "Failed to validate the ec key info from the public key";
return false;
}
} else {
@@ -563,114 +465,3 @@
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<Certificate>& certs) {
- std::unique_ptr<FILE, decltype(&fclose)> 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 b7924c7..df9a4b6 100644
--- a/verifier.h
+++ b/verifier.h
@@ -70,7 +70,11 @@
int verify_file(const unsigned char* addr, size_t length, const std::vector<Certificate>& keys,
const std::function<void(float)>& set_progress = nullptr);
-bool load_keys(const char* filename, std::vector<Certificate>& 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, RSADeleter>& rsa);
+
+// Checks that the field size of the curve for the EC key is 256 bits.
+bool CheckECKey(const std::unique_ptr<EC_KEY, ECKEYDeleter>& ec_key);
// Parses a PEM-encoded x509 certificate from the given buffer and saves it into |cert|. Returns
// false if there is a parsing failure or the signature's encryption algorithm is not supported.