libavb: Only load and verify hash partition if requested.
Currently avb_slot_verify() will load _and_ verify all hash partitions
mentioned in vbmeta structus even if a partition isn't in the
|requested_partitions| parameter.
The current behavior is not useful because verification of a hash
partition only works if you keep the loaded data around. If you don't
keep it around, you set yourself up for a time-of-check-to-time-of-use
(TOCTTOU) attack insofar that an attacker can change the partition
contents in the window between it was checked and until you load it
again.
This should save ~400ms (the cost of sha256'ing ~32 MiB of data) in
fs_mgr / early-mount. This is because fs_mgr requests no partitions to
be loaded since it is only interested in the vbmeta structs.
Bug: None
Test: New unit test + all unit tests pass.
Test: Manually tested on UEFI-based boot loader.
Change-Id: I3e60d6c01e431c43ee2c629ed84318cbeac44347
diff --git a/libavb/avb_slot_verify.c b/libavb/avb_slot_verify.c
index 39cad4f..7ba4075 100644
--- a/libavb/avb_slot_verify.c
+++ b/libavb/avb_slot_verify.c
@@ -107,6 +107,17 @@
goto out;
}
+ /* Don't bother loading or validating unless the partition was
+ * requested in the first place.
+ */
+ found = avb_strv_find_str(requested_partitions,
+ (const char*)desc_partition_name,
+ hash_desc.partition_name_len);
+ if (found == NULL) {
+ ret = AVB_SLOT_VERIFY_RESULT_OK;
+ goto out;
+ }
+
if (!avb_str_concat(part_name,
sizeof part_name,
(const char*)desc_partition_name,
@@ -209,25 +220,21 @@
out:
- if (ret == AVB_SLOT_VERIFY_RESULT_OK || result_should_continue(ret)) {
- /* If this is the requested partition, copy to slot_data. */
- found = avb_strv_find_str(requested_partitions,
- (const char*)desc_partition_name,
- hash_desc.partition_name_len);
- if (found != NULL) {
- AvbPartitionData* loaded_partition;
- if (slot_data->num_loaded_partitions == MAX_NUMBER_OF_LOADED_PARTITIONS) {
- avb_errorv(part_name, ": Too many loaded partitions.\n", NULL);
- ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM;
- goto fail;
- }
- loaded_partition =
- &slot_data->loaded_partitions[slot_data->num_loaded_partitions++];
- loaded_partition->partition_name = avb_strdup(found);
- loaded_partition->data_size = image_size;
- loaded_partition->data = image_buf;
- image_buf = NULL;
+ /* If it worked and something was loaded, copy to slot_data. */
+ if ((ret == AVB_SLOT_VERIFY_RESULT_OK || result_should_continue(ret)) &&
+ image_buf != NULL) {
+ AvbPartitionData* loaded_partition;
+ if (slot_data->num_loaded_partitions == MAX_NUMBER_OF_LOADED_PARTITIONS) {
+ avb_errorv(part_name, ": Too many loaded partitions.\n", NULL);
+ ret = AVB_SLOT_VERIFY_RESULT_ERROR_OOM;
+ goto fail;
}
+ loaded_partition =
+ &slot_data->loaded_partitions[slot_data->num_loaded_partitions++];
+ loaded_partition->partition_name = avb_strdup(found);
+ loaded_partition->data_size = image_size;
+ loaded_partition->data = image_buf;
+ image_buf = NULL;
}
fail:
diff --git a/libavb/avb_slot_verify.h b/libavb/avb_slot_verify.h
index e7c010d..8794b98 100644
--- a/libavb/avb_slot_verify.h
+++ b/libavb/avb_slot_verify.h
@@ -253,21 +253,21 @@
void avb_slot_verify_data_free(AvbSlotVerifyData* data);
/* Performs a full verification of the slot identified by |ab_suffix|
- * and load the contents of the partitions whose name is in the
- * NULL-terminated string array |requested_partitions| (each partition
- * must use hash verification). If not using A/B, pass an empty string
- * (e.g. "", not NULL) for |ab_suffix|. This parameter must include
- * the leading underscore, for example "_a" should be used to refer to
- * the first slot.
+ * and load and verify the contents of the partitions whose name is in
+ * the NULL-terminated string array |requested_partitions| (each
+ * partition must use hash verification). If not using A/B, pass an
+ * empty string (e.g. "", not NULL) for |ab_suffix|. This parameter
+ * must include the leading underscore, for example "_a" should be
+ * used to refer to the first slot.
*
* Typically the |requested_partitions| array only contains a single
* item for the boot partition, 'boot'.
*
- * Verification includes loading data from the 'vbmeta', all hash
- * partitions, and possibly other partitions (with |ab_suffix|
- * appended), inspecting rollback indexes, and checking if the public
- * key used to sign the data is acceptable. The functions in |ops|
- * will be used to do this.
+ * Verification includes loading and verifying data from the 'vbmeta',
+ * the requested hash partitions, and possibly other partitions (with
+ * |ab_suffix| appended), inspecting rollback indexes, and checking if
+ * the public key used to sign the data is acceptable. The functions
+ * in |ops| will be used to do this.
*
* If |out_data| is not NULL, it will be set to a newly allocated
* |AvbSlotVerifyData| struct containing all the data needed to
@@ -281,10 +281,11 @@
* ignore verification errors which is something needed in the
* UNLOCKED state. See the AvbSlotVerifyFlags enumeration for details.
*
- * The |hashtree_error_mode| parameter should be set the desired error
- * handling mode when hashtree validation fails inside the HLOS. This
- * value isn't used by libavb per se - it is forwarded to the HLOS
- * through the androidboot.veritymode cmdline parameter. See the
+ * The |hashtree_error_mode| parameter should be set to the desired
+ * error handling mode when hashtree validation fails inside the
+ * HLOS. This value isn't used by libavb per se - it is forwarded to
+ * the HLOS through the androidboot.veritymode and
+ * androidboot.vbmeta.invalidate_on_error cmdline parameters. See the
* AvbHashtreeErrorMode enumeration for details.
*
* Also note that |out_data| is never set if
diff --git a/test/avb_slot_verify_unittest.cc b/test/avb_slot_verify_unittest.cc
index a940cfc..8348bf2 100644
--- a/test/avb_slot_verify_unittest.cc
+++ b/test/avb_slot_verify_unittest.cc
@@ -1233,8 +1233,102 @@
for (size_t n = 0; n < slot_data->loaded_partitions[1].data_size; n++) {
EXPECT_EQ(slot_data->loaded_partitions[1].data[n], uint8_t(n));
}
-
avb_slot_verify_data_free(slot_data);
+
+ // Check that we loaded vbmeta_a, foo_a, and bar_a.
+ std::set<std::string> partitions = ops_.get_partition_names_read_from();
+ EXPECT_EQ(size_t(3), partitions.size());
+ EXPECT_TRUE(partitions.find("vbmeta_a") != partitions.end());
+ EXPECT_TRUE(partitions.find("foo_a") != partitions.end());
+ EXPECT_TRUE(partitions.find("bar_a") != partitions.end());
+}
+
+TEST_F(AvbSlotVerifyTest, OnlyLoadWhatHasBeenRequested) {
+ const size_t foo_partition_size = 16 * 1024 * 1024;
+ const size_t bar_partition_size = 32 * 1024 * 1024;
+ const size_t foo_image_size = 5 * 1024 * 1024;
+ const size_t bar_image_size = 10 * 1024 * 1024;
+ base::FilePath foo_path = GenerateImage("foo_a.img", foo_image_size);
+ base::FilePath bar_path = GenerateImage("bar_a.img", bar_image_size);
+
+ EXPECT_COMMAND(0,
+ "./avbtool add_hash_footer"
+ " --image %s"
+ " --partition_name foo"
+ " --partition_size %zd"
+ " --salt deadbeef"
+ " --internal_release_string \"\"",
+ foo_path.value().c_str(),
+ foo_partition_size);
+
+ EXPECT_COMMAND(0,
+ "./avbtool add_hash_footer"
+ " --image %s"
+ " --partition_name bar"
+ " --partition_size %zd"
+ " --salt deadbeef"
+ " --internal_release_string \"\"",
+ bar_path.value().c_str(),
+ bar_partition_size);
+
+ GenerateVBMetaImage("vbmeta_a.img",
+ "SHA256_RSA2048",
+ 4,
+ base::FilePath("test/data/testkey_rsa2048.pem"),
+ base::StringPrintf("--include_descriptors_from_image %s"
+ " --include_descriptors_from_image %s"
+ " --internal_release_string \"\"",
+ foo_path.value().c_str(),
+ bar_path.value().c_str()));
+
+ EXPECT_EQ(
+ "Minimum libavb version: 1.0\n"
+ "Header Block: 256 bytes\n"
+ "Authentication Block: 320 bytes\n"
+ "Auxiliary Block: 896 bytes\n"
+ "Algorithm: SHA256_RSA2048\n"
+ "Rollback Index: 4\n"
+ "Flags: 0\n"
+ "Release String: ''\n"
+ "Descriptors:\n"
+ " Hash descriptor:\n"
+ " Image Size: 5242880 bytes\n"
+ " Hash Algorithm: sha256\n"
+ " Partition Name: foo\n"
+ " Salt: deadbeef\n"
+ " Digest: "
+ "184cb36243adb8b87d2d8c4802de32125fe294ec46753d732144ee65df68a23d\n"
+ " Hash descriptor:\n"
+ " Image Size: 10485760 bytes\n"
+ " Hash Algorithm: sha256\n"
+ " Partition Name: bar\n"
+ " Salt: deadbeef\n"
+ " Digest: "
+ "baea4bbd261d0edf4d1fe5e6e5a36976c291eeba66b6a46fa81dba691327a727\n",
+ InfoImage(vbmeta_image_path_));
+
+ ops_.set_expected_public_key(
+ PublicKeyAVB(base::FilePath("test/data/testkey_rsa2048.pem")));
+ AvbSlotVerifyData* slot_data = NULL;
+ const char* requested_partitions[] = {"foo", NULL};
+ EXPECT_EQ(AVB_SLOT_VERIFY_RESULT_OK,
+ avb_slot_verify(ops_.avb_ops(),
+ requested_partitions,
+ "_a",
+ AVB_SLOT_VERIFY_FLAGS_NONE,
+ AVB_HASHTREE_ERROR_MODE_RESTART_AND_INVALIDATE,
+ &slot_data));
+ EXPECT_NE(nullptr, slot_data);
+ EXPECT_EQ(size_t(1), slot_data->num_loaded_partitions);
+ EXPECT_EQ("foo", std::string(slot_data->loaded_partitions[0].partition_name));
+ avb_slot_verify_data_free(slot_data);
+
+ // Check that we loaded vbmeta_a, foo_a but not bar_a.
+ std::set<std::string> partitions = ops_.get_partition_names_read_from();
+ EXPECT_EQ(size_t(2), partitions.size());
+ EXPECT_TRUE(partitions.find("vbmeta_a") != partitions.end());
+ EXPECT_TRUE(partitions.find("foo_a") != partitions.end());
+ EXPECT_TRUE(partitions.find("bar_a") == partitions.end());
}
TEST_F(AvbSlotVerifyTest, PublicKeyMetadata) {
diff --git a/test/fake_avb_ops.cc b/test/fake_avb_ops.cc
index c5563c8..4b38dcc 100644
--- a/test/fake_avb_ops.cc
+++ b/test/fake_avb_ops.cc
@@ -43,6 +43,10 @@
namespace avb {
+std::set<std::string> FakeAvbOps::get_partition_names_read_from() {
+ return partition_names_read_from_;
+}
+
AvbIOResult FakeAvbOps::read_from_partition(const char* partition,
int64_t offset,
size_t num_bytes,
@@ -51,6 +55,8 @@
base::FilePath path =
partition_dir_.Append(std::string(partition)).AddExtension("img");
+ partition_names_read_from_.insert(partition);
+
if (offset < 0) {
int64_t file_size;
if (!base::GetFileSize(path, &file_size)) {
diff --git a/test/fake_avb_ops.h b/test/fake_avb_ops.h
index 612c62c..53775d4 100644
--- a/test/fake_avb_ops.h
+++ b/test/fake_avb_ops.h
@@ -27,6 +27,7 @@
#include <base/files/file_util.h>
#include <map>
+#include <set>
#include <string>
#include <libavb_ab/libavb_ab.h>
@@ -154,6 +155,10 @@
permanent_attributes_hash_ = hash;
}
+ // Gets the partition names that were passed to the
+ // read_from_partition() operation.
+ std::set<std::string> get_partition_names_read_from();
+
// FakeAvbOpsDelegate methods.
AvbIOResult read_from_partition(const char* partition,
int64_t offset,
@@ -217,6 +222,8 @@
AvbAtxPermanentAttributes permanent_attributes_;
std::string permanent_attributes_hash_;
+
+ std::set<std::string> partition_names_read_from_;
};
} // namespace avb
