[4.13,28/43] SMB3: Validate negotiate request must always be signed
diff mbox

Message ID CAKywueRN3qOPHPxpbQhCfM+Bhrg2UBw+3Pjh0KP+2tWUZsZtbg@mail.gmail.com
State New
Headers show

Commit Message

Pavel Shilovsky March 19, 2018, 10:17 p.m. UTC
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:


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

Comments

Steve French March 22, 2018, 1:25 a.m. UTC | #1
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
Pavel Shilovsky March 22, 2018, 1:39 a.m. UTC | #2
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

Patch
diff mbox

--- 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;