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

fips: enforce minimum MAC key length of 112 bits #24199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xnox
Copy link
Contributor

@xnox xnox commented Apr 18, 2024

Most regulations require 112 bits. Post 2030 transition to 128 bits is likely. Implement security check to validate minimum MAC key length.

For FIPS, set it to 112 bits. This is inline with key sizes recommendations in https://doi.org/10.6028/NIST.SP.800-131Ar2.

This is a more sustainable change #24190

Checklist
  • documentation is added or updated
  • tests are added or updated
External guidance

Is the TLS1.3 KDF Extract KAT valid?

OSSL_SELF_TEST_DESC_KDF_TLS13_EXTRACT,

The KAT is valid.

Or is MAC key length somehow should only be enforced for the HMAC, KMAC, KDF external user functions, and yet allow TLS1.3-KDF internally only to call HMAC with an empty key?

The HMAC key can be an all zero bit string for the extraction operation of the TLS 1.3 KDF, so TLS 1.3 KDF should be able to internally call HMAC with an empty key.

When HMAC/KMAC are used as standalone functions (to output MAC tags to the operator), the HMAC/KMAC key must be at least 112 bits.

When HMAC/KMAC are used in KBKDF (key based key derivation function), the HMAC/KMAC key must be at least 112 bits.

When HMAC is used in the TLS 1.3 KDF or any other two step KDF (extract then expand) or one step KDF, a salt is used as the HMAC key and there is no length requirement. The salt is explicitly allowed by NIST to be all zeroes.

In the extraction step of the two step KDF, a salt is used as the HMAC key and there is no length requirement on it.

When HMAC is used in PBKDF, a password is used as the HMAC key and there is no length requirement imposed by NIST (a minimum length has to be explicitly chosen by the vendor, the vendor has to informally justify the minimum length, and the minimum length must be enforced by the code, but the vendor has the freedom to pick the minimum length).

My current implementation I believe satisfies all of the above with the following caveat: no distinction is made w.r.t. salt or key, meaning a suitably long all zeroes key is currently valid for HMAC/KMAC. I will try to add test cases to assert this. I don't know if that's ok - as basically that's our current defacto hint that one is passing in salt, not key. If that is not acceptable.... then some sort of in-provider-only API needs to be created that make distinction between KEY and SALT for HMAC/KMAC.

Separately this code update doesn't touch PBKDF2 as it seems to already do 112bit min enforcement. But will re-verify that.

@xnox xnox marked this pull request as draft April 18, 2024 18:43
@xnox xnox force-pushed the min-mac-key-securitycheck branch from 11d1baa to d4ab0a2 Compare April 18, 2024 18:44
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 18, 2024
@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Apr 19, 2024
@t8m
Copy link
Member

t8m commented Apr 19, 2024

I strongly believe that for this kind of nonsensical FIPS requirements that need to have clear exceptions we should use only explicit indicators.

@xnox
Copy link
Contributor Author

xnox commented Apr 19, 2024

I strongly believe that for this kind of nonsensical FIPS requirements that need to have clear exceptions we should use only explicit indicators.

I am still perplexed by this. Alternative is to have internal-only api, which KDF functions would use, which enforce key size at KDF start, but internally can use HMAC with an empty key size. Or maybe I misunderstood the requirements here.

@xnox
Copy link
Contributor Author

xnox commented Apr 19, 2024

(gdb) run kdf -keylen 32 -kdfopt key:16charlongsecret -kdfopt digest:SHA2-256 -kdfopt mode:EXTRACT_ONLY TLS13-KDF
Starting program: /home/xnox/upstream/openssl/apps/openssl kdf -keylen 32 -kdfopt key:16charlongsecret -kdfopt digest:SHA2-256 -kdfopt mode:EXTRACT_ONLY TLS13-KDF

Breakpoint 1, ossl_mac_check_key (min=0, requested=0) at providers/common/securitycheck.c:243
243	    return (requested >= 112);
(gdb) bt
#0  ossl_mac_check_key (min=0, requested=0) at providers/common/securitycheck.c:243
#1  0x00007ffff7c7cb37 in hmac_setkey (macctx=0x5555556b0a60, key=0x7ffff7d285e0 <default_zeros> "", keylen=0)
    at providers/implementations/macs/hmac_prov.c:150
