Re: Fw: Buffer overflow in CIFS VFS.

Re: Fw: Buffer overflow in CIFS VFS.

[Steve French]

You are correct that the CIFS code calls SendReceive in cases in which
the buffer may be too small to fit a large SMB response, and that
should be fixed (e.g. to avoid possible overflows due to a server
bug), None of the eight cases (SMB TreeDisconnect, SMB uLogoff, SMB
Close, SMB FindClose etc.) in which a small buffer is passed in to
SendReceive return more than a few dozen bytes (and they are fixed
size responses), but I agree that we have to be safe (and we have seen
at least one server corrupt the bcc in the ulogoffX response and
another on the NTCreateX response) so it would be good to fix.

There are probably better ways to handle this though than passing in
the buffer size as your patch does.   Since there are only two buffer
sizes that CIFS uses - it would be easier to pass in (or out) a flag
which indicates the buffer size.  But the function SendReceive2
already does that - and the easier way to handle this seems to be
changing the eight places in fs/cifs/cifssmb.c which call
small_smb_init and then call SendReceive, to call SendReceive2
instead.

> From: Przemyslaw Wegrzyn <czajnik@czajsoft.pl>
> To: linux-kernel@vger.kernel.org
> Subject: Buffer overflow in CIFS VFS.
>
> just to find something that looks like a buffer overflow  bug.
> The problem is in SendReceive() function in transport.c - it memcpy's
> message payload into a buffer passed via out_buf param. The function
> assumes that all buffers are of size (CIFSMaxBufSize +
> MAX_CIFS_HDR_SIZE) , unfortunately it is also called with smaller
> (MAX_CIFS_SMALL_BUFFER_SIZE) buffers.
>
> To check this finding I patched Samba server to send oversized logoffX
> messages. With ~ 16kB messages the client running 2.6.23.1 crashed upon
> unmounting.
>
> I've done a quick fix, available here:
> http://czajnick.sitenet.pl/cifs-buffer-overflow-fix.patch.gz



-- 
Thanks,

Steve

Re: Fw: Buffer overflow in CIFS VFS.

[Przemyslaw Wegrzyn]

Steve French wrote:
> You are correct that the CIFS code calls SendReceive in cases in which
> the buffer may be too small to fit a large SMB response, and that
> should be fixed (e.g. to avoid possible overflows due to a server
> bug), None of the eight cases (SMB TreeDisconnect, SMB uLogoff, SMB
> Close, SMB FindClose etc.) in which a small buffer is passed in to
> SendReceive return more than a few dozen bytes (and they are fixed
> size responses), but I agree that we have to be safe (and we have seen
> at least one server corrupt the bcc in the ulogoffX response and
> another on the NTCreateX response) so it would be good to fix.
>   
Well, mounting shares from untrusted server is quite uncommon, still
buffer overrun shall be considered a serious issue, imho. If the buffer
was on the stack, that would be directly exploitable.
> There are probably better ways to handle this though than passing in
> the buffer size as your patch does.   Since there are only two buffer
> sizes that CIFS uses - it would be easier to pass in (or out) a flag
> which indicates the buffer size.  But the function SendReceive2
> already does that - and the easier way to handle this seems to be
> changing the eight places in fs/cifs/cifssmb.c which call
> small_smb_init and then call SendReceive, to call SendReceive2
> instead.
>   
That is mostly a matter of taste. SendReceive takes buffer pointer and
SendReceive2 takes kvec  - I'd rather prefer to stay with  the same
function used for both small and large buffers, using two different
functions just because 2 different buffer sizes are passed around is
ugly, imho, nonorthogonal at least.  I guess that initial intention was
to use SendReceive2 in places where kvec is really needed. Also these
functions are almost identical, maybe SendReceive should wrap
SendReceive2 preparing kvec with a buffer pointer passed to it?

Obviously it is up to you, as a maintainer. I'd prefer adding a small
header to each buffer with the buffer size and perhaps a type, or even a
destructor function pointer. Simple macros could be used to obtain
buffer size, given the buffer body pointer, or to dispose the buffer.
That would save from checking the buffer type all over the code
explicitly, or even worse, make strange assumptions about the type of
buffer being passed - as we can see this is error-prone. That for a
little cost of a few additional bytes per buffer.

Re: Fw: Buffer overflow in CIFS VFS.

[J. Bruce Fields]

On Fri, Nov 09, 2007 at 11:59:46AM +0100, Przemyslaw Wegrzyn wrote:
> Steve French wrote:
> > You are correct that the CIFS code calls SendReceive in cases in which
> > the buffer may be too small to fit a large SMB response, and that
> > should be fixed (e.g. to avoid possible overflows due to a server
> > bug), None of the eight cases (SMB TreeDisconnect, SMB uLogoff, SMB
> > Close, SMB FindClose etc.) in which a small buffer is passed in to
> > SendReceive return more than a few dozen bytes (and they are fixed
> > size responses), but I agree that we have to be safe (and we have seen
> > at least one server corrupt the bcc in the ulogoffX response and
> > another on the NTCreateX response) so it would be good to fix.
> >   
> Well, mounting shares from untrusted server is quite uncommon, still
> buffer overrun shall be considered a serious issue, imho.

