Created attachment 16348 [details] truncated log of apt install samba-common-bin Forwarding https://bugs.debian.org/975882 When installing samba on Debian bullseye, the very concerning message "Weak crypto is allowed" gets printed when testparm checks the default smb.conf. This message does not include enough details for the person seeing it to understand where the weak crypto is coming from, what vulnerabilities in samba the weak crypto enables and what the consequences to interoperability etc are of disabling the weak crypto and how to disable the week crypto if desired. Probably the right way to do this would be to document the questions above on a wiki or web page or in the samba documentation and then add a link to that documentation from the testparm message.
This bug was referenced in samba master: 5c27740aeff273bcd5f027d36874e56170234146
Thanks for https://gitlab.com/samba-team/samba/-/commit/5c27740aeff273bcd5f027d36874e56170234146 > will fall back to these weak crypto algorithms if it is not possible > to use strong cryptography by default. What are the reasons of this fallback? Can we have more info?
Yeah, we need to enumerate the main cases that use 'weak' cryptography and so will fail if FIPS mode is enabled.
NTLM authentication is the main one, it uses RC4-MD5. Will look into it next week with Alexander.
I think that updating the testparm documentation isn't quite enough, since testparm could be run by things like the Debian package upgrade scripts and then the sysadmin could see the warning and wonder what it is about without, but have no idea where to go looking because the warning doesn't point that out. A simple "See DIAGNOSTICS in testparm(1) manual page." message (maybe just when weak crypto is allowed?) would make the documentation added much more accessible.
Yves-Alexis Perez <corsac@debian.org> send this message to the Debian bug: Since I had the same message I found this bug and the upstream bug, and dig a bit. And indeed, it seems totally unrelated to the samba configuration: https://sources.debian.org/src/samba/2:4.13.14+dfsg-1/source3/utils/testparm.c/#L763 https://sources.debian.org/src/samba/2:4.13.14+dfsg-1/lib/crypto/gnutls_weak_crypto.c/?hl=24#L24 testparm will report weak crypto as long as gnutls allows RC4 to be initialized (even if for example smb.conf disables NTLM auth or something). I have to admit I think this warning is poorly worded, even with the manpage patch.
How about this trivial patch? Hopefully it will make the matter much cleaner. diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c index 58ba46bc15f..4d419fd4805 100644 --- a/source3/utils/testparm.c +++ b/source3/utils/testparm.c @@ -875,7 +875,7 @@ static void do_per_share_checks(int s) } else { weak_crypo_str = "disallowed"; } - fprintf(stderr, "Weak crypto is %s\n", weak_crypo_str); + fprintf(stderr, "Weak crypto is %s by gnutls\n", weak_crypo_str); if (skip_logic_checks == 0) { ret = do_global_checks();
I totally agree. It should probably say something based on: GnuTLS <allows>/<denies> 'weak' cryptography due to system-wide cryptographic configuration (possibly FIPS mode) or the GNUTLS_FORCE_FIPS_MODE=0/1 environment variable. It should say as clearly as possible where the mode could come from etc. Nobody reads manpages, but it should be even more verbose there. See also https://lists.samba.org/archive/samba-technical/2022-March/137180.html Even the word 'weak' is loaded. RC4 is probably considered weak in 2020, but for most of the time since 2000 it was only weak in some particular arrangements, and not all our use cases were 'bad', some were well constructed with a confounder etc. A single word 'weak' doesn't help much, except to detect if the system allows RC4. In 2030 other algorithms we totally rely on will be 'weak', but we probably won't change this. Which brings us to the real purpose of this line, which is to allow this test to work: testprogs/blackbox/test_weak_crypto.sh I think a new option to testparm for the test, or a python hook (and so avoid testparm entirely) would be better, and then make the testparm side of things as clear as possible.
I still think that the patch to testparm needs to reference the documentation that was written about this issue in comment #2, which the proposed patch does not do. If the existing documentation about this isn't enough then it should definitely be updated.
Well, - fprintf(stderr, "Weak crypto is %s\n", weak_crypo_str); + fprintf(stderr, "Rquired for backwards compatibility weak crypto is %s by gnutls (man testparm, DIAGNOSTICS section)\n", weak_crypo_str); maybe? :)
Or maybe this one is better: diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c index 58ba46bc15f..80c1053a3f4 100644 --- a/source3/utils/testparm.c +++ b/source3/utils/testparm.c @@ -735,7 +733,6 @@ static void do_per_share_checks(int s) const char *caddr; static int show_defaults; static int skip_logic_checks = 0; - const char *weak_crypo_str = ""; bool ok; struct poptOption long_options[] = { @@ -870,12 +867,11 @@ static void do_per_share_checks(int s) fprintf(stderr,"Loaded services file OK.\n"); - if (samba_gnutls_weak_crypto_allowed()) { - weak_crypo_str = "allowed"; - } else { - weak_crypo_str = "disallowed"; + if (!samba_gnutls_weak_crypto_allowed()) { + fprintf(stderr, "Warning: weak crypto is required for backwards compatibility,\n" + "Warning: but is disallowed by gnutls library." + " See man testparm, DIAGNOSTICS section.\n"); } - fprintf(stderr, "Weak crypto is %s\n", weak_crypo_str); if (skip_logic_checks == 0) { ret = do_global_checks();
The suggestion in comment #11 looks fine for me, could you please open a Merge Request on Gitlab?
(In reply to Michael Tokarev from comment #10) dont forget to fix the typo here.. ;-) + fprintf(stderr, "Rquired for backwards compatibility weak crypto is + fprintf(stderr, "Required for backwards compatibility weak crypto is Rquired Required
https://gitlab.com/samba-team/samba/-/merge_requests/2537
Fixed in Samba 4.17 with commit b609734c52dc12cf80faa693e981a4ef0ce4be4a.