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
base: master
Are you sure you want to change the base?
Conversation
11d1baa
to
d4ab0a2
Compare
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. |
(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 |
55201d4
to
cac6395
Compare
Let's see if #24204 resolves the zero-length mac key during HKDF. |
cac6395
to
85dcb76
Compare
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. |
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:
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. |
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>
42efaa9
to
6d89e97
Compare
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. |
An interesting idea. @slontis, maybe add this to the indicators proposal?
This is precisely why we want implicit indicators by default with a way to circumvent if required. |
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: 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. |
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. |
@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. |
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
(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:
|
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 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. |
In general, there's a couple of ways to go about this:
|
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 |
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
External guidance
Is the TLS1.3 KDF Extract KAT valid?
openssl/providers/fips/self_test_data.inc
Line 600 in ea31561
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?
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.