-
Notifications
You must be signed in to change notification settings - Fork 27
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
crypto/internal: prevent cryptofallback in requirefips mode #1327
crypto/internal: prevent cryptofallback in requirefips mode #1327
Conversation
In many instances systemcrypto backend is tested for support of a particular algorithm. If it is supported, it is used, otherwise gocrypto fallback code path is used. This means effectively the toolchain allows to use MD5 DES TripleDES Ed25519 RC4 HKDF TLS1PF implemented with gocrypto, when FIPS module blocks these algorithms or doesn't even support them (i.e. when only base+fips providers are loaded). In some cases this might be due to incorrect check and/or incorrect runtime configuration of OpenSSL. It is very common to accidentaly activate "default" and "fips" providers in OpenSSL at the same time - which then exhibits odd properties. Specifically "default+fips" providers will list that RC4 and MD5 are supported without any property query strings. But fail at runtime when attempted to be used with property query string set to "fips=yes". If on the other hand "base" and "fips" providers loaded alone, RC4 and MD5 will not be listed as runtime available, and gocrypto fallback path may be taken by the toolchain. A similar issue is currently also present in cpython please see python/cpython#118224. Note that recommended way to configure OpenSSL in fips only mode is with base+fips providers alone - see https://github.com/openssl/openssl/blob/master/README-PROVIDERS.md such that default & legacy providers algorithms are not exposed at runtime. And this is how OpenSSL is configured in FIPS mode on Ubuntu Pro FIPS and Chainguard FIPS Images, and recommended by upstream. Please note internally md5 is used by go coverage, meaning in requirefips case coverage may fail to generate unless some additional APIs are introduced to allow insecure usage of md5 (equivalent to python's usedforsecurity=True|False flag). Or coverage ported to use SHA256. For a local reproducer use base+fips providers only, for example with following openssl.cnf: ``` config_diagnostics = 1 openssl_conf = openssl_init .include /etc/ssl/fipsmodule.cnf [openssl_init] providers = provider_sect alg_section = algorithm_sect [provider_sect] fips = fips_sect base = base_sect [base_sect] activate = 1 [algorithm_sect] default_properties = fips=yes ``` Or compile openssl without RC4 support by using 'no-rc4' configuration option. Signed-off-by: Dimitri John Ledkov <[email protected]>
Thanks for submitting this PR @xnox. Preventing falling back to Go crypto when On the other hand, we can't accept this right now, as it might break some of our costumers that expect non-fips algorithms to be available even is I would suggest you to create an issue in this same repo explaining your use case so we can better track this. |
Will do. Note that current behaviour is very mixed bag. As Supported APIs today can report misleading results (as in results from multiple providers). |
We have been making the |
which is possibly a double edged sword and will have unintended side-effects. For example, if today SupportsHash() returns True for a missing hash (false positive); boring.Enable is true; application is running in FIPS environment; and the attempt is made to the hash via OpenSSL which then at runtime correctly fails / is blocked. If SupportsHash() is fixed, to correctly return False for a missing hash (no false positivies); the app would fallback to gocrypto and will use unapproved implementation. |
And this is the side of the sword we want to be now. We currently aim to be as compatible as possible with upstream Go, which means falling back to Go crypto when necessary. Our stance is that if someone wants to be FIPS compliant then they should vet their code and ensure that only FIPS-approved algos are being used. There are code analysis tools that are good at doing this, such as CodeQL. This doesn't preclude that in the future we add a way to disable Go fallbacks, for example using |
There are more inconsistencies in the code and policy, so I want to clarify something:
Is this code buggy then? because it doesn't check if SHA1 is boring.SupportedHash() and thus if one builds 2030+ safe OpenSSL fips provider, this code will fail at runtime, whereas it should fallback to gocrypto? It just happens to be the case today that all OpenSSL fips providers have sha1, but this will not be true as we get closer to 2030. I guess my question is, should all current cases of "if bofing.Enabled {" in the code really, be "if boring.Enabled && boring.SupportsThisCodePathAtRuntime{" ? I and many others were under impression that today it is intentional that go-msft-fips toolchain blocks MD5 when boring is enabled. But if above bugs are fixed, suddenly MD5 will become enabled once again. |
Let be write some tests and collect the current set of behaviour as is today; before going to suggest any more fixes. |
Note that this toolchain has different behaviour against the Mariner 2.0 OpenSSL FIPS module, versus any other FIPS module. https://github.com/microsoft/azurelinux/blob/2.0/SPECS/openssl/openssl-1.1.1-fips.patch#L896 Specifically that appears to advertise support for MD5 and block it at runtime. If MD5 is completely removed from the FIPS module, like it typically is with 3.0.0+ fips modules, then suddenly this toolchain fallsback to gocrypto and runs, instead of getting a runtime failure. So i'm not sure above assement is correct. MD5 is blocked at runtime already (in the FIPS modules), and typically this toolchain is tricked into believing it will be available. Whereas, life can be made easier by neither advertising MD5 nor falling back to gocrypto. Also see #277 Thus removing SupportsHash() check in MD5 at least, will have no impact on existing Mariner builds, nor foreign FIPS modules, but will make this toolchain continue to have the same behaviour in the future when FIPS modules drop md5 altogether. |
I have alternative proposal, so will open a bug report and submit a new PR. |
In many instances systemcrypto backend is tested for support of a
particular algorithm. If it is supported, it is used, otherwise
gocrypto fallback code path is used. This means effectively the
toolchain allows to use MD5 DES TripleDES Ed25519 RC4 HKDF TLS1PF
implemented with gocrypto, when FIPS module blocks these algorithms or
doesn't even support them (i.e. when only base+fips providers are
loaded).
In some cases this might be due to incorrect check and/or incorrect
runtime configuration of OpenSSL. It is very common to accidentaly
activate "default" and "fips" providers in OpenSSL at the same time -
which then exhibits odd properties. Specifically "default+fips"
providers will list that RC4 and MD5 are supported without any
property query strings. But fail at runtime when attempted to be used
with property query string set to "fips=yes".
If on the other hand "base" and "fips" providers loaded alone, RC4 and
MD5 will not be listed as runtime available, and gocrypto fallback
path may be taken by the toolchain.
A similar issue is currently also present in cpython please see
python/cpython#118224.
Note that recommended way to configure OpenSSL in fips only mode is
with base+fips providers alone - see
https://github.com/openssl/openssl/blob/master/README-PROVIDERS.md
such that default & legacy providers algorithms are not exposed at
runtime. And this is how OpenSSL is configured in FIPS mode on Ubuntu
Pro FIPS and Chainguard FIPS Images, and recommended by upstream.
Please note internally md5 is used by go coverage, meaning in
requirefips case coverage may fail to generate unless some additional
APIs are introduced to allow insecure usage of md5 (equivalent to
python's usedforsecurity=True|False flag). Or coverage ported to use
SHA256.
For a local reproducer use base+fips providers only, for example with following openssl.cnf:
Or compile openssl without RC4 support by using 'no-rc4' configuration
option.
Signed-off-by: Dimitri John Ledkov [email protected]