Bug 8360 - smbd fails on access pattern of OS/2 client
smbd fails on access pattern of OS/2 client
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: File services
3.5.6
All All
: P4 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8399
  Show dependency treegraph
 
Reported: 2011-08-08 20:15 UTC by Marcel Müller
Modified: 2011-08-25 16:35 UTC (History)
2 users (show)

See Also:


Attachments
Test executable and test data for OS/2 or eCS clients (28.73 KB, application/octet-stream)
2011-08-08 20:15 UTC, Marcel Müller
no flags Details
only the basic stuff (3.19 KB, text/plain)
2011-08-10 04:02 UTC, Guenter Kukkukk
no flags Details
new OS/2 test applet (27.37 KB, application/zip)
2011-08-10 04:05 UTC, Guenter Kukkukk
no flags Details
wireshark trace. os2 against samba master (2.26 KB, application/octet-stream)
2011-08-10 04:11 UTC, Guenter Kukkukk
no flags Details
Patch for master (1.92 KB, patch)
2011-08-11 14:54 UTC, Volker Lendecke
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel Müller 2011-08-08 20:15:02 UTC
Created attachment 6759 [details]
Test executable and test data for OS/2 or eCS clients

Running the test case (see attachment) on an OS/2 client using the IBM peer requester (LANMAN2 protocol) causes the client application to fail with an error.

Steps to reproduce: run the attached test case from a samba share on an OS/2 client. It will not complete successfully.

The error happens at a WriteAndX request, but is reported to application at the last read in the test case, because of write back caching of the client. I guess that the bug exists since Samba 3.2 where reply.c has significantly changed. I had similar problems with Samba 3.2 but could not track them down.

A detailed log follows by Günter Kukkukk.

Environment:
- Client: eCS 1.01 with IBM peer requester (all available fixpacks allpied)
- Server: samba 3.5.6 on amd64 (Debian squeeze)
Comment 1 Guenter Kukkukk 2011-08-10 03:55:02 UTC
Sorry for the delay - i had some problems to get our "sleeping" OS/2 Warpserver running again.
Was needed cause all our compile/build stuff is located there.

Marcel, you did a really great job supplying a test program - i just modified it
to get the basics for a debug log and a network trace.


./smbd/process.c

void chain_reply(struct smb_request *req)
....
	if (chain_offset < already_used) {
		DEBUG(0,("GKK2: chain_offset (%u) < already_used (%d)\n", chain_offset, already_used));
		goto error;
	}

----
my added DEBUG code results to:
   GKK2: chain_offset (60) < already_used (217)
----
The current implementation assumes that the AndX info must be placed
at a _higher_ offset than the basic data.
Here the AndX (read) info is placed _before_ the write data offset.

Anyway - even wireshark is unhappy with this ...

Will add a (short) debug10 log, a wireshark trace and my new OS/2 test applet.
I think, a network trace against working windows is not needed.

Cheers, Günter
Comment 2 Guenter Kukkukk 2011-08-10 04:02:16 UTC
Created attachment 6767 [details]
only the basic stuff
Comment 3 Guenter Kukkukk 2011-08-10 04:05:06 UTC
Created attachment 6768 [details]
new OS/2 test applet
Comment 4 Guenter Kukkukk 2011-08-10 04:11:01 UTC
Created attachment 6769 [details]
wireshark trace. os2 against samba master
Comment 5 Guenter Kukkukk 2011-08-11 03:57:54 UTC
Hi Volker,

./smbd/process.c

void chain_reply(struct smb_request *req)
....
        /*
         * Check if the client tries to fool us. The request so far uses the
         * space to the end of the byte buffer in the request just
         * processed. The chain_offset can't point into that area. If that was
         * the case, we could end up with an endless processing of the chain,
         * we would always handle the same request.
         */

        already_used = PTR_DIFF(req->buf+req->buflen, smb_base(req->inbuf));
        if (chain_offset < already_used) {
              goto error;
        }
....

Is the above check really needed?
"... The chain_offset can't point into that area..."

