u2fd: make util::VectorToObject safer
This would prevent potential stack overflow issue.
BUG=b:173403271
TEST=check power button key work on DUT
TEST=CQ
Change-Id: I6ed587f8917dc94433f0e52aae8b9752a973660d
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2546672
Tested-by: John L Chen <zuan@chromium.org>
Tested-by: Leo Lai <cylai@google.com>
Reviewed-by: joe Chou <yich@google.com>
Reviewed-by: Leo Lai <cylai@google.com>
Commit-Queue: Leo Lai <cylai@google.com>
diff --git a/u2fd/u2f_msg_handler.cc b/u2fd/u2f_msg_handler.cc
index 7f460c7..6c350fc 100644
--- a/u2fd/u2f_msg_handler.cc
+++ b/u2fd/u2f_msg_handler.cc
@@ -296,8 +296,14 @@
struct u2f_generate_req generate_req = {
.flags = U2F_AUTH_ENFORCE // Require user presence, consume.
};
- util::VectorToObject(app_id, generate_req.appId);
- util::VectorToObject(*user_secret, generate_req.userSecret);
+ if (!util::VectorToObject(app_id, generate_req.appId,
+ sizeof(generate_req.appId))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(*user_secret, generate_req.userSecret,
+ sizeof(generate_req.userSecret))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
struct u2f_generate_resp generate_resp = {};
Cr50CmdStatus generate_status = static_cast<Cr50CmdStatus>(
@@ -332,10 +338,20 @@
};
if (allow_legacy_kh_sign_)
sign_req.flags |= SIGN_LEGACY_KH;
- util::VectorToObject(app_id, sign_req.appId);
- util::VectorToObject(*user_secret, sign_req.userSecret);
- util::VectorToObject(key_handle, &sign_req.keyHandle);
- util::VectorToObject(hash, sign_req.hash);
+ if (!util::VectorToObject(app_id, sign_req.appId, sizeof(sign_req.appId))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(*user_secret, sign_req.userSecret,
+ sizeof(sign_req.userSecret))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(key_handle, &sign_req.keyHandle,
+ sizeof(sign_req.keyHandle))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(hash, sign_req.hash, sizeof(sign_req.hash))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
struct u2f_sign_resp sign_resp = {};
Cr50CmdStatus sign_status =
@@ -371,9 +387,17 @@
struct u2f_sign_req sign_req = {
.flags = U2F_AUTH_CHECK_ONLY // No user presence required, no consume.
};
- util::VectorToObject(app_id, sign_req.appId);
- util::VectorToObject(*user_secret, sign_req.userSecret);
- util::VectorToObject(key_handle, &sign_req.keyHandle);
+ if (!util::VectorToObject(app_id, sign_req.appId, sizeof(sign_req.appId))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(*user_secret, sign_req.userSecret,
+ sizeof(sign_req.userSecret))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(key_handle, &sign_req.keyHandle,
+ sizeof(sign_req.keyHandle))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
Cr50CmdStatus sign_status =
static_cast<Cr50CmdStatus>(proxy_->SendU2fSign(sign_req, nullptr));
@@ -395,10 +419,13 @@
struct u2f_attest_req attest_req = {
.format = format, .dataLen = static_cast<uint8_t>(data.size())};
- util::VectorToObject(*user_secret, attest_req.userSecret);
- // Only a programming error can cause this CHECK to fail.
- CHECK_LE(data.size(), sizeof(attest_req.data));
- util::VectorToObject(data, attest_req.data);
+ if (!util::VectorToObject(*user_secret, attest_req.userSecret,
+ sizeof(attest_req.userSecret))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
+ if (!util::VectorToObject(data, attest_req.data, sizeof(attest_req.data))) {
+ return Cr50CmdStatus::kInvalidState;
+ }
struct u2f_attest_resp attest_resp = {};
Cr50CmdStatus attest_status = static_cast<Cr50CmdStatus>(
diff --git a/u2fd/util.h b/u2fd/util.h
index 2c05bdc..66aa064 100644
--- a/u2fd/util.h
+++ b/u2fd/util.h
@@ -41,10 +41,14 @@
// Utility function to copy bytes from a vector to an object. This is the
// inverse of AppendToVector.
template <typename VectorAllocator, typename ToType>
-void VectorToObject(const std::vector<uint8_t, VectorAllocator>& from,
- ToType* to) {
- // TODO(louiscollard): This, but nicer/safer.
+bool VectorToObject(const std::vector<uint8_t, VectorAllocator>& from,
+ ToType* to,
+ const size_t size) {
+ if (size < from.size()) {
+ return false;
+ }
memcpy(to, &from.front(), from.size());
+ return true;
}
// Utility function to copy part of a string to a vector.
diff --git a/u2fd/webauthn_handler.cc b/u2fd/webauthn_handler.cc
index 24b955e..f2fcdee 100644
--- a/u2fd/webauthn_handler.cc
+++ b/u2fd/webauthn_handler.cc
@@ -362,8 +362,14 @@
struct u2f_generate_req generate_req = {
.flags = U2F_AUTH_ENFORCE // Require user presence, consume.
};
- util::VectorToObject(rp_id_hash, generate_req.appId);
- util::VectorToObject(*user_secret, generate_req.userSecret);
+ if (!util::VectorToObject(rp_id_hash, generate_req.appId,
+ sizeof(generate_req.appId))) {
+ return MakeCredentialResponse::INVALID_REQUEST;
+ }
+ if (!util::VectorToObject(*user_secret, generate_req.userSecret,
+ sizeof(generate_req.userSecret))) {
+ return MakeCredentialResponse::INVALID_REQUEST;
+ }
struct u2f_generate_resp generate_resp = {};
@@ -526,10 +532,22 @@
struct u2f_sign_req sign_req = {
.flags = U2F_AUTH_ENFORCE // Require user presence, consume.
};
- util::VectorToObject(rp_id_hash, sign_req.appId);
- util::VectorToObject(*user_secret, sign_req.userSecret);
- util::VectorToObject(credential_id, &sign_req.keyHandle);
- util::VectorToObject(hash_to_sign, sign_req.hash);
+ if (!util::VectorToObject(rp_id_hash, sign_req.appId,
+ sizeof(sign_req.appId))) {
+ return GetAssertionResponse::INVALID_REQUEST;
+ }
+ if (!util::VectorToObject(*user_secret, sign_req.userSecret,
+ sizeof(sign_req.userSecret))) {
+ return GetAssertionResponse::INVALID_REQUEST;
+ }
+ if (!util::VectorToObject(credential_id, &sign_req.keyHandle,
+ sizeof(sign_req.keyHandle))) {
+ return GetAssertionResponse::INVALID_REQUEST;
+ }
+ if (!util::VectorToObject(hash_to_sign, sign_req.hash,
+ sizeof(sign_req.hash))) {
+ return GetAssertionResponse::INVALID_REQUEST;
+ }
struct u2f_sign_resp sign_resp = {};
@@ -595,9 +613,18 @@
}
struct u2f_sign_req sign_req = {.flags = U2F_AUTH_CHECK_ONLY};
- util::VectorToObject(rp_id_hash, sign_req.appId);
- util::VectorToObject(*user_secret, sign_req.userSecret);
- util::VectorToObject(credential_id, &sign_req.keyHandle);
+ if (!util::VectorToObject(rp_id_hash, sign_req.appId,
+ sizeof(sign_req.appId))) {
+ return HasCredentialsResponse::INVALID_REQUEST;
+ }
+ if (!util::VectorToObject(*user_secret, sign_req.userSecret,
+ sizeof(sign_req.userSecret))) {
+ return HasCredentialsResponse::INVALID_REQUEST;
+ }
+ if (!util::VectorToObject(credential_id, &sign_req.keyHandle,
+ sizeof(sign_req.keyHandle))) {
+ return HasCredentialsResponse::INVALID_REQUEST;
+ }
struct u2f_sign_resp sign_resp;
base::AutoLock(tpm_proxy_->GetLock());
