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.

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

NIST.SP.800-56Cr2 One-Step Key Derivation #1607

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Conversation

skmcgrail
Copy link
Member

@skmcgrail skmcgrail commented May 24, 2024

Description of changes:

This pull request implements the One-Step Key Derivation function defined in Section 4 of NIST.SP.800-56Cr2

Here we implement two of the three variants: option 1 (hash based) and option 2 (hmac based).

The abbreviation SSKDF is used in the public functions due to the extensive usage of the term within OpenSSL when referring to this algorithm. SSKDF == "Single-Step Key Derivation Function".

Call-outs:

  • We will need to add appropriate service indicator logic ahead of our next FIPS certification round.
  • There is some non-trivial amount of work required to wire-up the KDA-OneStep ACVP test vectors into our current Go ACVP setup and modulewrapper. I started going down this path, but there is some refactoring that requires to decouple some of the JSON structure from the current KDA-HKDF expectations. I'd rather keep this PR to the implementation and some confidence test vectors I ported from various other implementation sources.

Testing:

See the test vectors provided in sskdf.txt which I sourced from a number of locations as documented in that file. This currently provides coverage of the KDF functions for SHA-1, SHA-224, SHA-256, SHA-384, and SHA-512. SHA-3 algorithms will also get added coverage once we hook into the ACVP system.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

Attention: Patch coverage is 88.94472% with 22 lines in your changes missing coverage. Please review.

Project coverage is 78.19%. Comparing base (98735a2) to head (0dd1f41).

Files Patch % Lines
crypto/fipsmodule/kdf/sskdf.c 86.36% 21 Missing ⚠️
crypto/fipsmodule/kdf/kdf_test.cc 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1607      +/-   ##
==========================================
+ Coverage   78.16%   78.19%   +0.03%     
==========================================
  Files         562      564       +2     
  Lines       94683    94882     +199     
  Branches    13575    13604      +29     
==========================================
+ Hits        74008    74197     +189     
- Misses      20082    20091       +9     
- Partials      593      594       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skmcgrail skmcgrail changed the title One-step key derivation implementations NIST.SP.800-56Cr2 One-Step Key Derivation May 30, 2024
@skmcgrail skmcgrail marked this pull request as ready for review May 30, 2024 22:00
@skmcgrail skmcgrail requested a review from a team as a code owner May 30, 2024 22:00
@skmcgrail skmcgrail force-pushed the sskdf branch 4 times, most recently from ed1806d to 9d84a39 Compare June 3, 2024 17:36
@skmcgrail skmcgrail requested review from dkostic and nebeid June 3, 2024 19:23
include/openssl/kdf.h Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/internal.h Show resolved Hide resolved
crypto/fipsmodule/kdf/internal.h Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
@skmcgrail skmcgrail requested a review from dkostic June 7, 2024 00:41
@skmcgrail skmcgrail force-pushed the sskdf branch 2 times, most recently from 8b43006 to 69b5737 Compare June 10, 2024 15:53
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
Comment on lines 245 to 247
// UINT32_MAX is a sufficient max value to satisfy this requirement. See
// Section 4.2 Table 1 and 2
// https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Cr2.pdf
Copy link
Contributor

Choose a reason for hiding this comment

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

can you say a bit more on how you determine this based on those tables?

Copy link
Contributor

Choose a reason for hiding this comment

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

From the tables, it seems to me that we can allow up to 2^{64-3}-513/8 bytes for some hashes and 2^{128-3} - 1025/8 for the other ones. So why clamp it at UINT32_MAX? I understand we're limited by size_t of the length arguments, but that would make the check above enough, right? Could size_t exceed max_H_inputBits/8 according to the tables? I'm guessing no.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me revisit this with our internal customers first to see what the max derived output length they would reasonably require. It seems suspect that a single invocation of the SSKDF is going to ask for 32 GiB of output key material in one-shot with a given shared secret and info string?

OpenSSL's implementation currently places the following limitations that we might wish to impose to have matching capatability between our implementations:
secret_len <= 1<<30, info_len <= 1<<30, and also limits the requested derivation length to be derived_key_len <= 1<<30.

We could relax this restriction, but that could also result in issues in some hypothetical key agreement scheme where the party using AWS-LC is able to arrived to a derived key with larger inputs, while the party using OpenSSL would fail. Hypothetical of course, but such a scenario could arrise if we choose to differ. Regardless I think the call out here is that the values here, and those by OpenSSL are sufficient enough to satisfy the NIST requirements.

Copy link
Member Author

@skmcgrail skmcgrail Jun 13, 2024

Choose a reason for hiding this comment

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

Some proof that the OpenSSL limits are sufficient to cap the inputs to satisfy the NIST table 2 and 3:

#include <stddef.h>
#include <stdint.h>

#define OPENSSL_SSKDF_MAX (1 << 30)
#define NIST_MAX_APPROXIMATION (1UL << 60)

int check_overflow(size_t a, size_t b, size_t c) {
  if (a > SIZE_MAX - b || a + b > SIZE_MAX - c) {
    return 0;
  }
  return 1;
}

int main() {
  size_t z = OPENSSL_SSKDF_MAX;
  size_t info_len = OPENSSL_SSKDF_MAX;
  size_t counter = 4;

  if (!check_overflow(z, info_len, counter)) {
    return 1;
  }

  if (z + info_len + counter > NIST_MAX_APPROXIMATION) {
    return 2;
  }

  return 0;
}

This program returns 0 if you compile and execute it, thus showing the OpenSSL imposed limits are sufficient to appropriately cap using the smallest max input limit imposed by the tables (max 2^64 - 512 in bits). As the sum of the max secret length, max info length, and max counter will not exceed 2^60 which is the nearest power of 2 smaller then (2^64 - 512)/8. In total the limitation to the max input length for HMAC or Digest functions by OpenSSL results in the max allowed being no more then 2^32.

crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
Comment on lines 245 to 247
// UINT32_MAX is a sufficient max value to satisfy this requirement. See
// Section 4.2 Table 1 and 2
// https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Cr2.pdf
Copy link
Contributor

Choose a reason for hiding this comment

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

From the tables, it seems to me that we can allow up to 2^{64-3}-513/8 bytes for some hashes and 2^{128-3} - 1025/8 for the other ones. So why clamp it at UINT32_MAX? I understand we're limited by size_t of the length arguments, but that would make the check above enough, right? Could size_t exceed max_H_inputBits/8 according to the tables? I'm guessing no.

crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
include/openssl/kdf.h Show resolved Hide resolved
@skmcgrail skmcgrail force-pushed the sskdf branch 3 times, most recently from f21df2f to 61c1460 Compare June 13, 2024 23:32
crypto/fipsmodule/kdf/sskdf.c Outdated Show resolved Hide resolved
crypto/fipsmodule/kdf/sskdf.c Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
include/openssl/kdf.h Outdated Show resolved Hide resolved
@@ -174,6 +175,9 @@ static int sskdf_variant_hmac_compute(sskdf_variant_ctx *ctx, uint8_t *out,

uint32_t written;

// NIST.SP.800-56Cr2: Step 6.2 HMAC-hash(salt, counter || secret || info)
// Note: |variant_ctx->hmac_ctx| is already initalized with the salt during
// it's initial construction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
// it's initial construction.
// its initial construction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will make this documentation correction in the KDF counter PR.

@skmcgrail skmcgrail merged commit e3d34d7 into aws:main Jun 18, 2024
97 of 98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants