diff mbox series

[SMB3] Removing confusing error message by fixing buffer length checking in SMB3.11 negprot

Message ID CAH2r5mtcKoNSxyPNGD4i=5mKLpf1Y9R7Sr6E_eXgXqKcgmCyRg@mail.gmail.com
State New
Headers show
Series [SMB3] Removing confusing error message by fixing buffer length checking in SMB3.11 negprot | expand

Commit Message

Steve French April 8, 2018, 9:16 p.m. UTC 
SMB3: Fix length checking of SMB3.11 negotiate request

The length checking for SMB3.11 negotiate request includes
"negotiate contexts" which caused a buffer validation problem
and a confusing warning message on SMB3.11 mount e.g.:

     SMB2 server sent bad RFC1001 len 236 not 170

Fix the length checking for SMB3.11 negotiate to account for
the new negotiate context so that we don't log a warning on
SMB3.11 mount.

See attached

Comments

Aurélien Aptel April 10, 2018, 9:31 a.m. UTC | #1 
Steve French <smfrench@gmail.com> writes:
> SMB3: Fix length checking of SMB3.11 negotiate request
>
> The length checking for SMB3.11 negotiate request includes
> "negotiate contexts" which caused a buffer validation problem
> and a confusing warning message on SMB3.11 mount e.g.:
>
>      SMB2 server sent bad RFC1001 len 236 not 170
>
> Fix the length checking for SMB3.11 negotiate to account for
> the new negotiate context so that we don't log a warning on
> SMB3.11 mount.

code looks correct but I have a comment:

so the patch adds the check like this:

	clc_len = smb2_calc_size(hdr);

	if (shdr->Command == SMB2_NEGOTIATE)
		clc_len += get_neg_ctxt_len(hdr, len, clc_len);

but it might be cleaner to just fix the path that computes the size in
smb2_calc_size():

  smb2_calc_size()
    smb2_get_data_area()
      case SMB2_NEGOTIATE:
          *off = le16_to_cpu(
              ((struct smb2_negotiate_rsp *)hdr)->SecurityBufferOffset);
          *len = le16_to_cpu(
              ((struct smb2_negotiate_rsp *)hdr)->SecurityBufferLength);
          /*** PATCH HERE *****/
          break;

I think we could set off and len to the negctx off and len instead of the gss if
using 3.11.

you can do less debug checks than with your patch given the clc_len isnt
computed yet but I think it would be cleaner.

Cheers,
Steve French April 10, 2018, 12:59 p.m. UTC | #2 
We could move the check - but we would still have to check that the
other offset (security buffer offset) doesn't go beyond end of SMB.

It could save one line by moving this callout (the if command == SMB2_negotiate)

+#ifdef CONFIG_CIFS_SMB311
+    if (shdr->Command == SMB2_NEGOTIATE)
+        clc_len += get_neg_ctxt_len(hdr, len, clc_len);
+#endif /* SMB311 */

but not sure if it is any clearer.  Thoughts?

