From 3d98f5d1599794010151d3abd92295d88684658e Mon Sep 17 00:00:00 2001 From: Paul Crowley Date: Fri, 7 Feb 2020 11:49:09 -0800 Subject: [PATCH] Pass volume key as a KeyBuffer Not for security, but for consistency with the way we handle other keys, and to move the length check to where it belongs. Test: create private volume on Cuttlefish Bug: 147814592 Change-Id: I10fc4896183d050ce25ff174faf78f525cf62930 --- cryptfs.cpp | 11 +++++++++-- cryptfs.h | 6 ++++-- model/Disk.cpp | 3 ++- model/PrivateVolume.cpp | 10 ++-------- model/PrivateVolume.h | 4 ++-- 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/cryptfs.cpp b/cryptfs.cpp index 337bdc2..e00165f 100644 --- a/cryptfs.cpp +++ b/cryptfs.cpp @@ -71,6 +71,7 @@ extern "C" { using android::base::ParseUint; using android::base::StringPrintf; using android::fs_mgr::GetEntryForMountPoint; +using android::vold::KeyBuffer; using namespace android::dm; using namespace std::chrono_literals; @@ -1910,8 +1911,13 @@ errout: * as any metadata is been stored in a separate, small partition. We * assume it must be using our same crypt type and keysize. */ -int cryptfs_setup_ext_volume(const char* label, const char* real_blkdev, const unsigned char* key, +int cryptfs_setup_ext_volume(const char* label, const char* real_blkdev, const KeyBuffer& key, std::string* out_crypto_blkdev) { + if (key.size() != cryptfs_get_keysize()) { + SLOGE("Raw keysize %zu does not match crypt keysize %" PRIu32, key.size(), + cryptfs_get_keysize()); + return -1; + } uint64_t nr_sec = 0; if (android::vold::GetBlockDev512Sectors(real_blkdev, &nr_sec) != android::OK) { SLOGE("Failed to get size of %s: %s", real_blkdev, strerror(errno)); @@ -1929,7 +1935,8 @@ int cryptfs_setup_ext_volume(const char* label, const char* real_blkdev, const u android::base::GetBoolProperty("ro.crypto.allow_encrypt_override", false)) flags |= CREATE_CRYPTO_BLK_DEV_FLAGS_ALLOW_ENCRYPT_OVERRIDE; - return create_crypto_blk_dev(&ext_crypt_ftr, key, real_blkdev, out_crypto_blkdev, label, flags); + return create_crypto_blk_dev(&ext_crypt_ftr, reinterpret_cast(key.data()), + real_blkdev, out_crypto_blkdev, label, flags); } /* diff --git a/cryptfs.h b/cryptfs.h index 98ba7d6..463db7f 100644 --- a/cryptfs.h +++ b/cryptfs.h @@ -25,6 +25,8 @@ #include +#include "KeyBuffer.h" + #define CRYPT_FOOTER_OFFSET 0x4000 /* Return values for cryptfs_crypto_complete */ @@ -62,8 +64,8 @@ int cryptfs_restart(void); int cryptfs_enable(int type, const char* passwd, int no_ui); int cryptfs_changepw(int type, const char* newpw); int cryptfs_enable_default(int no_ui); -int cryptfs_setup_ext_volume(const char* label, const char* real_blkdev, const unsigned char* key, - std::string* out_crypto_blkdev); +int cryptfs_setup_ext_volume(const char* label, const char* real_blkdev, + const android::vold::KeyBuffer& key, std::string* out_crypto_blkdev); int cryptfs_revert_ext_volume(const char* label); int cryptfs_getfield(const char* fieldname, char* value, int len); int cryptfs_setfield(const char* fieldname, const char* value); diff --git a/model/Disk.cpp b/model/Disk.cpp index b66c336..bfaf2cd 100644 --- a/model/Disk.cpp +++ b/model/Disk.cpp @@ -216,7 +216,8 @@ void Disk::createPrivateVolume(dev_t device, const std::string& partGuid) { LOG(DEBUG) << "Found key for GUID " << normalizedGuid; - auto vol = std::shared_ptr(new PrivateVolume(device, keyRaw)); + auto keyBuffer = KeyBuffer(keyRaw.begin(), keyRaw.end()); + auto vol = std::shared_ptr(new PrivateVolume(device, keyBuffer)); if (mJustPartitioned) { LOG(DEBUG) << "Device just partitioned; silently formatting"; vol->setSilent(true); diff --git a/model/PrivateVolume.cpp b/model/PrivateVolume.cpp index 7fd46a2..1653fae 100644 --- a/model/PrivateVolume.cpp +++ b/model/PrivateVolume.cpp @@ -43,7 +43,7 @@ namespace vold { static const unsigned int kMajorBlockMmc = 179; -PrivateVolume::PrivateVolume(dev_t device, const std::string& keyRaw) +PrivateVolume::PrivateVolume(dev_t device, const KeyBuffer& keyRaw) : VolumeBase(Type::kPrivate), mRawDevice(device), mKeyRaw(keyRaw) { setId(StringPrintf("private:%u,%u", major(device), minor(device))); mRawDevPath = StringPrintf("/dev/block/vold/%s", getId().c_str()); @@ -64,19 +64,13 @@ status_t PrivateVolume::doCreate() { if (CreateDeviceNode(mRawDevPath, mRawDevice)) { return -EIO; } - if (mKeyRaw.size() != cryptfs_get_keysize()) { - PLOG(ERROR) << getId() << " Raw keysize " << mKeyRaw.size() - << " does not match crypt keysize " << cryptfs_get_keysize(); - return -EIO; - } // Recover from stale vold by tearing down any old mappings cryptfs_revert_ext_volume(getId().c_str()); // TODO: figure out better SELinux labels for private volumes - unsigned char* key = (unsigned char*)mKeyRaw.data(); - int res = cryptfs_setup_ext_volume(getId().c_str(), mRawDevPath.c_str(), key, &mDmDevPath); + int res = cryptfs_setup_ext_volume(getId().c_str(), mRawDevPath.c_str(), mKeyRaw, &mDmDevPath); if (res != 0) { PLOG(ERROR) << getId() << " failed to setup cryptfs"; return -EIO; diff --git a/model/PrivateVolume.h b/model/PrivateVolume.h index cb8e75d..819632b 100644 --- a/model/PrivateVolume.h +++ b/model/PrivateVolume.h @@ -37,7 +37,7 @@ namespace vold { */ class PrivateVolume : public VolumeBase { public: - PrivateVolume(dev_t device, const std::string& keyRaw); + PrivateVolume(dev_t device, const KeyBuffer& keyRaw); virtual ~PrivateVolume(); const std::string& getFsType() const { return mFsType; }; const std::string& getRawDevPath() const { return mRawDevPath; }; @@ -63,7 +63,7 @@ class PrivateVolume : public VolumeBase { std::string mPath; /* Encryption key as raw bytes */ - std::string mKeyRaw; + KeyBuffer mKeyRaw; /* Filesystem type */ std::string mFsType;