Handle "acceptable" Wycheproof inputs unambiguously.
This CL updates the JSON conversion to preserve the flags. A
WycheproofResult now captures both "result" and "flags". An "acceptable"
test case's validity is determined by its flags. By default, we consider
an "acceptable" case as invalid, but a test driver may mark some of them
as valid by listing the flags as a parameter.
Previously, some Wycheproof tests (I think it was x25519_tests.txt?) did
not contain enough information to resolve this unambiguously. This has
since been fixed.
This also makes the converted files smaller because we no longer expand the
flags into comments.
Change-Id: I2ca02d7f1b95f250409e8b23c4ad7bb595d77fdf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39188
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/cipher_extra/aead_test.cc b/crypto/cipher_extra/aead_test.cc
index 520ad72..bb3db9e 100644
--- a/crypto/cipher_extra/aead_test.cc
+++ b/crypto/cipher_extra/aead_test.cc
@@ -728,8 +728,8 @@
size_t out_len;
// Wycheproof tags small AES-GCM IVs as "acceptable" and otherwise does not
// use it in AEADs. Any AES-GCM IV that isn't 96 bits is absurd, but our API
- // supports those, so we treat "acceptable" as "valid" here.
- if (result != WycheproofResult::kInvalid) {
+ // supports those, so we treat SmallIv tests as valid.
+ if (result.IsValid({"SmallIv"})) {
// Decryption should succeed.
ASSERT_TRUE(EVP_AEAD_CTX_open(ctx.get(), out.data(), &out_len, out.size(),
iv.data(), iv.size(), ct_and_tag.data(),
diff --git a/crypto/cipher_extra/cipher_test.cc b/crypto/cipher_extra/cipher_test.cc
index 0a96bf3..a28e6e3 100644
--- a/crypto/cipher_extra/cipher_test.cc
+++ b/crypto/cipher_extra/cipher_test.cc
@@ -357,7 +357,7 @@
17, 31, 32, 33, 63, 64, 65, 512};
for (size_t chunk : chunk_sizes) {
SCOPED_TRACE(chunk);
- if (result == WycheproofResult::kValid) {
+ if (result.IsValid()) {
ASSERT_TRUE(EVP_DecryptInit_ex(ctx.get(), cipher, nullptr, key.data(),
iv.data()));
ASSERT_TRUE(DoCipher(ctx.get(), &out, ct, chunk));
diff --git a/crypto/cmac/cmac_test.cc b/crypto/cmac/cmac_test.cc
index bd6651d..e8a220a 100644
--- a/crypto/cmac/cmac_test.cc
+++ b/crypto/cmac/cmac_test.cc
@@ -148,7 +148,7 @@
// Some test vectors intentionally give the wrong key size. Our API
// requires the caller pick the sized CBC primitive, so these tests
// aren't useful for us.
- EXPECT_EQ(WycheproofResult::kInvalid, result);
+ EXPECT_FALSE(result.IsValid());
return;
}
@@ -164,7 +164,7 @@
// Truncate the tag, if requested.
out_len = std::min(out_len, tag_len);
- if (result == WycheproofResult::kValid) {
+ if (result.IsValid()) {
EXPECT_EQ(Bytes(tag), Bytes(out, out_len));
// Test the streaming API as well.
diff --git a/crypto/curve25519/x25519_test.cc b/crypto/curve25519/x25519_test.cc
index 9578dd0..1bf398f 100644
--- a/crypto/curve25519/x25519_test.cc
+++ b/crypto/curve25519/x25519_test.cc
@@ -23,6 +23,7 @@
#include "../internal.h"
#include "../test/file_test.h"
#include "../test/test_util.h"
+#include "../test/wycheproof_util.h"
TEST(X25519Test, TestVector) {
@@ -132,11 +133,8 @@
t->IgnoreInstruction("curve");
t->IgnoreAttribute("curve");
- // Our implementation tolerates the Wycheproof "acceptable"
- // inputs. Wycheproof's valid vs. acceptable criteria does not match our
- // X25519 return value, so we test only the overall output.
- t->IgnoreAttribute("result");
-
+ WycheproofResult result;
+ ASSERT_TRUE(GetWycheproofResult(t, &result));
std::vector<uint8_t> priv, pub, shared;
ASSERT_TRUE(t->GetBytes(&priv, "private"));
ASSERT_TRUE(t->GetBytes(&pub, "public"));
@@ -144,7 +142,8 @@
ASSERT_EQ(32u, priv.size());
ASSERT_EQ(32u, pub.size());
uint8_t secret[32];
- X25519(secret, priv.data(), pub.data());
+ int ret = X25519(secret, priv.data(), pub.data());
+ EXPECT_EQ(ret, result.IsValid({"NonCanonicalPublic", "Twist"}) ? 1 : 0);
EXPECT_EQ(Bytes(secret), Bytes(shared));
});
}
diff --git a/crypto/ecdh_extra/ecdh_test.cc b/crypto/ecdh_extra/ecdh_test.cc
index 52e49f1..4b88754 100644
--- a/crypto/ecdh_extra/ecdh_test.cc
+++ b/crypto/ecdh_extra/ecdh_test.cc
@@ -140,22 +140,16 @@
ASSERT_TRUE(GetWycheproofResult(t, &result));
std::vector<uint8_t> shared;
ASSERT_TRUE(t->GetBytes(&shared, "shared"));
+ // BoringSSL supports compressed coordinates.
+ bool is_valid = result.IsValid({"CompressedPoint"});
// Wycheproof stores the peer key in an SPKI to mimic a Java API mistake.
// This is non-standard and error-prone.
CBS cbs;
CBS_init(&cbs, peer_spki.data(), peer_spki.size());
bssl::UniquePtr<EVP_PKEY> peer_evp(EVP_parse_public_key(&cbs));
- if (!peer_evp) {
- // Note some of Wycheproof's "acceptable" entries are unsupported by
- // BoringSSL because they test explicit curves (forbidden by RFC 5480),
- // while others are supported because they used compressed coordinates. If
- // the peer key fails to parse, we consider it to match "acceptable", but if
- // the resulting shared secret matches below, it too matches "acceptable".
- //
- // TODO(davidben): Use the flags field to disambiguate these. Possibly
- // first get the Wycheproof folks to use flags more consistently.
- EXPECT_NE(WycheproofResult::kValid, result);
+ if (!peer_evp || CBS_len(&cbs) != 0) {
+ EXPECT_FALSE(is_valid);
return;
}
EC_KEY *peer_ec = EVP_PKEY_get0_EC_KEY(peer_evp.get());
@@ -170,11 +164,11 @@
int ret =
ECDH_compute_key(actual.data(), actual.size(),
EC_KEY_get0_public_key(peer_ec), key.get(), nullptr);
- if (result == WycheproofResult::kInvalid) {
- EXPECT_EQ(-1, ret);
- } else {
+ if (is_valid) {
EXPECT_EQ(static_cast<int>(actual.size()), ret);
EXPECT_EQ(Bytes(shared), Bytes(actual.data(), static_cast<size_t>(ret)));
+ } else {
+ EXPECT_EQ(-1, ret);
}
}
diff --git a/crypto/evp/evp_test.cc b/crypto/evp/evp_test.cc
index 4846fbc..9932982 100644
--- a/crypto/evp/evp_test.cc
+++ b/crypto/evp/evp_test.cc
@@ -645,13 +645,7 @@
bool sig_ok = DSA_check_signature(&valid, digest, digest_len, sig.data(),
sig.size(), dsa) &&
valid;
- if (result == WycheproofResult::kValid) {
- EXPECT_TRUE(sig_ok);
- } else if (result == WycheproofResult::kInvalid) {
- EXPECT_FALSE(sig_ok);
- } else {
- // this is a legacy signature, which may or may not be accepted.
- }
+ EXPECT_EQ(sig_ok, result.IsValid());
} else {
bssl::ScopedEVP_MD_CTX ctx;
EVP_PKEY_CTX *pctx;
@@ -664,14 +658,12 @@
}
int ret = EVP_DigestVerify(ctx.get(), sig.data(), sig.size(), msg.data(),
msg.size());
- if (result == WycheproofResult::kValid) {
- EXPECT_EQ(1, ret);
- } else if (result == WycheproofResult::kInvalid) {
- EXPECT_EQ(0, ret);
- } else {
- // this is a legacy signature, which may or may not be accepted.
- EXPECT_TRUE(ret == 1 || ret == 0);
- }
+ // BoringSSL does not enforce policies on weak keys and leaves it to the
+ // caller.
+ EXPECT_EQ(ret,
+ result.IsValid({"SmallModulus", "SmallPublicKey", "WeakHash"})
+ ? 1
+ : 0);
}
});
}
diff --git a/crypto/fipsmodule/aes/aes_test.cc b/crypto/fipsmodule/aes/aes_test.cc
index 5061e01..4c913d3 100644
--- a/crypto/fipsmodule/aes/aes_test.cc
+++ b/crypto/fipsmodule/aes/aes_test.cc
@@ -176,7 +176,7 @@
WycheproofResult result;
ASSERT_TRUE(GetWycheproofResult(t, &result));
- if (result != WycheproofResult::kInvalid) {
+ if (result.IsValid()) {
ASSERT_GE(ct.size(), 8u);
AES_KEY aes;
@@ -218,7 +218,10 @@
// should pass. However, both RFC 5649 and SP 800-38F section 5.3.1 say that
// the minimum length is one. Therefore we consider test cases with an empty
// message to be invalid.
- if (result != WycheproofResult::kInvalid && !msg.empty()) {
+ //
+ // Wycheproof marks various weak parameters as acceptable. We do not enforce
+ // policy in the library, so we map those flags to valid.
+ if (result.IsValid({"SmallKey", "WeakWrapping"}) && !msg.empty()) {
AES_KEY aes;
ASSERT_EQ(0, AES_set_decrypt_key(key.data(), 8 * key.size(), &aes));
std::vector<uint8_t> out(ct.size() - 8);
diff --git a/crypto/hkdf/hkdf_test.cc b/crypto/hkdf/hkdf_test.cc
index 504e927..793506f 100644
--- a/crypto/hkdf/hkdf_test.cc
+++ b/crypto/hkdf/hkdf_test.cc
@@ -286,8 +286,8 @@
std::vector<uint8_t> out(atoi(size_str.c_str()));
bool ret = HKDF(out.data(), out.size(), md, ikm.data(), ikm.size(),
salt.data(), salt.size(), info.data(), info.size());
- EXPECT_EQ(result == WycheproofResult::kValid, ret);
- if (result == WycheproofResult::kValid) {
+ EXPECT_EQ(result.IsValid(), ret);
+ if (result.IsValid()) {
EXPECT_EQ(Bytes(okm), Bytes(out));
}
});
diff --git a/crypto/hmac_extra/hmac_test.cc b/crypto/hmac_extra/hmac_test.cc
index b96abbd..c2d6199 100644
--- a/crypto/hmac_extra/hmac_test.cc
+++ b/crypto/hmac_extra/hmac_test.cc
@@ -142,7 +142,7 @@
WycheproofResult result;
ASSERT_TRUE(GetWycheproofResult(t, &result));
- if (result != WycheproofResult::kValid) {
+ if (!result.IsValid()) {
// Wycheproof tests assume the HMAC implementation checks the MAC. Ours
// simply computes the HMAC, so skip the tests with invalid outputs.
return;
diff --git a/crypto/test/wycheproof_util.cc b/crypto/test/wycheproof_util.cc
index 8f7dfeb..cd870dd 100644
--- a/crypto/test/wycheproof_util.cc
+++ b/crypto/test/wycheproof_util.cc
@@ -14,6 +14,10 @@
#include "./wycheproof_util.h"
+#include <stdlib.h>
+
+#include <algorithm>
+
#include <openssl/bn.h>
#include <openssl/digest.h>
#include <openssl/ec.h>
@@ -22,21 +26,55 @@
#include "./file_test.h"
+bool WycheproofResult::IsValid(
+ const std::vector<std::string> &acceptable_flags) const {
+ switch (raw_result) {
+ case WycheproofRawResult::kValid:
+ return true;
+ case WycheproofRawResult::kInvalid:
+ return false;
+ case WycheproofRawResult::kAcceptable:
+ for (const auto &flag : flags) {
+ if (std::find(acceptable_flags.begin(), acceptable_flags.end(), flag) ==
+ acceptable_flags.end()) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ abort();
+}
+
bool GetWycheproofResult(FileTest *t, WycheproofResult *out) {
std::string result;
if (!t->GetAttribute(&result, "result")) {
return false;
}
if (result == "valid") {
- *out = WycheproofResult::kValid;
+ out->raw_result = WycheproofRawResult::kValid;
} else if (result == "invalid") {
- *out = WycheproofResult::kInvalid;
+ out->raw_result = WycheproofRawResult::kInvalid;
} else if (result == "acceptable") {
- *out = WycheproofResult::kAcceptable;
+ out->raw_result = WycheproofRawResult::kAcceptable;
} else {
t->PrintLine("Bad result string '%s'", result.c_str());
return false;
}
+
+ out->flags.clear();
+ if (t->HasAttribute("flags")) {
+ std::string flags = t->GetAttributeOrDie("flags");
+ size_t idx = 0;
+ while (idx < flags.size()) {
+ size_t comma = flags.find(',', idx);
+ if (comma == std::string::npos) {
+ comma = flags.size();
+ }
+ out->flags.push_back(flags.substr(idx, comma - idx));
+ idx = comma + 1;
+ }
+ }
return true;
}
diff --git a/crypto/test/wycheproof_util.h b/crypto/test/wycheproof_util.h
index 4d8a14c..67e0ed3 100644
--- a/crypto/test/wycheproof_util.h
+++ b/crypto/test/wycheproof_util.h
@@ -17,18 +17,31 @@
#include <openssl/base.h>
+#include <string>
+#include <vector>
+
// This header contains convenience functions for Wycheproof tests.
class FileTest;
-enum class WycheproofResult {
+enum class WycheproofRawResult {
kValid,
kInvalid,
kAcceptable,
};
-// GetWycheproofResult sets |*out| to the parsed "result" key of |t|.
+struct WycheproofResult {
+ WycheproofRawResult raw_result;
+ std::vector<std::string> flags;
+
+ // IsValid returns true if the Wycheproof test should be considered valid. A
+ // test result of "acceptable" is treated as valid if all flags are included
+ // in |acceptable_flags| and invalid otherwise.
+ bool IsValid(const std::vector<std::string> &acceptable_flags = {}) const;
+};
+
+// GetWycheproofResult sets |*out| to the parsed "result" and "flags" keys of |t|.
bool GetWycheproofResult(FileTest *t, WycheproofResult *out);
// GetWycheproofDigest returns a digest function using the Wycheproof name, or