On Tue, Apr 10, 2018 at 4:31 AM, Aurélien Aptel <aaptel@suse.com> wrote:
> Steve French <smfrench@gmail.com> writes:
>> SMB3: Fix length checking of SMB3.11 negotiate request
>>
>> The length checking for SMB3.11 negotiate request includes
>> "negotiate contexts" which caused a buffer validation problem
>> and a confusing warning message on SMB3.11 mount e.g.:
>>
>>      SMB2 server sent bad RFC1001 len 236 not 170
>>
>> Fix the length checking for SMB3.11 negotiate to account for
>> the new negotiate context so that we don't log a warning on
>> SMB3.11 mount.
>
> code looks correct but I have a comment:
>
> so the patch adds the check like this:
>
>         clc_len = smb2_calc_size(hdr);
>
>         if (shdr->Command == SMB2_NEGOTIATE)
>                 clc_len += get_neg_ctxt_len(hdr, len, clc_len);
>
> but it might be cleaner to just fix the path that computes the size in
> smb2_calc_size():
>
>   smb2_calc_size()
>     smb2_get_data_area()
>       case SMB2_NEGOTIATE:
>           *off = le16_to_cpu(
>               ((struct smb2_negotiate_rsp *)hdr)->SecurityBufferOffset);
>           *len = le16_to_cpu(
>               ((struct smb2_negotiate_rsp *)hdr)->SecurityBufferLength);
>           /*** PATCH HERE *****/
>           break;
>
> I think we could set off and len to the negctx off and len instead of the gss if
> using 3.11.
>
> you can do less debug checks than with your patch given the clc_len isnt
> computed yet but I think it would be cleaner.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
Aurélien Aptel April 11, 2018, 9:01 a.m. UTC | #3 
Steve French <smfrench@gmail.com> writes:
> We could move the check - but we would still have to check that the
> other offset (security buffer offset) doesn't go beyond end of SMB.
>
> It could save one line by moving this callout (the if command == SMB2_negotiate)
>
> +#ifdef CONFIG_CIFS_SMB311
> +    if (shdr->Command == SMB2_NEGOTIATE)
> +        clc_len += get_neg_ctxt_len(hdr, len, clc_len);
> +#endif /* SMB311 */
>
> but not sure if it is any clearer.  Thoughts?

If you really want to do all the checks of get_neg_ctxt_len() then yes
there's probably no point in moving things around. My idea was to just
do return the right offsets/lenghts in case of 3.11 in
smb2_get_data_area(), no extra checks. There is some generic overlaping
and range checks in smb2_calc_size() using those results:

smb2_calc_size():
	smb2_get_data_area_len(&offset, &data_length, hdr);
                               ^^^^^^^^^^^^^^^^^^^^^ the offsets set earlier
	cifs_dbg(FYI, "SMB2 data length %d offset %d\n", data_length, offset);

	if (data_length > 0) {
		/*
		 * Check to make sure that data area begins after fixed area,
		 * Note that last byte of the fixed area is part of data area
		 * for some commands, typically those with odd StructureSize,
		 * so we must add one to the calculation (and 4 to account for
		 * the size of the RFC1001 hdr.
		 */
		if (offset + 4 + 1 < len) {
			cifs_dbg(VFS, "data area offset %d overlaps SMB2 header %d\n",
				 offset + 4 + 1, len);
			data_length = 0;
		} else {
			len = 4 + offset + data_length;
		}
	}


but it's not as exaustive as what you added in get_neg_ctxt_len().

So ultimately it's up to you :)

I'm ok with both. I guess more debug is always better so:

Reviewed-by: Aurelien Aptel <aaptel@suse.com>

Cheers,
Pavel Shilovsky April 12, 2018, 9:01 p.m. UTC | #4 
2018-04-08 14:16 GMT-07:00 Steve French <smfrench@gmail.com>:
> SMB3: Fix length checking of SMB3.11 negotiate request
>
> The length checking for SMB3.11 negotiate request includes
> "negotiate contexts" which caused a buffer validation problem
> and a confusing warning message on SMB3.11 mount e.g.:
>
>      SMB2 server sent bad RFC1001 len 236 not 170
>
> Fix the length checking for SMB3.11 negotiate to account for
> the new negotiate context so that we don't log a warning on
> SMB3.11 mount.
>
> See attached
> --
> Thanks,
>
> Steve