#2  0x00007ffff7c7ccfe in hmac_init (vmacctx=0x5555556b0a60, key=0x7ffff7d285e0 <default_zeros> "", keylen=0, params=0x0)
    at providers/implementations/macs/hmac_prov.c:181
#3  0x00007ffff7a38ca2 in EVP_MAC_init (ctx=0x5555556bab40, key=0x7ffff7d285e0 <default_zeros> "", keylen=0, params=0x0) at crypto/evp/mac_lib.c:118
#4  0x00007ffff7a394d3 in EVP_Q_mac (libctx=0x7ffff7e29c00 <default_context_int>, name=0x7ffff7d2854a "HMAC", propq=0x0, subalg=0x5555556b1760 "SHA2-256", 
    params=0x0, key=0x7ffff7d285e0 <default_zeros>, keylen=0, data=0x5555556b8450 "16charlongsecret", datalen=16, out=0x5555556ac5f0 "\214\220?", 
    outsize=32, outlen=0x0) at crypto/evp/mac_lib.c:281
#5  0x00007ffff7c62361 in HKDF_Extract (libctx=0x7ffff7e29c00 <default_context_int>, evp_md=0x5555556b1660, salt=0x7ffff7d285e0 <default_zeros> "", 
    salt_len=0, ikm=0x5555556b8450 "16charlongsecret", ikm_len=16, prk=0x5555556ac5f0 "\214\220?", prk_len=32) at providers/implementations/kdfs/hkdf.c:446
#6  0x00007ffff7c62a1f in prov_tls13_hkdf_generate_secret (libctx=0x7ffff7e29c00 <default_context_int>, md=0x5555556b1660, 
    prevsecret=0x7ffff7d285e0 <default_zeros> "", prevsecretlen=0, insecret=0x5555556b8450 "16charlongsecret", insecretlen=16, prefix=0x0, prefixlen=0, 
    label=0x0, labellen=0, out=0x5555556ac5f0 "\214\220?", outlen=32) at providers/implementations/kdfs/hkdf.c:657
#7  0x00007ffff7c62bcc in kdf_tls1_3_derive (vctx=0x5555556b09d0, key=0x5555556ac5f0 "\214\220?", keylen=32, params=0x0)
    at providers/implementations/kdfs/hkdf.c:685
#8  0x00007ffff7a30eb6 in EVP_KDF_derive (ctx=0x5555556b09b0, key=0x5555556ac5f0 "\214\220?", keylen=32, params=0x0) at crypto/evp/kdf_lib.c:144
#9  0x00005555555c887f in kdf_main (argc=1, argv=0x7fffffffdd88) at apps/kdf.c:181
#10 0x00005555555d2c65 in do_cmd (prog=0x5555556a3690, argc=10, argv=0x7fffffffdd40) at apps/openssl.c:426
#11 0x00005555555d27ed in main (argc=10, argv=0x7fffffffdd40) at apps/openssl.c:307

@xnox
Copy link
Contributor Author

xnox commented Apr 19, 2024

Let's see if #24204 resolves the zero-length mac key during HKDF.

@xnox xnox force-pushed the min-mac-key-securitycheck branch from cac6395 to 85dcb76 Compare April 19, 2024 11:10
@jvdsn
Copy link
Contributor

jvdsn commented Apr 19, 2024

How does this patch interact with PBKDF2? That algorithm uses HMAC internally on a password which may not be 112 bits long.

@xnox
Copy link
Contributor Author

xnox commented Apr 20, 2024

How does this patch interact with PBKDF2? That algorithm uses HMAC internally on a password which may not be 112 bits long.

I've updated current status on that in the description of the pull request. tl;dr need to double check stuff.

@paulidale
Copy link
Contributor

My though would be to ban this by default (implicit indicator) but have the option of a bypass which sets an explicit indicator.

This is what @slontis and I have been trying to plan.

@xnox
Copy link
Contributor Author

xnox commented Apr 22, 2024

My though would be to ban this by default (implicit indicator) but have the option of a bypass which sets an explicit indicator.

This is what @slontis and I have been trying to plan.

With this implementation:

