Bug 13092 - potential use of uninitialised memory in smb3_validate_negotiate()
potential use of uninitialised memory in smb3_validate_negotiate()
Status: RESOLVED FIXED
Product: CifsVFS
Classification: Unclassified
Component: kernel fs
3.x
All All
: P5 normal
: ---
Assigned To: David Disseldorp
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-18 22:29 UTC by David Disseldorp
Modified: 2017-11-06 10:28 UTC (History)
5 users (show)

See Also:


Attachments
patch to have Samba send an undersize validate-neg-info response (792 bytes, patch)
2017-10-18 22:34 UTC, David Disseldorp
no flags Details
patches to fix validate negotiate info response memory leak and buffer overrun (2.72 KB, patch)
2017-10-18 23:19 UTC, David Disseldorp
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2017-10-18 22:29:56 UTC
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...
Comment 3 David Disseldorp 2017-10-18 23:19:15 UTC
Created attachment 13705 [details]
patches to fix validate negotiate info response memory leak and buffer overrun
Comment 4 David Disseldorp 2017-10-20 11:09:12 UTC
(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).
Comment 5 Jeff Layton 2017-10-20 12:14:43 UTC
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.
Comment 6 David Disseldorp 2017-11-06 10:28:00 UTC
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>