I'm really no expert in that area - but when I remove the
   if (chain_offset < already_used) {
check - the os/2 testapplet is running fine.

In addition, i modified the os/2 test applet to
check whether the WriteAndX (read) call is really
working as expected - all seems to be ok so far.

Cheers, Günter
Comment 6 Volker Lendecke 2011-08-11 14:42:55 UTC
(In reply to comment #5)

> Is the above check really needed?
> "... The chain_offset can't point into that area..."
> 
> I'm really no expert in that area - but when I remove the
>    if (chain_offset < already_used) {
> check - the os/2 testapplet is running fine.

Thanks for that analysis. Yes, I think something like that check is really needed to avoid the DoS described in the comment. If someone goes in and points the chain offset again at the same command, then we're in a 100% loop. Probably the check is too strict though. It would be sufficient if the andx offset is just one byte beyond the last andx offset to protect against this. I'm right now trying to code that up.

> In addition, i modified the os/2 test applet to
> check whether the WriteAndX (read) call is really
> working as expected - all seems to be ok so far.

Have you checked with a sequence not only containing all X'es? If we get the offset calculation wrong then we're screwed as well.

Volker
Comment 7 Volker Lendecke 2011-08-11 14:54:48 UTC
Created attachment 6774 [details]
Patch for master

Can you try the attached patch? I don't have OS/2 available right now, so I can't really check whether this is right. I might have an off-by-one or so still in there.
Comment 8 Guenter Kukkukk 2011-08-11 23:41:23 UTC
Hi Volker,

your patch fixes the bug! :-)

I've checked your calculations and there is no
off-by-one or other mismatch:
  already_used (59)
  chain_offset (60) 

In addition i modified my test applet slightly,
to check whether the data is written at the right
file offset and also whether the AndX Read is
reading the right bytes from the specified file
offset.
Also a later "hexdump -C -v test.file" shows that  
all data is at expected offsets.

So i think, you can apply your patch and close
this bug.

Marcel, any comments from your side?
Cheers, Günter
Comment 9 Volker Lendecke 2011-08-12 14:18:50 UTC
Comment on attachment 6774 [details]
Patch for master

Jeremy, I'd like to get your ack on this as well
Comment 10 Volker Lendecke 2011-08-12 14:19:45 UTC
Pushed the patch to master, waiting for autobuild
Comment 11 Jeremy Allison 2011-08-12 21:20:59 UTC
Comment on attachment 6774 [details]
Patch for master

My goodness that's a subtle one. This one is needed for 3.6.1 and for 3.5.12 as well I think.

Impressive work Volker !

Jeremy.
Comment 12 Jeremy Allison 2011-08-12 21:21:24 UTC
Re-assigning to Karolin for inclusion in 3.6.1 and 3.5.12.

Jeremy.
Comment 13 Volker Lendecke 2011-08-14 05:25:04 UTC
Just to give credits -- Kukks did most of that work.
Comment 14 Marcel Müller 2011-08-14 13:39:23 UTC
I just tested the patch.

It also fixes a problem with OS/2 Thunderbird version 3 and up crashing when the profile resides on a samba server. PM123 also does no longer fail to write ID3 tags.

There is still a problem with Thunderbird 3 failing to install extensions if the profile is on a samba share, but that is most likely not related to this bug.


Marcel
Comment 15 Karolin Seeger 2011-08-18 18:56:55 UTC
Pushed to v3-6-test and v3-5-test.
Closing out bug report.

Thanks!
Comment 16 Karolin Seeger 2011-08-18 18:58:10 UTC
(In reply to comment #14)
> I just tested the patch.
> 
> It also fixes a problem with OS/2 Thunderbird version 3 and up crashing when
> the profile resides on a samba server. PM123 also does no longer fail to write
> ID3 tags.
> 
> There is still a problem with Thunderbird 3 failing to install extensions if
> the profile is on a samba share, but that is most likely not related to this
> bug.
> 
> 
> Marcel

Marcel,

please feel free to open a new bug report for the other issue.

Thanks!