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)
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
Created attachment 6767 [details] only the basic stuff
Created attachment 6768 [details] new OS/2 test applet
Created attachment 6769 [details] wireshark trace. os2 against samba master
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
(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
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.
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 on attachment 6774 [details] Patch for master Jeremy, I'd like to get your ack on this as well
Pushed the patch to master, waiting for autobuild
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.
Re-assigning to Karolin for inclusion in 3.6.1 and 3.5.12. Jeremy.
Just to give credits -- Kukks did most of that work.
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
Pushed to v3-6-test and v3-5-test. Closing out bug report. Thanks!
(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!