# openssl mac -digest SHA-256 -macopt hexkey:0000000000000000000000000000 -in /dev/null HMAC
B613679A0814D9EC772F95D778C35FC5FF1697C493715653C6C712144292C5AD

Is allowed.

Imho, enforcing same length for key and salt is good. And imho it sounds legit to assume all-zeros key is salt.

Otherwise, we need to change API of the MAC context, to allow setting "key/keyhex" or "salt/salthex" properties the destiction being which length (can be same) is enforced, and if all-zeroes are enforced or not.

Possibly we should be able to make "set salt" api internal only and not exported at all..... to allow only KDF like functions to set that. Unless that brings no value at all.

@xnox xnox marked this pull request as ready for review April 22, 2024 14:46
Most regulations require 112 bits. Post 2030 transition to 128 bits is
likely. Implement security check to validate minimum MAC key length.

For FIPS, set it to 112 bits. This is inline with key sizes
recommendations in https://doi.org/10.6028/NIST.SP.800-131Ar2.

Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk>
@xnox xnox force-pushed the min-mac-key-securitycheck branch from 42efaa9 to 6d89e97 Compare April 28, 2024 18:33
@xnox
Copy link
Contributor Author

xnox commented May 9, 2024

My though would be to ban this by default (implicit indicator) but have the option of a bypass which sets an explicit indicator.

This is what @slontis and I have been trying to plan.

any concerns about this code or implementation? as this currently is a block aka implicit indicator.

An explicit indicator can be added later.

I also ponder to add a "no-fips-indicators" build type, which would not have any bypasses - because i haven't seen any software actually implement or use indicators correctly.

cc @paulidale @slontis

@paulidale
Copy link
Contributor

I also ponder to add a "no-fips-indicators" build type, which would not have any bypasses

An interesting idea. @slontis, maybe add this to the indicators proposal?

because i haven't seen any software actually implement or use indicators correctly.

This is precisely why we want implicit indicators by default with a way to circumvent if required.
If we just did explicit indicators, nobody would check them and they'd be running out of compliance and likely less securely.

@0140454
Copy link
Contributor

0140454 commented May 10, 2024

I am curious about how this patch interact with a two-step KDF (e.g. HKDF)?

As per the response from our lab and your statement in PR description, there is no requirement on the salt used in HKDF. However, it seems that the length of salt must be at least 112 bits after applying this changes.

@xnox
Copy link
Contributor Author

xnox commented May 10, 2024

I am curious about how this patch interact with a two-step KDF (e.g. HKDF)?

As per the response from our lab and your statement in PR description, there is no requirement on the salt used in HKDF. However, it seems that the length of salt must be at least 112 bits after applying this changes.

Yes, but the vendor must pick minimum salt length and enforce it.

So.... For ease of implementation I chose it to be the same.

It would be amazing if you or anybody else can provide:
an alternative salt size; rearrange all APIs to add flag to indicate when key or salt is set; and enforce different lengths for them; and explain the choice of salt that labs and NIST would accept.

Note the above can always be done at a later time, on top of these proposed changes.

Note in 2030 transition to 128 will happen this lengths should be at the very least defines.

@t8m
Copy link
Member

t8m commented May 10, 2024

IMO this clearly shows that many FIPS requirements are nonsense that sometimes cannot even be reasonably obeyed.

The minimum MAC key length is a clear example. For such requirements we should provide an explicit indicator. We could optionally provide an implicit indicator with blocking the operation but that absolutely must be OFF by default.

And yeah, when HMAC is used in the HKDF context it is obvious that the explicit indicator value from the HMAC would have to be ignored and should not affect the approved status of the whole HKDF operation.

@xnox
Copy link
Contributor Author

xnox commented May 10, 2024

IMO this clearly shows that many FIPS requirements are nonsense that sometimes cannot even be reasonably obeyed.

The minimum MAC key length is a clear example. For such requirements we should provide an explicit indicator. We could optionally provide an implicit indicator with blocking the operation but that absolutely must be OFF by default.

And yeah, when HMAC is used in the HKDF context it is obvious that the explicit indicator value from the HMAC would have to be ignored and should not affect the approved status of the whole HKDF operation.

New FIPS submissions require HKDF minimum input & output lengths of 112 bits. This PR is only doing half of what is required for new submissions. It is actually more than this.

