| Message ID | CAKywueRN3qOPHPxpbQhCfM+Bhrg2UBw+3Pjh0KP+2tWUZsZtbg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Yes - Pavel is correct that patch of his fixes the oops (in my testing at least) for 4.9.87 stable that Srivatsa mentioned. On Mon, Mar 19, 2018 at 5:17 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote: > 2018-03-14 23:39 GMT-07:00 Srivatsa S. Bhat <srivatsa@csail.mit.edu>: >> 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 <srivatsa@csail.mit.edu> 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 <smfrench@gmail.com> >>>>>>> >>>>>>> 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 <lsahlber.redhat.com> >>>>>>> Signed-off-by: Steve French <smfrench@gmail.com> >>>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>>>> >>>>>>> --- >>>>>>> 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 //<ip_addr>/TestSMB/ testdir >>>> >>>> [ 53.073057] BUG: unable to handle kernel NULL pointer dereference at 0000000000000050 >>>> [ 53.073511] IP: [<ffffffff8138ee9a>] 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:[<ffffffff8138ee9a>] [<ffffffff8138ee9a>] 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] [<ffffffffa02194a6>] smb3_calc_signature+0xb6/0x290 [cifs] >>>> [ 53.091650] [<ffffffffa0218b5b>] smb2_sign_rqst+0x2b/0x40 [cifs] >>> <snip> >>>> 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: > > --- 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; > > 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
2018-03-21 18:25 GMT-07:00 Steve French <smfrench@gmail.com>: > Yes - Pavel is correct that patch of his fixes the oops (in my testing > at least) for 4.9.87 stable that Srivatsa mentioned. Great. So, we need to apply "CIFS: Enable encryption during session setup phase" (commit id cabfb3680f78981d26c078a26e5c748531257ebb) as a dependency for Srivatsa's original backport of the topic patch. -- 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;