From 31f9ee58d0c91d02d96d29ea6e09bf0e4f9c5b92 Mon Sep 17 00:00:00 2001 From: Kane York Date: Mon, 27 Jul 2020 16:50:56 -0700 Subject: fix(3p/nix/hash): provide a Status-returning constructor Additionally, add IsValidBase16() to restore the behavior of rejecting invalid base16, which absl's HexStringToBytes does not do. Change-Id: I777a36f5dc787aa54a2aa316d6728f68da129768 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1484 Tested-by: BuildkiteCI Reviewed-by: tazjin --- third_party/nix/src/libutil/CMakeLists.txt | 1 + third_party/nix/src/libutil/hash.cc | 120 +++++++++++++++++------------ third_party/nix/src/libutil/hash.hh | 10 +++ third_party/nix/src/libutil/types.hh | 2 + third_party/nix/src/tests/hash_test.cc | 42 +++++++++- 5 files changed, 123 insertions(+), 52 deletions(-) diff --git a/third_party/nix/src/libutil/CMakeLists.txt b/third_party/nix/src/libutil/CMakeLists.txt index 8713d7e9b8..db504940a0 100644 --- a/third_party/nix/src/libutil/CMakeLists.txt +++ b/third_party/nix/src/libutil/CMakeLists.txt @@ -48,6 +48,7 @@ target_sources(nixutil target_link_libraries(nixutil absl::strings + absl::statusor glog BZip2::BZip2 LibLZMA::LibLZMA diff --git a/third_party/nix/src/libutil/hash.cc b/third_party/nix/src/libutil/hash.cc index 5596ef0178..50169b0f19 100644 --- a/third_party/nix/src/libutil/hash.cc +++ b/third_party/nix/src/libutil/hash.cc @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -75,8 +76,18 @@ static std::string printHash16(const Hash& hash) { return std::string(buf, hash.hashSize * 2); } +bool Hash::IsValidBase16(absl::string_view s) { + for (char c : s) { + if ('0' <= c && c <= '9') continue; + if ('a' <= c && c <= 'f') continue; + if ('A' <= c && c <= 'F') continue; + return false; + } + return true; +} + // omitted: E O U T -const std::string base32Chars = "0123456789abcdfghijklmnpqrsvwxyz"; +constexpr char base32Chars[] = "0123456789abcdfghijklmnpqrsvwxyz"; constexpr signed char kUnBase32[] = { -1, -1, -1, -1, -1, -1, -1, -1, /* unprintables */ @@ -167,6 +178,15 @@ std::string Hash::to_string(Base base, bool includeType) const { } Hash::Hash(const std::string& s, HashType type) : type(type) { + absl::StatusOr result = deserialize(s, type); + if (result.ok()) { + *this = *result; + } else { + throw BadHash(result.status().message()); + } +} + +absl::StatusOr Hash::deserialize(const std::string& s, HashType type) { size_t pos = 0; bool isSRI = false; @@ -176,90 +196,88 @@ Hash::Hash(const std::string& s, HashType type) : type(type) { if (sep != std::string::npos) { isSRI = true; } else if (type == htUnknown) { - throw BadHash("hash '%s' does not include a type", s); + return absl::InvalidArgumentError( + absl::StrCat("hash string '", s, " does not include a type")); } } + HashType parsedType = type; if (sep != std::string::npos) { std::string hts = std::string(s, 0, sep); - this->type = parseHashType(hts); - if (this->type == htUnknown) { - throw BadHash("unknown hash type '%s'", hts); - } - if (type != htUnknown && type != this->type) { - throw BadHash("hash '%s' should have type '%s'", s, printHashType(type)); + parsedType = parseHashType(hts); + if (parsedType != type) { + return absl::InvalidArgumentError( + absl::StrCat("hash '", s, "' should have type '", printHashType(type), + "', found '", printHashType(parsedType), "'")); } pos = sep + 1; } - init(); + Hash dest(parsedType); size_t size = s.size() - pos; + absl::string_view sv(s.data() + pos, size); - if (!isSRI && size == base16Len()) { - auto parseHexDigit = [&](char c) { - if (c >= '0' && c <= '9') { - return c - '0'; - } - if (c >= 'A' && c <= 'F') { - return c - 'A' + 10; - } - if (c >= 'a' && c <= 'f') { - return c - 'a' + 10; - } - throw BadHash("invalid base-16 hash '%s'", s); - }; - - for (unsigned int i = 0; i < hashSize; i++) { - hash[i] = parseHexDigit(s[pos + i * 2]) << 4 | - parseHexDigit(s[pos + i * 2 + 1]); + if (!isSRI && size == dest.base16Len()) { + std::string bytes; + if (!IsValidBase16(sv)) { + return absl::InvalidArgumentError( + absl::StrCat("invalid base-16 hash: bad character in '", s, "'")); } + bytes = absl::HexStringToBytes(sv); + if (bytes.size() != dest.hashSize) { + return absl::InvalidArgumentError( + absl::StrCat("hash '", s, "' has wrong length for base16 ", + printHashType(dest.type))); + } + memcpy(dest.hash, bytes.data(), dest.hashSize); } - else if (!isSRI && size == base32Len()) { + else if (!isSRI && size == dest.base32Len()) { for (unsigned int n = 0; n < size; ++n) { - char c = s[pos + size - n - 1]; - unsigned char digit = 0; - for (digit = 0; digit < base32Chars.size(); ++digit) { /* !!! slow */ - if (base32Chars[digit] == c) { - break; - } - } - if (digit >= 32) { - throw BadHash("invalid base-32 hash '%s'", s); + char c = sv[size - n - 1]; + // range: -1, 0..31 + signed char digit = kUnBase32[static_cast(c)]; + if (digit < 0) { + return absl::InvalidArgumentError( + absl::StrCat("invalid base-32 hash: bad character ", + absl::CEscape(absl::string_view(&c, 1)))); } unsigned int b = n * 5; unsigned int i = b / 8; unsigned int j = b % 8; - hash[i] |= digit << j; + dest.hash[i] |= digit << j; - if (i < hashSize - 1) { - hash[i + 1] |= digit >> (8 - j); + if (i < dest.hashSize - 1) { + dest.hash[i + 1] |= digit >> (8 - j); } else { if ((digit >> (8 - j)) != 0) { - throw BadHash("invalid base-32 hash '%s'", s); + return absl::InvalidArgumentError( + absl::StrCat("invalid base-32 hash '", s, "'")); } } } } - else if (isSRI || size == base64Len()) { - std::string d; - if (!absl::Base64Unescape(std::string(s, pos), &d)) { - // TODO(grfn): replace this with StatusOr - throw Error("Invalid Base64"); + else if (isSRI || size == dest.base64Len()) { + std::string decoded; + if (!absl::Base64Unescape(sv, &decoded)) { + return absl::InvalidArgumentError("invalid base-64 hash"); } - if (d.size() != hashSize) { - throw BadHash("invalid %s hash '%s'", isSRI ? "SRI" : "base-64", s); + if (decoded.size() != dest.hashSize) { + return absl::InvalidArgumentError( + absl::StrCat("hash '", s, "' has wrong length for base64 ", + printHashType(dest.type))); } - assert(hashSize); - memcpy(hash, d.data(), hashSize); + memcpy(dest.hash, decoded.data(), dest.hashSize); } else { - throw BadHash("hash '%s' has wrong length for hash type '%s'", s, - printHashType(type)); + return absl::InvalidArgumentError(absl::StrCat( + "hash '", s, "' has wrong length for ", printHashType(dest.type))); } + + return dest; } union Ctx { diff --git a/third_party/nix/src/libutil/hash.hh b/third_party/nix/src/libutil/hash.hh index 58f808896f..0b7b11edd0 100644 --- a/third_party/nix/src/libutil/hash.hh +++ b/third_party/nix/src/libutil/hash.hh @@ -1,5 +1,7 @@ #pragma once +#include + #include "libutil/serialise.hh" #include "libutil/types.hh" @@ -36,6 +38,10 @@ struct Hash { string. */ Hash(const std::string& s, HashType type = htUnknown); + /* Status-returning version of above constructor */ + static absl::StatusOr deserialize(const std::string& s, + HashType type = htUnknown); + void init(); /* Check whether a hash is set. */ @@ -64,6 +70,10 @@ struct Hash { (e.g. "sha256:"). */ std::string to_string(Base base = Base32, bool includeType = true) const; + /* Returns whether the passed string contains entirely valid base16 + characters. */ + static bool IsValidBase16(absl::string_view s); + /* Returns whether the passed string contains entirely valid base32 characters. */ static bool IsValidBase32(absl::string_view s); diff --git a/third_party/nix/src/libutil/types.hh b/third_party/nix/src/libutil/types.hh index e2ea86fdcf..3d37e4efee 100644 --- a/third_party/nix/src/libutil/types.hh +++ b/third_party/nix/src/libutil/types.hh @@ -44,6 +44,8 @@ struct FormatOrString { inline std::string fmt(const std::string& s) { return s; } +inline std::string fmt(std::string_view s) { return std::string(s); } + inline std::string fmt(const char* s) { return s; } inline std::string fmt(const FormatOrString& fs) { return fs.s; } diff --git a/third_party/nix/src/tests/hash_test.cc b/third_party/nix/src/tests/hash_test.cc index 2ed4dca3bd..ea10e7b700 100644 --- a/third_party/nix/src/tests/hash_test.cc +++ b/third_party/nix/src/tests/hash_test.cc @@ -1,12 +1,16 @@ #include "libutil/hash.hh" +#include #include class HashTest : public ::testing::Test {}; +using testing::EndsWith; +using testing::HasSubstr; + namespace nix { -TEST(HASH_TEST, SHA256) { +TEST(HashTest, SHA256) { auto hash = hashString(HashType::htSHA256, "foo"); ASSERT_EQ(hash.base64Len(), 44); ASSERT_EQ(hash.base32Len(), 52); @@ -40,4 +44,40 @@ TEST(HashTest, SHA256Decode) { ASSERT_EQ(hash, *base64); } +TEST(HashTest, SHA256DecodeFail) { + EXPECT_THAT( + Hash::deserialize("sha256:LCa0a2j/xo/5m0U8HTBBNBNCLXBkg7+g+YpeiGJm56==", + HashType::htSHA256) + .status() + .message(), + HasSubstr("wrong length")); + EXPECT_THAT( + Hash::deserialize("sha256:LCa0a2j/xo/5m0U8HTBBNBNCLXBkg7+g+YpeiGJm56,=", + HashType::htSHA256) + .status() + .message(), + HasSubstr("invalid base-64")); + + EXPECT_THAT(Hash::deserialize( + "sha256:1bp7cri8hplaz6hbz0v4f0nl44rl84q1sg25kgwqzipzd1mv89i", + HashType::htSHA256) + .status() + .message(), + HasSubstr("wrong length")); + absl::StatusOr badB32Char = Hash::deserialize( + "sha256:1bp7cri8hplaz6hbz0v4f0nl44rl84q1sg25kgwqzipzd1mv89i,", + HashType::htSHA256); + EXPECT_THAT(badB32Char.status().message(), HasSubstr("invalid base-32")); + EXPECT_THAT(badB32Char.status().message(), EndsWith(",")); + + EXPECT_THAT( + Hash::deserialize( + "sha256:" + "2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7 ", + HashType::htSHA256) + .status() + .message(), + HasSubstr("invalid base-16")); +} + } // namespace nix -- cgit 1.4.1