+#ifdef CONFIG_CIFS_SMB311
+static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len, __u32
non_ctxlen)
+{
+       __u16 neg_count;
+       __u32 nc_offset, size_of_pad_before_neg_ctxts;
+       struct smb2_negotiate_rsp *pneg_rsp = (struct smb2_negotiate_rsp *)hdr;
+
+       /* Negotiate contexts are only valid for latest dialect SMB3.11 */
+       neg_count = le16_to_cpu(pneg_rsp->NegotiateContextCount);
+       if ((neg_count == 0) ||
+          (pneg_rsp->DialectRevision != cpu_to_le16(SMB311_PROT_ID)))
+               return 0;
+
+       /* Make sure that negotiate contexts start after gss security blob */
+       nc_offset = le32_to_cpu(pneg_rsp->NegotiateContextOffset);
+       if (nc_offset < non_ctxlen - 4 /* RFC1001 len field */) {
                                                   ^^^
RFC header should be changed to server->vals->header_preamble_size.

+               printk_once(KERN_WARNING "invalid negotiate context offset\n");
+               return 0;
+       }
+       size_of_pad_before_neg_ctxts = nc_offset - (non_ctxlen - 4);

                               ^^^
the same as above.

Also, please drop stable@ tag. SMB3.11 is not experimental since the
current release, there is not so much value to backport this 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
Steve French April 12, 2018, 10:16 p.m. UTC | #5 
I am adding fix as you suggested for the +4/-4 to the
server->vals->header_preamble_size but didn't add cc: stable to that
followon patch since want to make sure everything 3.11 related (due to
the high importance for security) doesn't have dependencies on that

On Thu, Apr 12, 2018 at 4:01 PM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> 2018-04-08 14:16 GMT-07:00 Steve French <smfrench@gmail.com>:
>> SMB3: Fix length checking of SMB3.11 negotiate request
>>
>> The length checking for SMB3.11 negotiate request includes
>> "negotiate contexts" which caused a buffer validation problem
>> and a confusing warning message on SMB3.11 mount e.g.:
>>
>>      SMB2 server sent bad RFC1001 len 236 not 170
>>
>> Fix the length checking for SMB3.11 negotiate to account for
>> the new negotiate context so that we don't log a warning on
>> SMB3.11 mount.
>>
>> See attached
>> --
>> Thanks,
>>
>> Steve
>
>
> +#ifdef CONFIG_CIFS_SMB311
> +static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len, __u32
> non_ctxlen)
> +{
> +       __u16 neg_count;
> +       __u32 nc_offset, size_of_pad_before_neg_ctxts;
> +       struct smb2_negotiate_rsp *pneg_rsp = (struct smb2_negotiate_rsp *)hdr;
> +
> +       /* Negotiate contexts are only valid for latest dialect SMB3.11 */
> +       neg_count = le16_to_cpu(pneg_rsp->NegotiateContextCount);
> +       if ((neg_count == 0) ||
> +          (pneg_rsp->DialectRevision != cpu_to_le16(SMB311_PROT_ID)))
> +               return 0;
> +
> +       /* Make sure that negotiate contexts start after gss security blob */
> +       nc_offset = le32_to_cpu(pneg_rsp->NegotiateContextOffset);
> +       if (nc_offset < non_ctxlen - 4 /* RFC1001 len field */) {
>                                                    ^^^
> RFC header should be changed to server->vals->header_preamble_size.
>
> +               printk_once(KERN_WARNING "invalid negotiate context offset\n");
> +               return 0;
> +       }
> +       size_of_pad_before_neg_ctxts = nc_offset - (non_ctxlen - 4);
>
>                                ^^^
> the same as above.
>
> Also, please drop stable@ tag. SMB3.11 is not experimental since the
> current release, there is not so much value to backport this patch.
>
> --
> Best regards,
> Pavel Shilovsky
diff mbox series

Patch

From d2b1e074a47c206a058b0e648050d986285435d7 Mon Sep 17 00:00:00 2001
From: Steve French <smfrench@gmail.com>
Date: Sun, 8 Apr 2018 16:14:31 -0500
Subject: [PATCH] SMB3: Fix length checking of SMB3.11 negotiate request

The length checking for SMB3.11 negotiate request includes
"negotiate contexts" which caused a buffer validation problem
and a confusing warning message on SMB3.11 mount e.g.:

     SMB2 server sent bad RFC1001 len 236 not 170

Fix the length checking for SMB3.11 negotiate to account for
the new negotiate context so that we don't log a warning on
SMB3.11 mount.

CC: Stable <stable@vger.kernel.org>
Signed-off-by: Steve French <smfrench@gmail.com>
---
 fs/cifs/smb2misc.c | 39 +++++++++++++++++++++++++++++++++++++++
 fs/cifs/smb2pdu.h  |  7 +++++++
 2 files changed, 46 insertions(+)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 5406e95f5d92..9df9f0b48160 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -93,6 +93,41 @@  static const __le16 smb2_rsp_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = {
 	/* SMB2_OPLOCK_BREAK */ cpu_to_le16(24)
 };
 
+#ifdef CONFIG_CIFS_SMB311
+static __u32 get_neg_ctxt_len(struct smb2_hdr *hdr, __u32 len, __u32 non_ctxlen)
+{
+	__u16 neg_count;
+	__u32 nc_offset, size_of_pad_before_neg_ctxts;
+	struct smb2_negotiate_rsp *pneg_rsp = (struct smb2_negotiate_rsp *)hdr;
+
+	/* Negotiate contexts are only valid for latest dialect SMB3.11 */
+	neg_count = le16_to_cpu(pneg_rsp->NegotiateContextCount);
+	if ((neg_count == 0) ||
+	   (pneg_rsp->DialectRevision != cpu_to_le16(SMB311_PROT_ID)))
+		return 0;
+
+	/* Make sure that negotiate contexts start after gss security blob */
+	nc_offset = le32_to_cpu(pneg_rsp->NegotiateContextOffset);
+	if (nc_offset < non_ctxlen - 4 /* RFC1001 len field */) {
+		printk_once(KERN_WARNING "invalid negotiate context offset\n");
+		return 0;
+	}
+	size_of_pad_before_neg_ctxts = nc_offset - (non_ctxlen - 4);
+
+	/* Verify that at least minimal negotiate contexts fit within frame */
+	if (len < nc_offset + (neg_count * sizeof(struct smb2_neg_context))) {
+		printk_once(KERN_WARNING "negotiate context goes beyond end\n");
+		return 0;
+	}
+
+	cifs_dbg(FYI, "length of negcontexts %d pad %d\n",
+		len - nc_offset, size_of_pad_before_neg_ctxts);
+
+	/* length of negcontexts including pad from end of sec blob to them */
+	return (len - nc_offset) + size_of_pad_before_neg_ctxts;
+}
+#endif /* CIFS_SMB311 */
+
 int
 smb2_check_message(char *buf, unsigned int length, struct TCP_Server_Info *srvr)
 {
@@ -198,6 +233,10 @@  smb2_check_message(char *buf, unsigned int length, struct TCP_Server_Info *srvr)
 
 	clc_len = smb2_calc_size(hdr);
 
+#ifdef CONFIG_CIFS_SMB311
+	if (shdr->Command == SMB2_NEGOTIATE)
+		clc_len += get_neg_ctxt_len(hdr, len, clc_len);
+#endif /* SMB311 */
 	if (srvr->vals->header_preamble_size + len != clc_len) {
 		cifs_dbg(FYI, "Calculated size %u length %zu mismatch mid %llu\n",
 			 clc_len, srvr->vals->header_preamble_size + len, mid);
diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
index 253e2c7c952f..0e0a0af89e4d 100644
--- a/fs/cifs/smb2pdu.h
+++ b/fs/cifs/smb2pdu.h
@@ -263,6 +263,13 @@  struct smb2_negotiate_req {
 #define SMB2_NT_FIND			0x00100000
 #define SMB2_LARGE_FILES		0x00200000
 
+struct smb2_neg_context {
+	__le16	ContextType;
+	__le16	DataLength;
+	__le32	Reserved;
+	/* Followed by array of data */
+} __packed;
+
 #define SMB311_SALT_SIZE			32
 /* Hash Algorithm Types */
 #define SMB2_PREAUTH_INTEGRITY_SHA512	cpu_to_le16(0x0001)
-- 
2.14.1