Ok, but what is the current meaning of "--enable-fips" ? because in current master branch it doesn't do nowhere near enough to satisfy any of the current 140-2 nor 140-3 requirements.

This is because requirements for both 140-2 and 140-3 have evolved over time, and many opt-outs have expired by time for new submissions.

Should I add a new configuration feature "enable-fips-future" or "enable-fips-implicit-only" or "enable-fips-2024" which does this and other similar PRs that are receiving push back (i.e. removing P-196 and lower).

Because sure, one can use explicit indicators for now; but that's still kicking things down the road as eventually indicators will not be allowed or desired either. And I'm trying to stay ahead of the curve here.

@xnox
Copy link
Contributor Author

xnox commented May 10, 2024

@t8m can i join OTC meeting or some such to discuss future plans for fips, such that I can make progress towards end goals; in piece meal upgrades. Because current status quo is "uncertifiable" and this PR tries to move the needle towards "less uncertifiable".

I am under no illusion that these changes absolutely are breaking existing behaviour; and will prevent ability to match past behaviour, which has now expired for usage.

@t8m
Copy link
Member

t8m commented May 10, 2024

as eventually indicators will not be allowed or desired either.

Any background for this? I do not see any reason why they would not be allowed in future. And it is definitely not something that could happen earlier than in FIPS 140-4 or something.

@xnox
Copy link
Contributor Author

xnox commented May 10, 2024

as eventually indicators will not be allowed or desired either.

Any background for this? I do not see any reason why they would not be allowed in future. And it is definitely not something that could happen earlier than in FIPS 140-4 or something.

See CMVP Implementation Guidance where allowed things in existing modules are continued to be allowed; but testing of new submissions or updated submissions by labs become prohibited. Example

IG D.G, RSA-based key encapsulation/un-encapsulation algorithm with PKCS1-v1.5 padding allowance has expired in December 31, 2023.

(labs are no longer allowed to test or submit that, despite existing modules out there allowing such usage - some without indicators, others with indicators)

Meaning, whilst current code/submissions may have that inplace, new submissions cannot. Hence "fips-future" configuration comes in, because if you are a new submission you must block PKCS1-v1.5 padding for encryption (still allowed for signature creation and verfication).

Hence the question of what is the purpose of "enable-fips" is it backwards compat with existing submissions or for new players / new submissions. Because the needs, wants, and what is possible to submit diverge with time.

About this code - would it help to set default to 0 and allow a define/configuration to ovveride the enforced length to whatever one wants? Because the length here would be:

  • 0 for updates of existing submisisons
  • 112 for new submissions
  • 128 for submissions attempting to be year 2030+ compliant due to upcoming announced 128 transition

@jvdsn
Copy link
Contributor

jvdsn commented May 10, 2024

as eventually indicators will not be allowed or desired either.

Any background for this? I do not see any reason why they would not be allowed in future. And it is definitely not something that could happen earlier than in FIPS 140-4 or something.

See CMVP Implementation Guidance where allowed things in existing modules are continued to be allowed; but testing of new submissions or updated submissions by labs become prohibited. Example

I don't know if that's the best way to put it. At its core, indicators are required in any case where you have approved services (algorithms) and non-approved services (algorithms) in the same boundary. The part that changes is which algorithms CMVP considers as approved or non-approved. There will always be indicators as long as the module implements non-approved algorithms.

I am curious about how this patch interact with a two-step KDF (e.g. HKDF)?
As per the response from our lab and your statement in PR description, there is no requirement on the salt used in HKDF. However, it seems that the length of salt must be at least 112 bits after applying this changes.

Yes, but the vendor must pick minimum salt length and enforce it.

