linux-kernel - Re: [PATCH 0/8] smb: client: More crypto library conversions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251014060737.GD2763@sol>
Date: Mon, 13 Oct 2025 23:07:37 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Enzo Matsumiya <ematsumiya@...e.de>
Cc: linux-cifs@...r.kernel.org, Steve French <sfrench@...ba.org>,
	samba-technical@...ts.samba.org, linux-crypto@...r.kernel.org,
	linux-kernel@...r.kernel.org, Paulo Alcantara <pc@...guebit.org>,
	Ronnie Sahlberg <ronniesahlberg@...il.com>,
	Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>,
	Bharath SM <bharathsm@...rosoft.com>
Subject: Re: [PATCH 0/8] smb: client: More crypto library conversions

On Mon, Oct 13, 2025 at 11:44:37AM -0300, Enzo Matsumiya wrote:
> Hi Eric,
> 
> On 10/11, Eric Biggers wrote:
> > This series converts fs/smb/client/ to access SHA-512, HMAC-SHA256, MD5,
> > and HMAC-MD5 using the library APIs instead of crypto_shash.
> > 
> > This simplifies the code significantly.  It also slightly improves
> > performance, as it eliminates unnecessary overhead.
> > 
> > Tested with Samba with all SMB versions, with mfsymlinks in the mount
> > options, 'server min protocol = NT1' and 'server signing = required' in
> > smb.conf, and doing a simple file data and symlink verification test.
> > That seems to cover all the modified code paths.
> > 
> > However, with SMB 1.0 I get "CIFS: VFS: SMB signature verification
> > returned error = -13", regardless of whether this series is applied or
> > not.  Presumably, testing that case requires some other setting I
> > couldn't find.
> > 
> > Regardless, these are straightforward conversions and all the actual
> > crypto is exactly the same as before, as far as I can tell.
> 
> I think the overall series looks good and do a great cleanup.
> 
> Just a minor nit about fips_enabled: since it's now being handled
> explicitly (rather than an error on cifs_alloc_hash() currently), I
> think it makes sense to move the check to mount code path when
> 'sectype == NTLMv2' (I don't particularly care about SMB1, but
> something similar can be done for 'smb1 && sign' cases I guess).

For MD5 message signing and NTLMv2, this series keeps the fips_enabled
checks where they were before.  That is, they're inserted where the
calls to cifs_alloc_hash() were before.  I think moving them to earlier
locations is best done in later patches, as it's not obvious (at least
to me) exactly how to do that.

I spent a while reading the code again, and this is what I came up with:

- For MD5 message signing: cifs_enable_signing() is probably the right
  place to disallow it.  However, it's called regardless of the signing
  algorithm.  So it needs a parameter passed in that tells it that the
  signing algorithm will be MD5 (or equivalently the dialect is SMB1).

- For NTLMv2: select_sec() and SMB2_select_sec() are where the
  authentication protocol is selected and are probably the right places
  to disallow NTLMv2 and RawNTLMSSP.  However, those are two places, not
  one.  We'd have to remember to put the fips_enabled check in both.

The nice thing about keeping the fips_enabled checks next to the actual
uses of the algorithms is that it ensures they stay in sync.  So maybe
we should just stay with that here.

- Eric