[PATCH] cifs: fix length checks in checkSMB

[PATCH] cifs: fix length checks in checkSMB

[Jeff Layton]

The cERROR message in checkSMB when the calculated length doesn't match
the RFC1001 length is incorrect in many cases. It always says that the
RFC1001 length is bigger than the SMB, even when it's actually the
reverse.

Fix the error message to say the reverse of what it does now and remove
the arbitrary check when an RFC1001 length is larger than the SMB.
There's no reason to reject those packets since we can just ignore the
junk that's hanging off the end.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/misc.c |   19 +++----------------
 1 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 72e99ec..959d629 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -467,23 +467,10 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
 			if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
 				return 0; /* bcc wrapped */
 		}
-		cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
+		cFYI(1, "Calculated size %u vs length %u mismatch for mid=%u",
 				clc_len, 4 + len, smb->Mid);
-		/* Windows XP can return a few bytes too much, presumably
-		an illegal pad, at the end of byte range lock responses
-		so we allow for that three byte pad, as long as actual
-		received length is as long or longer than calculated length */
-		/* We have now had to extend this more, since there is a
-		case in which it needs to be bigger still to handle a
-		malformed response to transact2 findfirst from WinXP when
-		access denied is returned and thus bcc and wct are zero
-		but server says length is 0x21 bytes too long as if the server
-		forget to reset the smb rfc1001 length when it reset the
-		wct and bcc to minimum size and drop the t2 parms and data */
-		if ((4+len > clc_len) && (len <= clc_len + 512))
-			return 0;
-		else {
-			cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
+		if (4+len < clc_len) {
+			cERROR(1, "RFC1001 size %d smaller than SMB for mid=%u",
 					len, smb->Mid);
 			return 1;
 		}
-- 
1.7.3.4

Re: [PATCH] cifs: fix length checks in checkSMB

[Steve French]

If a server has a bug and sends an SMB which is more than just "round up" off
in length - I can imagine a case for making exceptions depending on the
details of the type of malformed frame, but since we
already allow 512 bytes (for "incorrect server rounding of frame size" bugs)
we should be careful.  If the length is more than 512 bytes off - this is
a server bug and the probability that the rest of the frame is incorrect
is very high.  How off is the frame you want to allow.

No objection to fixing the error message - not clear on the other part.
Generally my intuition is that allowing a badly malformed response
without a clear server bug to workaround unnecessarily weakens
our validity checking.   The better/stricter the validity checking the more
likely we are to throw away attack frames (intentionally illegal responses
designed to mess up the kernel).

On Thu, Jan 27, 2011 at 6:45 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The cERROR message in checkSMB when the calculated length doesn't match
> the RFC1001 length is incorrect in many cases. It always says that the
> RFC1001 length is bigger than the SMB, even when it's actually the
> reverse.
>
> Fix the error message to say the reverse of what it does now and remove
> the arbitrary check when an RFC1001 length is larger than the SMB.
> There's no reason to reject those packets since we can just ignore the
> junk that's hanging off the end.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/misc.c |   19 +++----------------
>  1 files changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 72e99ec..959d629 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -467,23 +467,10 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
>                        if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
>                                return 0; /* bcc wrapped */
>                }
> -               cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
> +               cFYI(1, "Calculated size %u vs length %u mismatch for mid=%u",
>                                clc_len, 4 + len, smb->Mid);
> -               /* Windows XP can return a few bytes too much, presumably
> -               an illegal pad, at the end of byte range lock responses
> -               so we allow for that three byte pad, as long as actual
> -               received length is as long or longer than calculated length */
> -               /* We have now had to extend this more, since there is a
> -               case in which it needs to be bigger still to handle a
> -               malformed response to transact2 findfirst from WinXP when
> -               access denied is returned and thus bcc and wct are zero
> -               but server says length is 0x21 bytes too long as if the server
> -               forget to reset the smb rfc1001 length when it reset the
> -               wct and bcc to minimum size and drop the t2 parms and data */
> -               if ((4+len > clc_len) && (len <= clc_len + 512))
> -                       return 0;
> -               else {
> -                       cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
> +               if (4+len < clc_len) {
> +                       cERROR(1, "RFC1001 size %d smaller than SMB for mid=%u",
>                                        len, smb->Mid);
>                        return 1;
>                }
> --
> 1.7.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Thanks,

Steve

Re: [PATCH] cifs: fix length checks in checkSMB

[Jeff Layton]

On Thu, 27 Jan 2011 14:41:14 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> If a server has a bug and sends an SMB which is more than just "round up" off
> in length - I can imagine a case for making exceptions depending on the
> details of the type of malformed frame, but since we
> already allow 512 bytes (for "incorrect server rounding of frame size" bugs)
> we should be careful.  If the length is more than 512 bytes off - this is
> a server bug and the probability that the rest of the frame is incorrect
> is very high.  How off is the frame you want to allow.
> 
> No objection to fixing the error message - not clear on the other part.
> Generally my intuition is that allowing a badly malformed response
> without a clear server bug to workaround unnecessarily weakens
> our validity checking.   The better/stricter the validity checking the more
> likely we are to throw away attack frames (intentionally illegal responses
> designed to mess up the kernel).
> 

I see no problem with allowing an RFC frame up to the size of the
buffer. By the time we get to this check, we've *already* received it
into the response buffer. cifs_demultiplex_thread has this check:

                /* else we have an SMB response */
                if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
                            (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
                        cERROR(1, "Invalid size SMB length %d pdu_length %d",
                                        length, pdu_length+4);
                        cifs_reconnect(server);
                        csocket = server->ssocket;
                        wake_up(&server->response_q);
                        continue;
                }

...so we've already received it into the buffer, and we know that the
RFC length fits within the buffer, and we've checked that the
calculated SMB length fits within the RFC length.

So what's the purpose of only allowing an RFC frame that's 512 bytes
bigger than the SMB? It's certainly inefficient for the server to send
us such a frame, but it's not really harmful and I see no need to
reject it for that reason alone.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: fix length checks in checkSMB

[Steve French]

On Thu, Jan 27, 2011 at 4:02 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 27 Jan 2011 14:41:14 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> If a server has a bug and sends an SMB which is more than just "round up" off
>> in length - I can imagine a case for making exceptions depending on the
>> details of the type of malformed frame, but since we
>> already allow 512 bytes (for "incorrect server rounding of frame size" bugs)
>> we should be careful.  If the length is more than 512 bytes off - this is
>> a server bug and the probability that the rest of the frame is incorrect
>> is very high.  How off is the frame you want to allow.
>>
>> No objection to fixing the error message - not clear on the other part.
>> Generally my intuition is that allowing a badly malformed response
>> without a clear server bug to workaround unnecessarily weakens
>> our validity checking.   The better/stricter the validity checking the more
>> likely we are to throw away attack frames (intentionally illegal responses
>> designed to mess up the kernel).
>>
>
> I see no problem with allowing an RFC frame up to the size of the
> buffer. By the time we get to this check, we've *already* received it
> into the response buffer. cifs_demultiplex_thread has this check:
>
>                /* else we have an SMB response */
>                if ((pdu_length > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) ||
>                            (pdu_length < sizeof(struct smb_hdr) - 1 - 4)) {
>                        cERROR(1, "Invalid size SMB length %d pdu_length %d",
>                                        length, pdu_length+4);
>                        cifs_reconnect(server);
>                        csocket = server->ssocket;
>                        wake_up(&server->response_q);
>                        continue;
>                }
>
> ...so we've already received it into the buffer, and we know that the
> RFC length fits within the buffer, and we've checked that the
> calculated SMB length fits within the RFC length.
>
> So what's the purpose of only allowing an RFC frame that's 512 bytes
> bigger than the SMB? It's certainly inefficient for the server to send
> us such a frame, but it's not really harmful and I see no need to
> reject it for that reason alone.

I would rather not continue parsing a frame that is clearly broken.  Since
a legitimate server is unlikely to send such a frame, the probability
of such a frame coming from a hostile server or proxy or man
in the middle is high.   A "random" RFC length that is wrong is likely
to be generated in a random attack, less likely by a legitimate server.


-- 
Thanks,

Steve

[PATCH] cifs: fix length checks in checkSMB (try #2)

[Jeff Layton]

The cERROR message in checkSMB when the calculated length doesn't match
the RFC1001 length is incorrect in many cases. It always says that the
RFC1001 length is bigger than the SMB, even when it's actually the
reverse.

Fix the error message to say the reverse of what it does now when the
SMB length goes beyond the end of the received data. Also, clarify the
error message when the RFC length is too big. Finally, clarify the
comments to show that the 512 byte limit on extra data at the end of
the packet is arbitrary.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/misc.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 72e99ec..77dc732 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -467,25 +467,26 @@ checkSMB(struct smb_hdr *smb, __u16 mid, unsigned int length)
 			if (((4 + len) & 0xFFFF) == (clc_len & 0xFFFF))
 				return 0; /* bcc wrapped */
 		}
-		cFYI(1, "Calculated size %d vs length %d mismatch for mid %d",
+		cFYI(1, "Calculated size %u vs length %u mismatch for mid=%u",
 				clc_len, 4 + len, smb->Mid);
-		/* Windows XP can return a few bytes too much, presumably
-		an illegal pad, at the end of byte range lock responses
-		so we allow for that three byte pad, as long as actual
-		received length is as long or longer than calculated length */
-		/* We have now had to extend this more, since there is a
-		case in which it needs to be bigger still to handle a
-		malformed response to transact2 findfirst from WinXP when
-		access denied is returned and thus bcc and wct are zero
-		but server says length is 0x21 bytes too long as if the server
-		forget to reset the smb rfc1001 length when it reset the
-		wct and bcc to minimum size and drop the t2 parms and data */
-		if ((4+len > clc_len) && (len <= clc_len + 512))
-			return 0;
-		else {
-			cERROR(1, "RFC1001 size %d bigger than SMB for Mid=%d",
+
+		if (4 + len < clc_len) {
+			cERROR(1, "RFC1001 size %u smaller than SMB for mid=%u",
 					len, smb->Mid);
 			return 1;
+		} else if (len > clc_len + 512) {
+			/*
+			 * Some servers (Windows XP in particular) send more
+			 * data than the lengths in the SMB packet would
+			 * indicate on certain calls (byte range locks and
+			 * trans2 find first calls in particular). While the
+			 * client can handle such a frame by ignoring the
+			 * trailing data, we choose limit the amount of extra
+			 * data to 512 bytes.
+			 */
+			cERROR(1, "RFC1001 size %u more than 512 bytes larger "
+				  "than SMB for mid=%u", len, smb->Mid);
+			return 1;
 		}
 	}
 	return 0;
-- 
1.7.3.4

Re: [PATCH] cifs: fix length checks in checkSMB (try #2)

[Steve French]

On Fri, Jan 28, 2011 at 6:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The cERROR message in checkSMB when the calculated length doesn't match
> the RFC1001 length is incorrect in many cases. It always says that the
> RFC1001 length is bigger than the SMB, even when it's actually the
> reverse.
>
> Fix the error message to say the reverse of what it does now when the
> SMB length goes beyond the end of the received data. Also, clarify the
> error message when the RFC length is too big. Finally, clarify the
> comments to show that the 512 byte limit on extra data at the end of
> the packet is arbitrary.

Do we have any data to indicate that other servers send incorrect
rfc1001 lengths larger than 512 bytes?  In eight years of test events, I
have seen only a few responses (from Windows and NetApp IIRC) where the
length was slightly too long.  The 512 byte was chosen because it
was large enough to pass testing to all servers we had seen,
allowing us to workaround problematic responses from real servers
while still limiting the probability that "randomly generated" frames
with incorrect lengths would pass validation checks and be
processed.

If we have data on a byte range lock or find first response from Windows
generating more than 512 bytes of junk then we should remove the
check.

The theory proposed to explain why we see incorrect frame lengths
from some servers was that due to rounding or due to RFC1001 length
being calculated before it was determined whether the frame would return
an smb error (e.g. access control error) error - the length could be off
by up to sizeof of one t2 infolevel  Length could be off up to the sizeof of
the fixed portion of one transact2 infolevel (all of which are less
than 512 bytes)
if the server e.g. decides to include one fewer findfirst entry in a response,
or if a server fills in  t2 query file info data, then gets an access error,
leaving the data, but adjusting the smb length, leaving the incorrect
rfc1001 len.
In all of the theorized and observed (at test events)
cases, the server would never be off by more than sizeof largest transact2
infolevel.

Also note that removing this check makes it harder for new server
manufacturers to catch serious bugs in their products as the client
no longer will do validity checking for them.  Speaking from past experience
it is more painful to debug a server mixing up the wrong type of response
for a request if validity checking is overly relaxed.

Does anyone have data on an existing server in the field that sends
anything off by more than
512 bytes (the most I remember a server being off was about 100 or so bytes)
in frame length?


-- 
Thanks,

Steve

Re: [PATCH] cifs: fix length checks in checkSMB (try #2)

[Jeff Layton]

On Fri, 28 Jan 2011 12:53:52 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Jan 28, 2011 at 6:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > The cERROR message in checkSMB when the calculated length doesn't match
> > the RFC1001 length is incorrect in many cases. It always says that the
> > RFC1001 length is bigger than the SMB, even when it's actually the
> > reverse.
> >
> > Fix the error message to say the reverse of what it does now when the
> > SMB length goes beyond the end of the received data. Also, clarify the
> > error message when the RFC length is too big. Finally, clarify the
> > comments to show that the 512 byte limit on extra data at the end of
> > the packet is arbitrary.
> 
> Do we have any data to indicate that other servers send incorrect
> rfc1001 lengths larger than 512 bytes?  In eight years of test events, I
> have seen only a few responses (from Windows and NetApp IIRC) where the
> length was slightly too long.  The 512 byte was chosen because it
> was large enough to pass testing to all servers we had seen,
> allowing us to workaround problematic responses from real servers
> while still limiting the probability that "randomly generated" frames
> with incorrect lengths would pass validation checks and be
> processed.
> 

I'm not aware of any. My reason for proposing the patch to remove the
+512b limitation was simply that it didn't seem necessary to impose
such a limitation. The lengths all fit within what we had received so
ignoring any extra junk tacked on the end is harmless.

> If we have data on a byte range lock or find first response from Windows
> generating more than 512 bytes of junk then we should remove the
> check.
> 
> The theory proposed to explain why we see incorrect frame lengths
> from some servers was that due to rounding or due to RFC1001 length
> being calculated before it was determined whether the frame would return
> an smb error (e.g. access control error) error - the length could be off
> by up to sizeof of one t2 infolevel  Length could be off up to the sizeof of
> the fixed portion of one transact2 infolevel (all of which are less
> than 512 bytes)
> if the server e.g. decides to include one fewer findfirst entry in a response,
> or if a server fills in  t2 query file info data, then gets an access error,
> leaving the data, but adjusting the smb length, leaving the incorrect
> rfc1001 len.
> In all of the theorized and observed (at test events)
> cases, the server would never be off by more than sizeof largest transact2
> infolevel.
> 
> Also note that removing this check makes it harder for new server
> manufacturers to catch serious bugs in their products as the client
> no longer will do validity checking for them.  Speaking from past experience
> it is more painful to debug a server mixing up the wrong type of response
> for a request if validity checking is overly relaxed.
> 
> Does anyone have data on an existing server in the field that sends
> anything off by more than
> 512 bytes (the most I remember a server being off was about 100 or so bytes)
> in frame length?
> 

I give up. The latest patch (try #2) doesn't remove any checks. It just
clarifies the cERROR printk's and the comments.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: fix length checks in checkSMB (try #2)

[Steve French]

On Fri, Jan 28, 2011 at 1:28 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 28 Jan 2011 12:53:52 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Fri, Jan 28, 2011 at 6:24 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > The cERROR message in checkSMB when the calculated length doesn't match
>> > the RFC1001 length is incorrect in many cases. It always says that the
>> > RFC1001 length is bigger than the SMB, even when it's actually the
>> > reverse.
>> >
>> > Fix the error message to say the reverse of what it does now when the
>> > SMB length goes beyond the end of the received data. Also, clarify the
>> > error message when the RFC length is too big. Finally, clarify the
>> > comments to show that the 512 byte limit on extra data at the end of
>> > the packet is arbitrary.
>>
>> Do we have any data to indicate that other servers send incorrect
>> rfc1001 lengths larger than 512 bytes?  In eight years of test events, I
>> have seen only a few responses (from Windows and NetApp IIRC) where the
>> length was slightly too long.  The 512 byte was chosen because it
>> was large enough to pass testing to all servers we had seen,
>> allowing us to workaround problematic responses from real servers
>> while still limiting the probability that "randomly generated" frames
>> with incorrect lengths would pass validation checks and be
>> processed.
>>
>
> I'm not aware of any. My reason for proposing the patch to remove the
> +512b limitation was simply that it didn't seem necessary to impose
> such a limitation. The lengths all fit within what we had received so
> ignoring any extra junk tacked on the end is harmless.
>
>> If we have data on a byte range lock or find first response from Windows
>> generating more than 512 bytes of junk then we should remove the
>> check.
>>
>> The theory proposed to explain why we see incorrect frame lengths
>> from some servers was that due to rounding or due to RFC1001 length
>> being calculated before it was determined whether the frame would return
>> an smb error (e.g. access control error) error - the length could be off
>> by up to sizeof of one t2 infolevel  Length could be off up to the sizeof of
>> the fixed portion of one transact2 infolevel (all of which are less
>> than 512 bytes)
>> if the server e.g. decides to include one fewer findfirst entry in a response,
>> or if a server fills in  t2 query file info data, then gets an access error,
>> leaving the data, but adjusting the smb length, leaving the incorrect
>> rfc1001 len.
>> In all of the theorized and observed (at test events)
>> cases, the server would never be off by more than sizeof largest transact2
>> infolevel.
>>
>> Also note that removing this check makes it harder for new server
>> manufacturers to catch serious bugs in their products as the client
>> no longer will do validity checking for them.  Speaking from past experience
>> it is more painful to debug a server mixing up the wrong type of response
>> for a request if validity checking is overly relaxed.
>>
>> Does anyone have data on an existing server in the field that sends
>> anything off by more than
>> 512 bytes (the most I remember a server being off was about 100 or so bytes)
>> in frame length?
>>
>
> I give up. The latest patch (try #2) doesn't remove any checks. It just
> clarifies the cERROR printk's and the comments.

Patch if fine and mergeable.   I don't object to the error cleanup,
but I thought
you were implying that you had data on two cases where servers
would still fail with such a check.



-- 
Thanks,

Steve