Also, a compromised machine on the same network could forge the
malicious reply in some cases, right?

--b.

Re: Fw: Buffer overflow in CIFS VFS.

[Steve French]

I have done an analysis of the SMB functions (56 callers of
SendReceive, 4 of SendReceive2 and 2 callers of
SendReceiveBlockingLock) and found additional changes which should
help performance, by reducing the number of expensive large buffer
allocations and also by freeing buffers back to the pool faster.   See
below.   The obvious need is to create an SendReceive-NoResponse (or
equivalent) which
frees the SMB request buffer after send, and does not copy into an smb
response buffer.  The following functions need to be changed to use
that (and also address the problem that Przemyslaw noted):

TreeDisconnect, uLogoff, Close, findClose, SetFileSize, SetFileTimes,
	(Lock and PosixLock need slightly more complicated change
	since only some of their paths can use proposed new function)

There is a longer list of functions which should be optimized to call
a SendReceiveNoResponse like function as well (but we can do this
later after 2.6.24) since we also do not parse their response SMB (it
would make buffer allocation more efficient to free the request
buffers faster and avoid a memcpy):

PosixDelFile, DelFile, Rmdir, MkDir, Rename, RenameOpenFile, Copy,
CreateSymLink, CreateHardLinkUnix, CreateHardLink, SetPosixACL,
SetFSUnixInfo (also change to small buf allocate), SetEOF, SetTimes,
SetAttrLegacy, UnixSetPerms, SetEA

Some other functions can be changed to small buffer alloc requests:
Negotiate, SetFSUnixInfo, QueryReparseLink, GetExtAttr, the 6 QFSInfos
requests, Notify


Explanatory Key for the list below:
-------------------------------------------------
SR = the function currently uses SendReceive
SR2 = uses SendReceive2()
SRB = uses SendReceiveBlockingLock
large = uses smb_init (allocates large buffer)
small = uses small_smb_init (allocates small buffer)

LR = getting a large response back from the server is common
NR = no return SMB needed to be parsed by caller, processing only
depends on value of SMB error
ID = idempotent (does not change state significantly if repeated)
L  = can be long operation (and take longer than a few seconds)
B  = operation may block indefinitely
PB = operation has variable length request size due to path name in request
IOV = needs to send iovec

		List of SMB Functions and their characteristics
		-------------------------------------------------------------
(in fs/cifs/cifssmb.c)
Negotiate	large		 LR*, ID
TreeDisconnect	small	SR 	 NR
uLogoff	  	small	SR	 NR
PosixDelFile    large	SR	 PB,NR
DelFile         large   SR	 PB,NR
RmDir		large   SR	 PB,NR
MkDir		large   SR 	 PB,NR
PosixCreate     large	SR	 PB
LegacyOpen      large	SR	 PB
Open		large	SR	 PB
Read		small	SR2	 LR, ID
Write		large	SR	 (need mixed user/kernel IOV to change) L, ID?
Write2		small	SR2	 IOV, L
Lock		small	SR/SRB   NR, B
PosixLock	small	SR/SRB	 (NR on setpath), B
Close		small	SR	 NR
Rename		large	SR	 NR, PB
RenameOpenFile	large	SR	 NR, PB
Copy		large   SR	 NR, PB
CreateSymLink	large	SR	 NR, PB
CreateHardLinkU large	SR	 NR, PB
CreateHardLink	large	SR	 NR, PB
QuerySymLink	large	SR	 LR, PB, ID
QueryReparseLk	large	SR	 LR, ID
QueryPosixACL	large	SR	 LR, PB, ID
SetPosixACL	large	SR	 NR, PB
GetExtAttr	large	SR	 LR, ID
GetCIFSACL	small	SR2	 LR, ID
QueryInformatn	large	SR	 PB, ID
QPathInfo	large	SR	 PB, ID
UnixQPathInfo	large	SR	 PB, ID
FindSingle	large	SR	 PB, LR?, ID (remove)
FindFirst	large	SR	 PB, LR
FindNext	large	SR	 PB*, LR, ID*  (resume key)
FindClose	small	SR	 NR
GetSrvInodeNum	large	SR	 PB, ID
GetDFSRefer	large	SR	 PB, LR, ID
OldQFSInfo	large	SR	 ID
QFSInfo		large	SR	 ID
QFSAttribute	large	SR	 ID
QFSDeviceInfo	large	SR	 ID
QFSUnixInfo	large	SR	 ID
SetFSUnixInfo	large	SR	 ID, NR
QFSPosixInfo	large	SR	 ID
SetEOF		large	SR	 PB, NR
SetFileSize	small	SR	 NR
SetFileTimes	small	SR	 NR
SetTimes	large	SR	 PB, NR
SetAttrLegacy	large	SR	 PB, NR
UnixSetPerms	large	SR	 PB, NR
Notify		large	SR
QueryAllEAs	large	SR	 PB, LR, ID
QueryEA		large	SR	 PB, LR, ID
SetEA		large	SR	 PB, NR

