Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow HMAC via EVP_PKEY raw privkey functions #1338

Merged
merged 3 commits into from
Jan 19, 2024
Merged

allow HMAC via EVP_PKEY raw privkey functions #1338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11a5d91
allow HMAC via EVP_PKEY raw privkey functions
samuel40791765 Dec 26, 2023
95398a1
PR comments; Null checks and new test
samuel40791765 Jan 17, 2024
424ce04
missed free key
samuel40791765 Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions crypto/evp_extra/p_hmac_asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
#include <openssl/evp.h>
#include <openssl/mem.h>

#include "../internal.h"
#include "internal.h"


Expand All @@ -66,9 +67,56 @@ static int hmac_size(OPENSSL_UNUSED const EVP_PKEY *pkey) {

static void hmac_key_free(EVP_PKEY *pkey) {
HMAC_KEY *key = pkey->pkey.ptr;
if (key != NULL) {
OPENSSL_free(key->key);
}
OPENSSL_free(key);
}

static int hmac_set_key(EVP_PKEY *pkey, const uint8_t *priv, size_t len,
OPENSSL_UNUSED const uint8_t *pubkey,
OPENSSL_UNUSED size_t pubkey_len) {
if (pkey->pkey.ptr != NULL) {
return 0;
}

HMAC_KEY *key = HMAC_KEY_new();
if (key == NULL) {
return 0;
}

key->key = OPENSSL_memdup(priv, len);
if (key->key == NULL && len > 0) {
OPENSSL_free(key);
return 0;
samuel40791765 marked this conversation as resolved.
Show resolved Hide resolved
}
key->key_len = len;
pkey->pkey.ptr = key;
return 1;
}

static int hmac_get_key(const EVP_PKEY *pkey, uint8_t *priv, size_t *len) {
HMAC_KEY *key = pkey->pkey.ptr;
if (key == NULL || len == NULL) {
return 0;
}

// The semantics of the EVP APIs are to return the length, if |priv| is NULL.
if (priv == NULL) {
*len = key->key_len;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do a null check on len here, or can we always assume that to be a valid pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check above

return 1;
}

// Retrieve the key, if |*len| has a large enough length.
if (*len < key->key_len) {
return 0;
}
*len = key->key_len;
OPENSSL_memcpy(priv, key->key, key->key_len);
return 1;
}


const EVP_PKEY_ASN1_METHOD hmac_asn1_meth = {
EVP_PKEY_HMAC,
{0xff} /* placeholder oid */,
Expand All @@ -79,9 +127,9 @@ const EVP_PKEY_ASN1_METHOD hmac_asn1_meth = {
NULL /*priv_decode */,
NULL /* priv_encode */,
NULL /* priv_encode_v2 */,
NULL /* set_priv_raw */,
hmac_set_key /* set_priv_raw */,
NULL /* set_pub_raw */,
NULL /* get_priv_raw */,
hmac_get_key /* get_priv_raw */,
NULL /* get_pub_raw */,
NULL /* pkey_opaque */,
hmac_size /* pkey_size */,
Expand Down
7 changes: 6 additions & 1 deletion crypto/fipsmodule/evp/evp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,13 @@ EVP_PKEY *EVP_PKEY_new_mac_key(int type, ENGINE *engine, const uint8_t *mac_key,
if(key == NULL) {
goto err;
}
key->key = mac_key;
key->key = OPENSSL_memdup(mac_key, mac_key_len);
samuel40791765 marked this conversation as resolved.
Show resolved Hide resolved
if (key->key == NULL && mac_key_len > 0) {
OPENSSL_free(key);
goto err;
}
key->key_len = mac_key_len;

if(!EVP_PKEY_assign(ret, EVP_PKEY_HMAC, key)) {
OPENSSL_free(key);
goto err;
Expand Down
2 changes: 1 addition & 1 deletion crypto/fipsmodule/evp/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ typedef struct {
} HMAC_PKEY_CTX;

typedef struct {
const uint8_t *key;
uint8_t *key;
size_t key_len;
} HMAC_KEY;

Expand Down
36 changes: 36 additions & 0 deletions crypto/hmac_extra/hmac_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,42 @@ static void RunHMACTestEVP(const std::vector<uint8_t> &key,
}
ASSERT_TRUE(EVP_DigestSignFinal(mctx.get(), actual.data(), &len));
EXPECT_EQ(Bytes(tag), Bytes(actual.data(), tag.size()));


// Test |EVP_PKEY| key creation with |EVP_PKEY_new_raw_private_key|.
bssl::UniquePtr<EVP_PKEY> raw_pkey(EVP_PKEY_new_raw_private_key(
EVP_PKEY_HMAC, nullptr, key.data(), key.size()));
mctx.Reset();
len = 0;
actual.clear();
EXPECT_TRUE(
EVP_DigestSignInit(mctx.get(), nullptr, md, nullptr, raw_pkey.get()));
EXPECT_TRUE(EVP_DigestSignUpdate(mctx.get(), msg.data(), msg.size()));
EXPECT_TRUE(EVP_DigestSignFinal(mctx.get(), nullptr, &len));
actual.resize(len);
EXPECT_TRUE(EVP_DigestSignFinal(mctx.get(), actual.data(), &len));
actual.resize(len);
EXPECT_EQ(Bytes(tag), Bytes(actual.data(), tag.size()));

// Test retrieving key passed into |raw_pkey| with
// |EVP_PKEY_get_raw_private_key|.
std::vector<uint8_t> retrieved_key;
size_t retrieved_key_len;
EXPECT_TRUE(EVP_PKEY_get_raw_private_key(raw_pkey.get(), nullptr,
&retrieved_key_len));
EXPECT_EQ(key.size(), retrieved_key_len);
retrieved_key.resize(retrieved_key_len);
EXPECT_TRUE(EVP_PKEY_get_raw_private_key(raw_pkey.get(), retrieved_key.data(),
&retrieved_key_len));
retrieved_key.resize(retrieved_key_len);
EXPECT_EQ(Bytes(retrieved_key), Bytes(key));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering branch coverage... should we add a case where we resize the buffer andretrieved_key_len to something smaller than the key size and and expect failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I added one more here. There are cases where the key could be NULL, so I've filtered those out here.


// Test retrieving key with a buffer length that's too small. This should fail
if (!key.empty()) {
size_t short_key_len = retrieved_key_len - 1;
EXPECT_FALSE(EVP_PKEY_get_raw_private_key(
raw_pkey.get(), retrieved_key.data(), &short_key_len));
}
}


Expand Down
Loading