close_directory sets a security context without security_token this later leads to a crash in security_token_has_sid when the veto files check is applied smb: \> mkdir bla smb: \> cd bla smb: \bla\> mkdir blub smb: \bla\> cd .. [now do a chmod 000 /share/root/bla/blub] smb: \> rmdir bla Receiving SMB: Server stopped responding Call returned zero bytes (EOF) removing remote directory file \bla Volker to add more details
Can you post the stack backtrace please ? Looks like that's not the only place that sets a security context without a token. We even have the nt_token_check_sid() function which protects against NULL args. I'd love to see where this is being used. Jeremy.
It's from within rmdir_internals->is_visible_file->user_can_read_file. A few things need to come together: veto files and hide unreadable both set. You try to delete a non-empty directory where you don't have read access to the remaining contents.
Ah, btw, the real fix I believe is to add the nt token to the delete_on_close token in locking.tdb. We have that as pidl now, so that should not be too hard.
Can you post the stack backtrace ? That'll make it really easy for me to see. Cheers, Jeremy.
No, not right now. Believe me, if I had it readily available I would have sent it to you with my first post. As it is late here and apparently I have not provided sufficient information for you to reproduce it yourself, I'll close this bug for now, we will fix that ourselves later.
Don't be such a complete *asshole*. This is the kind of behavior noted in "How Open Source Projects Survive Poisonous People (And You Can Too)": http://video.google.com/videoplay?docid=-4216011961522818645 and the kind of behavior Ulrich Drepper was rightly condemned for. I won't stand for such behavior in Samba. I'm re-opening the bug and re-assigning to myself and will fix asap (I've been in a school parents meeting this afternoon) once I've got a backtrace I create myself. Jeremy.
Ok, I see the exact problem. Your recommendation is correct (adding the nt token to the delete on close token struct). As you mention this is easier to fix for master, for 3.6.x we need to add to the hand marshalling code instead of just using the pidl method. I'll start work on this asap (although I'm up at the Linux Foundation Collaboration Summit Tues-Thurs this week) and see if I can get something to test soon. Jeremy.
Created attachment 7422 [details] Initial attempt at patch for master. Ok, here's an initial patch for master. Currently fails due to valgrind catching an uninitialized memory read when marshalling the "security_token" from conn->session_info->security_token - but I can't see why that is uninitialized at the moment (may not be a complete rebuild). I'm adding this here as an idea of what I think the fix will eventually be, whilst I still work on it. Jeremy.
Created attachment 7423 [details] Patch for master Ok, found the missing hunk that caused the valgrind error. I think this might contain the logic to fix it - will do more tests in the next day or so, and if they pass I'll split it up into a couple of smaller changes and try an autobuild. Jeremy.
(In reply to comment #6) > Don't be such a complete *asshole*. This is the kind of behavior noted in > "How Open Source Projects Survive Poisonous People (And You Can Too)": Sorry, but I wanted to take away the pressure by closing the bug. It seemed that I did not provide enough information for you to reproduce it. It was 11pm yesterday, and I was too tired to reproduce it again to generate the stacktrace. Sorry that I did not provide the level of information that was necessary for you to easily see the problem. THAT is the reason why I closed it and why I wanted to take the pressure from you. It was a mistake in the first place to file this bug in such a hurry, we should have prepared it MUCH better than we did. Next time.
Never mind. I understand being too tired/busy to produce a stacktrace :-). Closing the bug doesn't reduce any pressure though, it just causes me immense irritation and could cause me to drop it on the floor by accident :-). Don't worry about logging bugs early and without enough info - I'd rather know about them sooner rather than later (especially if they're in my code, like this one was). I think my prototype patch fixes the issue for master (I'll try and correctly reproduce sometime over the next 3 days - I'm up in SF at the LinuxCollab summit Tues-Thurs, but I have a laptop :-). Once I know it's fixed I'll back-port for 3.6.x. Cheers, Jeremy.
the crash occurs even without veto files enabled as the check in rmdir_internals() checks for lp_veto_files()!=0 and as lp_veto_files() returns an empty string, we go into the loop Sorry for not posting the stacktrace earlier, here it is: 2012-04-03T18:14:41.577094+02:00 mgmt001st001 smbd[3965898]: BACKTRACE: 22 stack frames: 2012-04-03T18:14:41.577169+02:00 mgmt001st001 smbd[3965898]: #0 bin/smbd(log_stack_trace+0x1a) [0x7febbef5da1a] 2012-04-03T18:14:41.577238+02:00 mgmt001st001 smbd[3965898]: #1 bin/smbd(smb_panic+0x2b) [0x7febbef5daeb] 2012-04-03T18:14:41.577332+02:00 mgmt001st001 smbd[3965898]: #2 bin/smbd(+0x42b2d4) [0x7febbef4d2d4] 2012-04-03T18:14:41.577400+02:00 mgmt001st001 smbd[3965898]: #3 /lib64/libc.so.6(+0x32ac0) [0x7febbbd28ac0] 2012-04-03T18:14:41.577466+02:00 mgmt001st001 smbd[3965898]: #4 bin/smbd(security_token_has_sid+0xa) [0x7febbef8da8a] 2012-04-03T18:14:41.577533+02:00 mgmt001st001 smbd[3965898]: #5 bin/smbd(se_access_check+0xd0) [0x7febbef74d60] 2012-04-03T18:14:41.577600+02:00 mgmt001st001 smbd[3965898]: #6 bin/smbd(can_access_file_acl+0x83) [0x7febbecb0163] 2012-04-03T18:14:41.577666+02:00 mgmt001st001 smbd[3965898]: #7 bin/smbd(is_visible_file+0x410) [0x7febbec2a720] 2012-04-03T18:14:41.577733+02:00 mgmt001st001 smbd[3965898]: #8 bin/smbd(close_file+0x7ff) [0x7febbec85cbf] 2012-04-03T18:14:41.577812+02:00 mgmt001st001 smbd[3965898]: #9 bin/smbd(reply_rmdir+0x185) [0x7febbec598e5] 2012-04-03T18:14:41.577880+02:00 mgmt001st001 smbd[3965898]: #10 bin/smbd(+0x17bc2c) [0x7febbec9dc2c] 2012-04-03T18:14:41.577948+02:00 mgmt001st001 smbd[3965898]: #11 bin/smbd(+0x17c05b) [0x7febbec9e05b] 2012-04-03T18:14:41.578035+02:00 mgmt001st001 smbd[3965898]: #12 bin/smbd(+0x17cd90) [0x7febbec9ed90] 2012-04-03T18:14:41.578091+02:00 mgmt001st001 smbd[3965898]: #13 bin/smbd(run_events_poll+0x376) [0x7febbef6dde6] 2012-04-03T18:14:41.578157+02:00 mgmt001st001 smbd[3965898]: #14 bin/smbd(smbd_process+0x885) [0x7febbec9cf45] 2012-04-03T18:14:41.578222+02:00 mgmt001st001 smbd[3965898]: #15 bin/smbd(+0x6b540f) [0x7febbf1d740f] 2012-04-03T18:14:41.578288+02:00 mgmt001st001 smbd[3965898]: #16 bin/smbd(run_events_poll+0x376) [0x7febbef6dde6] 2012-04-03T18:14:41.578353+02:00 mgmt001st001 smbd[3965898]: #17 bin/smbd(+0x44c353) [0x7febbef6e353] 2012-04-03T18:14:41.578419+02:00 mgmt001st001 smbd[3965898]: #18 bin/smbd(_tevent_loop_once+0x90) [0x7febbef6eca0] 2012-04-03T18:14:41.578484+02:00 mgmt001st001 smbd[3965898]: #19 bin/smbd(main+0xf53) [0x7febbf1d8853] 2012-04-03T18:14:41.578551+02:00 mgmt001st001 smbd[3965898]: #20 /lib64/libc.so.6(__libc_start_main+0xfd) [0x7febbbd14c9d] 2012-04-03T18:14:41.578616+02:00 mgmt001st001 smbd[3965898]: #21 bin/smbd(+0xf47f9) [0x7febbec167f9]
To correct the wrong check for lp_veto_files, I would propose the following change: diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 25c8b46..f5e19f1 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -817,7 +817,8 @@ static NTSTATUS rmdir_internals(TALLOC_CTX *ctx, files_struct *fsp) return NT_STATUS_OK; } - if(((errno == ENOTEMPTY)||(errno == EEXIST)) && lp_veto_files(SNUM(conn))) { + if ( ((errno == ENOTEMPTY) || (errno == EEXIST)) && + talloc_get_size(lp_veto_files(SNUM(conn))) > 1) { /* * Check to see if the only thing in this directory are * vetoed files/directories. If so then delete them and
Ah, normally in this case we'd use if (*lp_veto_files() != '\0'), it's used that way for other calls that return an empty string from the param code. I'll add this into the completed fix - thanks ! Jeremy.
Arg. Due to conference activity it's really hard to get time to work on this. I'll make this my top priority when I'm back in the office on Fri. April 6th. I should be able to get the master patchset pushed then and also start the 3.6.x back-port. Sorry for the delay. Jeremy.
I wasn't sure about potential security relevance of this problem so I opened the bug to be only visible for the team. Can we lift this limitation or is security affected?
No security issues, you can open this one up.
Ok, tested this patch. Successfully changes the crash to : NT_STATUS_DIRECTORY_NOT_EMPTY removing remote directory file \bla I'll push into master then look at the back-port. Jeremy.
Back port to 3.6.x is proceeding, should be done early next week ! Thanks for your patience. Jeremy.
Created attachment 7442 [details] Back-port 6 part git-am fix for 3.6.next. Ok, here's the 3.6.next version of the fix. Passes tests here (under valgrind). Please test and let me know if you're ok with it ! Cheers, Jeremy.
Comment on attachment 7442 [details] Back-port 6 part git-am fix for 3.6.next. Adding Volker for a second review.
Comment on attachment 7442 [details] Back-port 6 part git-am fix for 3.6.next. Bleegh. Messed up the testing, patch is bad (fails valgind). Wait until I fix (sorry).
Created attachment 7443 [details] six-part git-am patch for 3.6.next. Now (correctly :-) passes valgind. Please test and confirm it fixes the problem in 3.6.next. Thanks ! Jeremy.
Comment on attachment 7443 [details] six-part git-am patch for 3.6.next. Adding Volker for belt-and-braces review. This stuff is tricky :-).
Comment on attachment 7443 [details] six-part git-am patch for 3.6.next. Pretty intrusive, but I don't see a way that is both less intrusive and correct.
Thanks for the review. Re-assigning to Karolin for inclusion in 3.6.next. Jeremy.
Pushed to v3-6-test. Closing out bug report. Thanks!
*** Bug 8936 has been marked as a duplicate of this bug. ***