-
Notifications
You must be signed in to change notification settings - Fork 3k
Crypto: Hide forbidden algorithms when FIPS enabled #10312
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
base: maint
Are you sure you want to change the base?
Conversation
CT Test Results 2 files 14 suites 4m 43s ⏱️ For more details on these failures, see this check. Results for commit 0f9b884. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
4fd908c to
65f3f3d
Compare
0c6bd7f to
aa72b5e
Compare
sverker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing, and it seems like
EVP_MD_fetch(NULL, p->str_v3, "fips=yes") // if supported with FIPS enabled
EVP_MD_fetch(NULL, p->str_v3, "fips=no") // if supported with FIPS disabled
EVP_MD_fetch(NULL, p->str_v3, "") // if supported in current mode set by EVP_default_properties_enable_fips()
lib/crypto/c_src/algorithms.c
Outdated
| static bool IS_PUBKEY_FORBIDDEN_IN_FIPS(const struct pkey_availability_t* p) { | ||
| return p->flags == FIPS_FORBIDDEN_PKEY_ALL | ||
| || p->flags == FIPS_PKEY_NOT_AVAIL; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a difference between FIPS_PKEY_NOT_AVAIL and FIPS_FORBIDDEN_PKEY_ALL?
lib/crypto/c_src/algorithms.c
Outdated
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_from_name(NULL, str_v3, "fips=yes"); | ||
| /* failed: algorithm not available, do not add */ | ||
| if (ctx) { | ||
| if (EVP_PKEY_keygen_init(ctx) <= 0) { /* can't generate keys */ | ||
| unavailable |= FIPS_FORBIDDEN_PKEY_KEYGEN; | ||
| } | ||
| EVP_PKEY_CTX_free(ctx); | ||
|
|
||
| ctx = EVP_PKEY_CTX_new_from_name(NULL, str_v3, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "fips=yes" in one place but not the other.
lib/crypto/c_src/algorithms.c
Outdated
| static size_t curves_lazy_init(ErlNifEnv* env, const bool fips_enabled) { | ||
| size_t result = 0; | ||
| if (algo_curve.count >= 0) return algo_curve.count; | ||
|
|
||
| enif_mutex_lock(algo_curve.mtx_init_curve_types); | ||
| if (algo_curve.count < 0) { | ||
| init_curves(env, fips_enabled); /* also updates algo_curve.count[0] or [1] */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pass fips_enabled to init_curves? We only have one array of curves initialized once here. And fips mode can be switched on/off in runtime with crypto:enable_fips_mode/1 (discouraged but used by our tests).
| enif_make_existing_atom(env, atom_name, &atom, ERL_NIF_UTF8); | ||
| if (!atom) { | ||
| atom = enif_make_atom(env, atom_name); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enif_make_atom already does this optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused. In erl_nif.c: enif_make_atom calls enif_make_atom_len, which calls enif_make_new_atom_len, which calls erts_atom_put. That doesn't look like its checking for existence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... which calls erts_atom_put_index, which takes the atom table read lock and does a lookup.
atom_read_lock();
aix = index_get(&erts_atom_table, (void*) &a);
atom_read_unlock();
if (aix >= 0) {
/* Already in table no need to verify it */
return aix;
}
lib/crypto/c_src/algorithms_kem.cpp
Outdated
| return; // failed to find the algorithm, do not add | ||
| } | ||
| #endif // FIPS_SUPPORT && HAS_3_0_API | ||
| return output.push_back(algo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function return type is void ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just confusing.
lib/crypto/src/crypto.erl
Outdated
| Do not use this function in your code, it is designed to only be used by the crypto | ||
| library or by Erlang self-tests. This function is called automatically on first load | ||
| of the crypto NIF with the value `fips_mode :: true | false` from crypto app environment. | ||
|
|
||
| This operation is not thread-safe, it should only be called once (by the Erlang crypto | ||
| library) and user code calling it, while there are SSL operations running, might get | ||
| undesired consequences, because the attached OpenSSL library structures will switch | ||
| on the fly. Unintended non-FIPS algorithms might become enabled in your FIPS-only code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can skip the fact that enable_fips_mode is called once by the crypto app itself. That's an irrelevant implementation detail.
lib/crypto/c_src/fips.c
Outdated
| bool result = FIPS_mode_set(fips_mode) ? atom_true : atom_false; | ||
| if (result && previous_setting != fips_mode) { | ||
| /* Reinitialize the algorithms which may disappear or reappear when FIPS mode changes */ | ||
| algorithms_reset_cache(); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return type is ERL_NIF_TERM which cannot be treated as a bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testing for FIPS mode is not done yet. I now have tested only under non-FIPS, so those code blocks remain disabled with #ifdef, will find it out soon! Fixed this one, thanks for noticing.
f707bd4 to
ce79ee2
Compare
6a32681 to
a713a5e
Compare
|
Still WIP cleaning up the CI build errors, tests, under varied OpenSSL versions, but the main work is done |
a713a5e to
aeb8c84
Compare
FIPS doc page updated, for enable_fips_mode discourage its manual invocation New store module for algorithms Porting changes from maint Merge in crypto.erl and test SUITE
aeb8c84 to
0f9b884
Compare
Fixes #9520
Problem
crypto:supports/0is currently undocumented but i will add a doc string to it.Crypto returns some algorithms which are indirectly forbidden under FIPS (i.e. listed as available but not working).
The Expectation
Expected: All algorithms returned by
crypto:supports/0will be available at least in one way.Bonus points for returning forbidden algorithm names separately so that the users will not complain that something "has disappeared" and will see the reason.
Solution
The call to
crypto:supports/0under FIPS build with fips=enabled incryptoapp settings, will filter the algorithms by their availability in a much more strict way (by actually trying to initialize each algorithm and recording the failures as a sign of unavailability) and will return an additional section in the results containing disabled algorithms:Note this will only appear if FIPS is enabled. And this will only appear if SSL API is 3.0 or later.
The behaviour with FIPS disabled or older SSL will remain unchanged.
TO DO