While looking at a memory leak in cifs.ko smb3_validate_negotiate(), I came across the following worrying code: 725 if (rsplen != sizeof(struct validate_negotiate_info_rsp)) { 726 cifs_dbg(VFS, "invalid protocol negotiate response size: %d\n", 727 rsplen); 728 729 /* relax check since Mac returns max bufsize allowed on ioctl */ 730 if (rsplen > CIFSMaxBufSize) 731 return -EIO; 732 } Commit 7db0a6efdc3e ("SMB3: Work around mount failure when using SMB3 dialect to Macs"), means that we now permit responses which don't match sizeof(struct validate_negotiate_info_rsp), as long as they're under CIFSMaxBufSize - There's no check for undersize responses. An undersize response results in comparisons (only) against uninitialized memory corresponding to the Dialect, SecurityMode and/or Capabilities members of struct validate_negotiate_info_rsp. E.g. 733 734 /* check validate negotiate info response matches what we got earlier */ 735 if (pneg_rsp->Dialect != 736 cpu_to_le16(tcon->ses->server->vals->protocol_id)) 737 goto vneg_out; My main question here is whether this bug has security implications - if I understand the purpose of validate negotiate info correctly, it's normally issued by the client immediately following SMB2 Negotiate, to ensure that dialect negotiation wasn't downgraded by a MITM attacker. That would mean that the session is authenticated, and that packet signing would already be active, in which case I don't think it's worth a CVE here. What do others think? Patches to follow...
Created attachment 13705 [details] patches to fix validate negotiate info response memory leak and buffer overrun
(In reply to David Disseldorp from comment #3) > Created attachment 13705 [details] > patches to fix validate negotiate info response memory leak and buffer > overrun FWIW, since raising this, Shu Wang has sent a similar fix for the memory leak (only) to the public ML (SMB: fix memory leak in smb3_validate_negotiate).
Patches look sane. The potentially uninitialized fields are not saved off and used anywhere. We just compare them to what we have and exit this function with EIO if they're not the same. So I don't see this as real security risk. Worst case, an attacker might cause the validate to succeed or fail when it shouldn't? It looks like the whole SMB has to be signed anyway so it seems like it would be hard to spoof this response or take advantage of one that was shortened like this.
Closing - fix is in mainline as: commit a2d9daad1d2dfbd307ab158044d1c323d7babbde Author: David Disseldorp <ddiss@suse.de> Date: Fri Oct 20 14:49:38 2017 +0200 SMB: fix validate negotiate info uninitialised memory use An undersize validate negotiate info server response causes the client to use uninitialised memory for struct validate_negotiate_info_rsp comparisons of Dialect, SecurityMode and/or Capabilities members. Link: https://bugzilla.samba.org/show_bug.cgi?id=13092 Fixes: 7db0a6efdc3e ("SMB3: Work around mount failure when using SMB3 dialect to Macs") Signed-off-by: David Disseldorp <ddiss@suse.de> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com> Signed-off-by: Steve French <smfrench@gmail.com>