Opened 3 years ago

Closed 11 months ago

#19979 closed enhancement (implemented)

Use OpenSSL 1.1.0 HKDF in Tor when available.

Reported by: nickm Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: openssl110, easy, refactor, code-removal
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

OpenSSL 1.1.0 now includes HKDF support. We should consider using their implementation instead of ours when it's available.

Child Tickets

Change History (11)

comment:1 in reply to:  description Changed 3 years ago by icanhasaccount

Replying to nickm:

OpenSSL 1.1.0 now includes HKDF support. We should consider using their implementation instead of ours when it's available.

I had a quick look at this today - the implementation in openssl seems to fail (
error:0F073041:common libcrypto routines:CRYPTO_memdup:malloc failure) when the key length is zero (Its one of the tests for the current implementation in test_crypto.c). Example below as attachments are flagged as spam:

#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>
#include <string.h>

#include <openssl/err.h>
#include <openssl/evp.h>
#include <openssl/kdf.h>

#define N_KEYS 3

static const char *keys[] = {

"",
"Tor",
"AN ALARMING ITEM TO FIND ON YOUR CREDIT-RATING STATEMENT"

};

static const char *expected[] = {

/* "" */
"d3490ed48b12a48f9547861583573fe3f19aafe3f81dc7fc75"
"eeed96d741b3290f941576c1f9f0b2d463d1ec7ab2c6bf71cd"
"d7f826c6298c00dbfe6711635d7005f0269493edf6046cc7e7"
"dcf6abe0d20c77cf363e8ffe358927817a3d3e73712cee28d8",
/* Tor */
"5521492a85139a8d9107a2d5c0d9c91610d0f95989975ebee6"
"c02a4f8d622a6cfdf9b7c7edd3832e2760ded1eac309b76f8d"
"66c4a3c4d6225429b3a016e3c3d45911152fc87bc2de9630c3"
"961be9fdb9f93197ea8e5977180801926d3321fa21513e59ac",
/* AN ALARMING ITEM TO FIND ON YOUR CREDIT-RATING STATEMENT */
"a2aa9b50da7e481d30463adb8f233ff06e9571a0ca6ab6df0f"
"b206fa34e5bc78d063fc291501beec53b36e5a0e434561200c"
"5f8bd13e0f88b3459600b4dc21d69363e2895321c06184879d"
"94b18f078411be70b767c7fc40679a9440a0c95ea83a23efbf"

};

int
crypto_expand_key_material_rfc5869_sha256(

const uint8_t *key_in, size_t key_in_len,
const uint8_t *salt_in, size_t salt_in_len,
const uint8_t *info_in, size_t info_in_len,
uint8_t *key_out, size_t key_out_len)

{

EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, NULL);

if (info_in_len > 1024)

goto fail;

if (EVP_PKEY_derive_init(pctx) <= 0)

goto fail;

if (EVP_PKEY_CTX_set_hkdf_md(pctx, EVP_sha256()) <= 0)

goto fail;

if (EVP_PKEY_CTX_set1_hkdf_salt(pctx, salt_in, salt_in_len) <= 0)

goto fail;

if (EVP_PKEY_CTX_set1_hkdf_key(pctx, key_in, key_in_len) <= 0)

goto fail; if (EVP_PKEY_CTX_add1_hkdf_info(pctx, info_in, info_in_len) <= 0)
goto fail;

if (EVP_PKEY_derive(pctx, key_out, &key_out_len) <= 0)

goto fail;

EVP_PKEY_CTX_free(pctx);

return 0;

fail:

EVP_PKEY_CTX_free(pctx);
return -1;

}

void
dump_key_material(const uint8_t *key, const size_t key_sz)
{

for (size_t i = 0; i < key_sz; i++)

fprintf(stdout, "%02x", key[i]);

fprintf(stdout, "\n");

}

int
main(int argc, char argv)
{

uint8_t key_material[100];
const uint8_t salt[] = "ntor-curve25519-sha256-1:key_extract";
const size_t salt_len = strlen((char*)salt);
const uint8_t m_expand[] = "ntor-curve25519-sha256-1:key_expand";
const size_t m_expand_len = strlen((char*)m_expand);

OpenSSL_add_all_algorithms();

for (size_t i = 0; i < N_KEYS; i++) {

memset(key_material, 0, sizeof(key_material));
const int res = crypto_expand_key_material_rfc5869_sha256(

(uint8_t *)keys[i],
strlen(keys[i]), salt, salt_len,
m_expand, m_expand_len,
key_material, sizeof(key_material));

if (res == -1) {

fprintf(stdout, "Failed for key: \"%s\"\n", keys[i]);
fprintf(stdout, "Expected: \n%s\n", expected[i]);
const unsigned long err = ERR_get_error();
fprintf(stdout, "OpenSSL says wut: %s\n",

ERR_error_string(err, NULL));

continue;

}
fprintf(stdout, "For key: %s\n", keys[i]);
fprintf(stdout, "Expected: \n%s\n", expected[i]);
fprintf(stdout, "Actual: \n");
dump_key_material(key_material, sizeof(key_material));

}

EVP_cleanup();

}

Last edited 3 years ago by icanhasaccount (previous) (diff)

comment:2 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:3 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:4 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:5 Changed 2 years ago by nickm

Keywords: refactor code-removal added

comment:6 Changed 12 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:7 Changed 12 months ago by rl1987

Status: acceptedneeds_review

comment:8 Changed 11 months ago by dgoulet

Reviewer: nickm

comment:9 Changed 11 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: needs_reviewneeds_revision

Looks mostly good, but maybe we should just remove support for 0-length keys here? They don't actually achieve anything, we don't use them except in the tests, and they require us to compile both implementations.

Also, this code has moved from crypto.c into crypto_hkdf.c with d38e7ddf5b930ae7e4d3a5da63cfc32d92a8dfa7, but it would be easy enough to apply the patch to the new location.

comment:10 Changed 11 months ago by rl1987

Status: needs_revisionneeds_review

comment:11 Changed 11 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Thanks; merged!

Note: See TracTickets for help on using tickets.