(in fs/cifs/sess.c)
SessionSetup	large	SR2	 IOV, large request, LR

(in fs/cifs/connect.c)
TreeConnect	large	SR	 PB
other uses of SendReceive* in this file are obsolete

(in fs/cifs/transport.c)
send_lock_cancel  no buffer allocate, SR  NR   function looks broken



On Nov 9, 2007 4:59 AM, Przemyslaw Wegrzyn <czajnik@czajsoft.pl> wrote:
> to use SendReceive2 in places where kvec is really needed. Also these
> functions are almost identical, maybe SendReceive should wrap
> SendReceive2 preparing kvec with a buffer pointer passed to it?
>
> Obviously it is up to you, as a maintainer. I'd prefer adding a small
> header to each buffer with the buffer size and perhaps a type, or even a
> destructor function pointer. Simple macros could be used to obtain
> buffer size, given the buffer body pointer, or to dispose the buffer.
> That would save from checking the buffer type all over the code
> explicitly, or even worse, make strange assumptions about the type of
> buffer being passed - as we can see this is error-prone. That for a
> little cost of a few additional bytes per buffer.
That might be better, although without memory pools, this would perform
much worse

>



-- 
Thanks,

Steve

Re: Fw: Buffer overflow in CIFS VFS.

[Przemyslaw Wegrzyn]

Steve French wrote:
> below.   The obvious need is to create an SendReceive-NoResponse (or
> equivalent) which
> frees the SMB request buffer after send, and does not copy into an smb
> response buffer.  The following functions need to be changed to use
>   
How about modifying SendReceive to behave like that if NULL is passed as
output buffer ?

>> Obviously it is up to you, as a maintainer. I'd prefer adding a small
>> header to each buffer with the buffer size and perhaps a type, or even a
>> destructor function pointer. Simple macros could be used to obtain
>> buffer size, given the buffer body pointer, or to dispose the buffer.
>> That would save from checking the buffer type all over the code
>> explicitly, or even worse, make strange assumptions about the type of
>> buffer being passed - as we can see this is error-prone. That for a
>> little cost of a few additional bytes per buffer.
>>     
> That might be better, although without memory pools, this would perform
> much worse
>   
Why ? I don't get your point here.

Przemyslaw

Re: Fw: Buffer overflow in CIFS VFS.

[Steve French]

On Nov 10, 2007 7:03 AM, Przemyslaw Wegrzyn <czajnik@czajsoft.pl> wrote:
> Steve French wrote:
> > That might be better, although without memory pools, this would perform
> > much worse
> >
> Why ? I don't get your point here.
>
> Przemyslaw
>

What I meant is that two fixed size memory pools rather variable size
kmallocs helps performance.  By using two fixed size buffers (small -
which fits the typical smb request and response, and large which fits
the maximum size request other than write which is handled via an
iovec) and taking advantage of memory pools, cifs can always make
progress even in low memory situations (reducing the likelihood of
deadlock) and it reduces the number of times that cifs has to do the
very expensive allocation of 16.5K.  If cifs only used variable size
request and response buffers, since they are frequently more than one
page in size we would be forcing the memory manager to find contiguous
pages which can be very slow.



-- 
Thanks,

Steve

Re: Fw: Buffer overflow in CIFS VFS.

[Przemyslaw Wegrzyn]

Steve French wrote:
> On Nov 10, 2007 7:03 AM, Przemyslaw Wegrzyn <czajnik@czajsoft.pl> wrote:
>   
>> Steve French wrote:
>>     
>>> That might be better, although without memory pools, this would perform
>>> much worse
>>>
>>>       
>> Why ? I don't get your point here.
>>     
> What I meant is that two fixed size memory pools rather variable size
> kmallocs helps performance.  By using two fixed size buffers (small -
> which fits the typical smb request and response, and large which fits
> the maximum size request other than write which is handled via an
> iovec) and taking advantage of memory pools, cifs can always make
> progress even in low memory situations (reducing the likelihood of
> deadlock) and it reduces the number of times that cifs has to do the
> very expensive allocation of 16.5K.  If cifs only used variable size
> request and response buffers, since they are frequently more than one
> page in size we would be forcing the memory manager to find contiguous
> pages which can be very slow.
>   
That was not really my intention to allocate variable size buffers -
it's perfectly Ok for me to have 2 fixed-size pools. What I meant was to
allocate buffers with some kind of header containing buffer size and
type of buffer or whatever needed info. Obviously, you could use
pointers to buffer body all over the code, functions/macros that need to
access the header would move the pointer back by sizeof(header). Then
one could e.g. provide a single function for disposing the buffer, and
no longer care which pool is it allocated from, for the additional
memory cost - the function would look into the header to check that for
you. Hope it's clear now (my English is far from perfect).

Przemyslaw