I don't believe there is such a requirement on the salt length. In any case, HKDF isn't the easiest example to use for service indicators. This is because HKDF is not an explicitly approved or allowed algorithm by CMVP (you won't find it in https://csrc.nist.gov/Projects/cryptographic-module-validation-program/sp-800-140-series-supplemental-information/sp800-140d). HKDF can be considered as a specific instance of the approved TwoStep KDF algorithm defined in SP 800-56Cr2. However, that also means that it needs to comply to the rules for the algorithms in SP 800-56Cr2.

I do agree with the point that the rules for underlying algorithms don't necessarily apply to the higher level algorithms. E.g. for HKDF, the rules for HMAC key sizes should not be applied to the salt.

@xnox
Copy link
Contributor Author

xnox commented May 10, 2024

as eventually indicators will not be allowed or desired either.

Any background for this? I do not see any reason why they would not be allowed in future. And it is definitely not something that could happen earlier than in FIPS 140-4 or something.

See CMVP Implementation Guidance where allowed things in existing modules are continued to be allowed; but testing of new submissions or updated submissions by labs become prohibited. Example

I don't know if that's the best way to put it. At its core, indicators are required in any case where you have approved services (algorithms) and non-approved services (algorithms) in the same boundary. The part that changes is which algorithms CMVP considers as approved or non-approved. There will always be indicators as long as the module implements non-approved algorithms.

I am curious about how this patch interact with a two-step KDF (e.g. HKDF)?
As per the response from our lab and your statement in PR description, there is no requirement on the salt used in HKDF. However, it seems that the length of salt must be at least 112 bits after applying this changes.

Yes, but the vendor must pick minimum salt length and enforce it.

I don't believe there is such a requirement on the salt length. In any case, HKDF isn't the easiest example to use for service indicators. This is because HKDF is not an explicitly approved or allowed algorithm by CMVP (you won't find it in https://csrc.nist.gov/Projects/cryptographic-module-validation-program/sp-800-140-series-supplemental-information/sp800-140d). HKDF can be considered as a specific instance of the approved TwoStep KDF algorithm defined in SP 800-56Cr2. However, that also means that it needs to comply to the rules for the algorithms in SP 800-56Cr2.

I do agree with the point that the rules for underlying algorithms don't necessarily apply to the higher level algorithms. E.g. for HKDF, the rules for HMAC key sizes should not be applied to the salt.

Ok, but right now there is no API to HMAC to specify if something is a "key" or "salt". Do we need to add flags to HMAC derive to specify if one is setting salt or key? I.e. "set salt" vs "set key" - which for HMAC algorithm computation is the same, but have different size enforcement policies.

@jvdsn
Copy link
Contributor

jvdsn commented May 10, 2024

Ok, but right now there is no API to HMAC to specify if something is a "key" or "salt". Do we need to add flags to HMAC derive to specify if one is setting salt or key? I.e. "set salt" vs "set key" - which for HMAC algorithm computation is the same, but have different size enforcement policies.

In general, there's a couple of ways to go about this:

  • If the HMAC API blocks the non-approved (according to the HMAC approval rules) usage by default, then the HKDF API can either use a lower-level HMAC API (which does not contain the blocking) or pass some flag to the HMAC API to allow the non-approved (according to the HMAC approval rules) usage.
  • If the HMAC API allows the non-approved (according to the HMAC approval rules) usage by default but sets an indicator, then the HKDF API can ignore that indicator (i.e., not propagate it to the user) and substitute its own based on the HKDF approval rules.

@xnox
Copy link
Contributor Author

xnox commented May 10, 2024

Ok, but right now there is no API to HMAC to specify if something is a "key" or "salt". Do we need to add flags to HMAC derive to specify if one is setting salt or key? I.e. "set salt" vs "set key" - which for HMAC algorithm computation is the same, but have different size enforcement policies.

In general, there's a couple of ways to go about this:

  • If the HMAC API blocks the non-approved (according to the HMAC approval rules) usage by default, then the HKDF API can either use a lower-level HMAC API (which does not contain the blocking) or pass some flag to the HMAC API to allow the non-approved (according to the HMAC approval rules) usage.
  • If the HMAC API allows the non-approved (according to the HMAC approval rules) usage by default but sets an indicator, then the HKDF API can ignore that indicator (i.e., not propagate it to the user) and substitute its own based on the HKDF approval rules.

I like how KDF_PBKDF2_MIN_SALT_LEN is implemented and enforced in pbkdf2 which already enforces 128bit salt it seems.

Trying to see how I can create a fips-internal provider API only, to have something like EVP_MD_CTX_FLAG_KEY_IS_SALT which only TLS / two-step kdf could set, within the fips provider boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants