From patchwork Mon Mar 19 22:17:03 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pavel Shilovsky X-Patchwork-Id: 10295571 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 8C035600F6 for ; Mon, 19 Mar 2018 22:17:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6FA9229506 for ; Mon, 19 Mar 2018 22:17:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 645862950A; Mon, 19 Mar 2018 22:17:20 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 15B7C29506 for ; Mon, 19 Mar 2018 22:17:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934012AbeCSWRP (ORCPT ); Mon, 19 Mar 2018 18:17:15 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:38536 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936368AbeCSWRG (ORCPT ); Mon, 19 Mar 2018 18:17:06 -0400 Received: by mail-lf0-f68.google.com with SMTP id y2-v6so25669054lfc.5 for ; Mon, 19 Mar 2018 15:17:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=BJjBqgxoq6Lc5lJCWLMJt8lZkcB5sEFR1Y+i6TySlTA=; b=b+rpiF0E4n+1T/88ptMKMTCpiWrLlwHzBMH7ypUu3I3s9brWAsGx0Hy2EVMrOEdk4o UzfXe4A8n6+rTzV2IbdO8c7a2ywzYBo/8z1aGrvdkAy4PqR58t8kFbMVhwL9VGJdxkBc NIQbkuxDRVuOQP0xAZLqheA0ep7AVQtw7WhEiRdF3Bbs/r4dHgI7iWuw4H/Vsox9iYs/ KpdBoAXvWImyfQknGCIQPCX8fh2RBmEJ/ZU5/VRN+QiOyxD3/Glj7ONDw8jCS3H3Vz6m XytomTHxAqmdDuGQrEggjL5MnyBPitjUxQP45W4+QHFcD/hkm1bZlhZY+0/6hQ4EZSxG oVVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=BJjBqgxoq6Lc5lJCWLMJt8lZkcB5sEFR1Y+i6TySlTA=; b=Oqnw1L6GwxpOsLzUjRBdwBUME0Xp29VU8W8RNWuK3DdzM/vRS3DojcXn2ocsi02kX1 BwCMlYHOqmdqlftU7M44XCkCO5YGTSwj59qjFI8j/bHNQpiQsSEEIaCb1lOQFEt5nkJH m9+KVmSkwfLlkDVxSSRa1je39II6ZhbQhEzM/GGMqXhNmIUut1FS6v+3qjU8llUuGFvB wjsbtMeWyfT2QU/Df07M8H21EVQhvb1pc8Bs45z8FP87UeqlVS5+6XJQ2L9cLP/Ue386 3dGqi4DiBtccxC9TSwfClln1QwDnHbcqAo7VCDwlj0JgLidw33VepWOXkwrQ9iEGK39M twzw== X-Gm-Message-State: AElRT7E5LWZg/qt8pVcavac+iuazRSFlMHKHS5L3ErBZYdimtL8Iyd4x Sz2dN0JNphIgt0105qMGOtKDL1ICzQRCRiYYAg== X-Google-Smtp-Source: AG47ELuy0+SRlRDRSlSDs1Y+ft5QCF0kpl83ERenpIkIM3KW0qUxXE6MMGxwKHElyU1A+5BagA01bBjYHObNBfHu6ms= X-Received: by 2002:a19:e819:: with SMTP id f25-v6mr5357035lfh.62.1521497824697; Mon, 19 Mar 2018 15:17:04 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a19:280a:0:0:0:0:0 with HTTP; Mon, 19 Mar 2018 15:17:03 -0700 (PDT) In-Reply-To: <8b65044e-a0aa-8216-7a02-40298f5cc42c@csail.mit.edu> References: <20171031095530.520746935@linuxfoundation.org> <20171031095531.633196173@linuxfoundation.org> <97340c9a-0ea2-0d3d-cf26-58c799d76cae@mageia.org> <20171101151803.GB31285@kroah.com> <4ba67095-4075-688f-d3fb-157847aee4d9@csail.mit.edu> <8b65044e-a0aa-8216-7a02-40298f5cc42c@csail.mit.edu> From: Pavel Shilovsky Date: Mon, 19 Mar 2018 15:17:03 -0700 Message-ID: Subject: Re: [PATCH 4.13 28/43] SMB3: Validate negotiate request must always be signed To: "Srivatsa S. Bhat" Cc: Steve French , CIFS , =?UTF-8?Q?Aur=C3=A9lien_Aptel?= , Pavel Shilovskiy Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 2018-03-14 23:39 GMT-07:00 Srivatsa S. Bhat : > On 3/13/18 11:51 PM, Steve French wrote: >> Srivatsa - >> I dug up an earlier note of yours - and yes you are correct, I verified that >> >> crypto_shash_setkey is called with cmacaes set to NULL >> >> >> You did good work isolating this - I had missed one of your important >> earlier emails. > > Thank you! > >> I tried a similar approach - but calling >> >> server->ops->generate_signingkey(ses) >> >> later (before crypto_shash_setkey) which didn't work. So more than >> one thing missing. >> > > Agreed... And unfortunately mainline has diverged so much from stable > around this code (with lots of code re-organization etc) that it seems > rather hard to isolate the rest of the fix for this issue. > >> Aurelien/Pavel, >> Ideas? >> > > > Regards, > Srivatsa > >> >> On Wed, Jan 3, 2018 at 6:15 PM, Srivatsa S. Bhat wrote: >>> On 11/1/17 8:18 AM, Greg Kroah-Hartman wrote: >>>> On Tue, Oct 31, 2017 at 03:02:11PM +0200, Thomas Backlund wrote: >>>>> Den 31.10.2017 kl. 11:55, skrev Greg Kroah-Hartman: >>>>>> 4.13-stable review patch. If anyone has any objections, please let me know. >>>>>> >>>>>> ------------------ >>>>>> >>>>>> From: Steve French >>>>>> >>>>>> commit 4587eee04e2ac7ac3ac9fa2bc164fb6e548f99cd upstream. >>>>>> >>>>>> According to MS-SMB2 3.2.55 validate_negotiate request must >>>>>> always be signed. Some Windows can fail the request if you send it unsigned >>>>>> >>>>>> See kernel bugzilla bug 197311 >>>>>> >>>>>> Acked-by: Ronnie Sahlberg >>>>>> Signed-off-by: Steve French >>>>>> Signed-off-by: Greg Kroah-Hartman >>>>>> >>>>>> --- >>>>>> fs/cifs/smb2pdu.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> --- a/fs/cifs/smb2pdu.c >>>>>> +++ b/fs/cifs/smb2pdu.c >>>>>> @@ -1963,6 +1963,9 @@ SMB2_ioctl(const unsigned int xid, struc >>>>>> } else >>>>>> iov[0].iov_len = get_rfc1002_length(req) + 4; >>>>>> + /* validate negotiate request must be signed - see MS-SMB2 3.2.5.5 */ >>>>>> + if (opcode == FSCTL_VALIDATE_NEGOTIATE_INFO) >>>>>> + req->hdr.sync_hdr.Flags |= SMB2_FLAGS_SIGNED; >>>>>> rc = SendReceive2(xid, ses, iov, n_iov, &resp_buftype, flags, &rsp_iov); >>>>>> cifs_small_buf_release(req); >>>>>> >>>>>> >>>>>> >>>>> >>>>> This one needs to be backported to all stable kernels as the commit that >>>>> introduced the regression: >>>>> ' >>>>> 0603c96f3af50e2f9299fa410c224ab1d465e0f9 >>>>> SMB: Validate negotiate (to protect against downgrade) even if signing off >>>>> >>>>> is backported in stable trees as of: 4.9.53, 4.4.90, 3.18.73 >>>> >>>> Oh wait, it breaks the builds on older kernels, that's why I didn't >>>> apply it :) >>>> >>>> Can you provide me with a working backport? >>>> >>> >>> Hi Steve, >>> >>> Is there a version of this fix available for stable kernels? >>> >>> I tried applying this patch to 4.4.109 (and a similar one for 4.9.74), >>> but it didn't fix the problem. Instead, I actually got a NULL pointer >>> dereference when I tried to mount an SMB3 share. >>> >>> Here is the patch I tried on 4.4.109: >>> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>> index f2ff60e..3963bd2 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -1559,6 +1559,9 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, >>> } else >>> iov[0].iov_len = get_rfc1002_length(req) + 4; >>> >>> + /* validate negotiate request must be signed - see MS-SMB2 3.2.5.5 */ >>> + if (opcode == FSCTL_VALIDATE_NEGOTIATE_INFO) >>> + req->hdr.Flags |= SMB2_FLAGS_SIGNED; >>> >>> rc = SendReceive2(xid, ses, iov, num_iovecs, &resp_buftype, 0); >>> rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base; >>> >>> >>> This results in the following NULL pointer dereference when I try >>> mounting: >>> >>> # mount -vvv -t cifs -o vers=3.0,credentials=.smbcred ///TestSMB/ testdir >>> >>> [ 53.073057] BUG: unable to handle kernel NULL pointer dereference at 0000000000000050 >>> [ 53.073511] IP: [] crypto_shash_setkey+0x1a/0xc0 >>> [ 53.073973] PGD 0 >>> [ 53.074427] Oops: 0000 [#1] SMP >>> [ 53.074946] Modules linked in: arc4(E) ecb(E) md4(E) cifs(E) dns_resolver(E) vmw_vsock_vmci_transport(E) vsock(E) hid_generic(E) usbhid(E) hid(E) xt_conntrack(E) mousedev(E) iptable_nat(E) nf_conntrack_ipv4(E) nf_defrag_ipv4(E) nf_nat_ipv4(E) nf_nat(E) iptable_filter(E) ip_tables(E) crc32c_intel(E) xt_LOG(E) nf_conntrack(E) jitterentropy_rng(E) hmac(E) sha256_ssse3(E) sha256_generic(E) drbg(E) vmw_balloon(E) ansi_cprng(E) aesni_intel(E) aes_x86_64(E) glue_helper(E) lrw(E) gf128mul(E) ablk_helper(E) cryptd(E) psmouse(E) evdev(E) uhci_hcd(E) ehci_pci(E) ehci_hcd(E) usbcore(E) intel_agp(E) usb_common(E) vmw_vmci(E) i2c_piix4(E) intel_gtt(E) nfit(E) battery(E) tpm_tis(E) tpm(E) ac(E) button(E) sch_fq_codel(E) autofs4(E) >>> [ 53.079435] CPU: 3 PID: 829 Comm: mount.cifs Tainted: G E 4.4.109-possible-fix1+ #21 >>> [ 53.079983] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 04/05/2016 >>> [ 53.081086] task: ffff8800b4f41940 ti: ffff8800b92ac000 task.ti: ffff8800b92ac000 >>> [ 53.081667] RIP: 0010:[] [] crypto_shash_setkey+0x1a/0xc0 >>> [ 53.082247] RSP: 0018:ffff8800b92af9a8 EFLAGS: 00010282 >>> [ 53.082604] systemd-journald[284]: Compressed data object 721 -> 468 using XZ >>> [ 53.083419] RAX: ffff8800af5943c0 RBX: ffff8800b484a800 RCX: 00000000ffff0ec7 >>> [ 53.084001] RDX: 0000000000000010 RSI: ffff8800b900af18 RDI: 0000000000000000 >>> [ 53.084602] RBP: ffff8800b92af9e0 R08: ffff8800b92afb64 R09: 0000000000000000 >>> [ 53.085184] R10: 3031322e3030312e R11: 00000000000007f5 R12: 0000000000000002 >>> [ 53.085755] R13: 0000000000000000 R14: ffff8800b900af18 R15: 0000000000000010 >>> [ 53.086333] FS: 00007fb659b45740(0000) GS:ffff88013fcc0000(0000) knlGS:0000000000000000 >>> [ 53.086907] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> [ 53.087480] CR2: 0000000000000050 CR3: 00000000b7970000 CR4: 00000000001606e0 >>> [ 53.088107] Stack: >>> [ 53.088681] ffff8800bba5d8c0 ffff8800b92afa08 ffff8800b484a800 0000000000000002 >>> [ 53.089281] ffff8800b92afac8 000008012400007d ffff8800b484a800 ffff8800b92afa50 >>> [ 53.089871] ffffffffa02194a6 ffff8800b92afb70 ffff8800af5943c0 ffff8800b7a2f800 >>> [ 53.090457] Call Trace: >>> [ 53.091054] [] smb3_calc_signature+0xb6/0x290 [cifs] >>> [ 53.091650] [] smb2_sign_rqst+0x2b/0x40 [cifs] >> >>> The problem seems to be that crypto_shash_setkey() is called without >>> calling smb3_crypto_shash_allocate() first. So I looked up how mainline >>> avoids this issue, and it looks like the following commit makes a call >>> to generate_signingkey() even when server->sign == false, and this path >>> eventually calls smb3_crytpto_shash_allocate()), thus avoiding the NULL >>> pointer dereference. >>> >>> cabfb3680f78 (CIFS: Enable encryption during session setup phase) >>> >>> >>> So, I adopted this change, and now my resulting patch looks like this: >>> >>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>> index f2ff60e..19cc92c 100644 >>> --- a/fs/cifs/smb2pdu.c >>> +++ b/fs/cifs/smb2pdu.c >>> @@ -833,7 +833,7 @@ ssetup_exit: >>> >>> if (!rc) { >>> mutex_lock(&server->srv_mutex); >>> - if (server->sign && server->ops->generate_signingkey) { >>> + if (server->ops->generate_signingkey) { >>> rc = server->ops->generate_signingkey(ses); >>> kfree(ses->auth_key.response); >>> ses->auth_key.response = NULL; >>> @@ -1559,6 +1559,9 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, >>> } else >>> iov[0].iov_len = get_rfc1002_length(req) + 4; >>> >>> + /* validate negotiate request must be signed - see MS-SMB2 3.2.5.5 */ >>> + if (opcode == FSCTL_VALIDATE_NEGOTIATE_INFO) >>> + req->hdr.Flags |= SMB2_FLAGS_SIGNED; >>> >>> rc = SendReceive2(xid, ses, iov, num_iovecs, &resp_buftype, 0); >>> rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base; >>> Hi Steve, Srivatsa, Have you tried to amend the rest of the patch (CIFS: Enable encryption during session setup phase) ? https://git.samba.org/?p=sfrench/cifs-2.6.git;a=commitdiff;h=cabfb3680f78981d26c078a26e5c748531257ebb The remaining portion has the important piece: NTLMSSP_NEGOTIATE_KEY_XCH to instruct the server to exchange keys sec_blob->NegotiateFlags = cpu_to_le32(flags); @@ -407,13 +406,12 @@ int build_ntlmssp_auth_blob(unsigned char **pbuffer, flags = NTLMSSP_NEGOTIATE_56 | NTLMSSP_REQUEST_TARGET | NTLMSSP_NEGOTIATE_TARGET_INFO | NTLMSSP_NEGOTIATE_128 | NTLMSSP_NEGOTIATE_UNICODE | - NTLMSSP_NEGOTIATE_NTLM | NTLMSSP_NEGOTIATE_EXTENDED_SEC; - if (ses->server->sign) { + NTLMSSP_NEGOTIATE_NTLM | NTLMSSP_NEGOTIATE_EXTENDED_SEC | + NTLMSSP_NEGOTIATE_SEAL; ^^^ the same as above. + if (ses->server->sign) flags |= NTLMSSP_NEGOTIATE_SIGN; - if (!ses->server->session_estab || - ses->ntlmssp->sesskey_per_smbsess) - flags |= NTLMSSP_NEGOTIATE_KEY_XCH; - } + if (!ses->server->session_estab || ses->ntlmssp->sesskey_per_smbsess) + flags |= NTLMSSP_NEGOTIATE_KEY_XCH; the same. tmp = *pbuffer + sizeof(AUTHENTICATE_MESSAGE); sec_blob->NegotiateFlags = cpu_to_le32(flags); According to the MS-NLMP (https://msdn.microsoft.com/en-us/library/cc236692.aspx), the server will generate the exchange the key in presence of any flag from two: NTLMSSP_NEGOTIATE_SEAL or NTLMSSP_NEGOTIATE_SIGN. Basically, the proper fix would be to add NTLMSSP_NEGOTIATE_SIGN flag unconditionally if we allow signed requests on mounts without signing enabled by the client. --- Best regards, Pavel Shilovsky -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -344,13 +344,12 @@ void build_ntlmssp_negotiate_blob(unsigned char *pbuffer, /* BB is NTLMV2 session security format easier to use here? */ flags = NTLMSSP_NEGOTIATE_56 | NTLMSSP_REQUEST_TARGET | NTLMSSP_NEGOTIATE_128 | NTLMSSP_NEGOTIATE_UNICODE | - NTLMSSP_NEGOTIATE_NTLM | NTLMSSP_NEGOTIATE_EXTENDED_SEC; - if (ses->server->sign) { + NTLMSSP_NEGOTIATE_NTLM | NTLMSSP_NEGOTIATE_EXTENDED_SEC | + NTLMSSP_NEGOTIATE_SEAL; ^^^ we are adding NTLMSSP_NEGOTIATE_SEAL and... + if (ses->server->sign) flags |= NTLMSSP_NEGOTIATE_SIGN; - if (!ses->server->session_estab || - ses->ntlmssp->sesskey_per_smbsess) - flags |= NTLMSSP_NEGOTIATE_KEY_XCH; - } + if (!ses->server->session_estab || ses->ntlmssp->sesskey_per_smbsess) + flags |= NTLMSSP_NEGOTIATE_KEY_XCH;