Bug 14583 - testparm: explain what "Weak crypto is allowed" means
Summary: testparm: explain what "Weak crypto is allowed" means
Status: REOPENED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.13.2
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andreas Schneider
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-27 01:52 UTC by Paul Wise
Modified: 2022-04-01 04:31 UTC (History)
3 users (show)

See Also:


Attachments
truncated log of apt install samba-common-bin (260 bytes, text/plain)
2020-11-27 01:52 UTC, Paul Wise
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Wise 2020-11-27 01:52:38 UTC
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.
Comment 1 Samba QA Contact 2020-11-27 13:49:08 UTC
This bug was referenced in samba master:

5c27740aeff273bcd5f027d36874e56170234146
Comment 2 Mathieu Parent 2020-11-27 14:19:51 UTC
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?
Comment 3 Andrew Bartlett 2020-11-27 20:39:04 UTC
Yeah, we need to enumerate the main cases that use 'weak' cryptography and so will fail if FIPS mode is enabled.
Comment 4 Andreas Schneider 2020-11-28 19:06:34 UTC
NTLM authentication is the main one, it uses RC4-MD5. Will look into it next week with Alexander.
Comment 5 Paul Wise 2021-02-26 01:50:51 UTC
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.
Comment 6 Paul Wise 2021-11-11 09:28:47 UTC
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.
Comment 7 Michael Tokarev 2022-03-31 19:03:04 UTC
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();
Comment 8 Andrew Bartlett 2022-03-31 19:16:02 UTC
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.
Comment 9 Paul Wise 2022-04-01 04:31:10 UTC
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.