Bug 8837 - smbd crashes when deleting directory and veto files are enabled
Summary: smbd crashes when deleting directory and veto files are enabled
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.6.3
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
: 8936 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-02 12:47 UTC by Christian Ambach
Modified: 2020-12-11 11:42 UTC (History)
2 users (show)

See Also:


Attachments
Initial attempt at patch for master. (10.93 KB, patch)
2012-04-03 03:42 UTC, Jeremy Allison
no flags Details
Patch for master (11.79 KB, patch)
2012-04-03 03:59 UTC, Jeremy Allison
no flags Details
Back-port 6 part git-am fix for 3.6.next. (22.49 KB, patch)
2012-04-09 20:22 UTC, Jeremy Allison
no flags Details
six-part git-am patch for 3.6.next. (25.22 KB, patch)
2012-04-09 21:18 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Ambach 2012-04-02 12:47:20 UTC
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
Comment 1 Jeremy Allison 2012-04-02 20:33:06 UTC
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.
Comment 2 Volker Lendecke 2012-04-02 20:41:05 UTC
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.
Comment 3 Volker Lendecke 2012-04-02 20:42:03 UTC
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.
Comment 4 Jeremy Allison 2012-04-02 20:45:51 UTC
Can you post the stack backtrace ? That'll make it really easy for me to see.

Cheers,

Jeremy.
Comment 5 Volker Lendecke 2012-04-02 20:49:11 UTC
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.
Comment 6 Jeremy Allison 2012-04-02 23:40:34 UTC
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.
Comment 7 Jeremy Allison 2012-04-03 00:47:24 UTC
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.
Comment 8 Jeremy Allison 2012-04-03 03:42:29 UTC
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.
Comment 9 Jeremy Allison 2012-04-03 03:59:33 UTC
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.
Comment 10 Volker Lendecke 2012-04-03 06:16:02 UTC
(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.
Comment 11 Jeremy Allison 2012-04-03 17:33:50 UTC
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.
Comment 12 Christian Ambach 2012-04-03 20:57:56 UTC
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]
Comment 13 Christian Ambach 2012-04-03 21:00:15 UTC
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
Comment 14 Jeremy Allison 2012-04-03 21:46:28 UTC
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.
Comment 15 Jeremy Allison 2012-04-04 05:05:00 UTC
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.
Comment 16 Christian Ambach 2012-04-04 08:03:30 UTC
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?
Comment 17 Jeremy Allison 2012-04-04 15:55:07 UTC
No security issues, you can open this one up.
Comment 18 Jeremy Allison 2012-04-04 21:23:12 UTC
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.
Comment 19 Jeremy Allison 2012-04-07 00:24:03 UTC
Back port to 3.6.x is proceeding, should be done early next week !

Thanks for your patience.

Jeremy.
Comment 20 Jeremy Allison 2012-04-09 20:22:40 UTC
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 21 Jeremy Allison 2012-04-09 20:23:02 UTC
Comment on attachment 7442 [details]
Back-port 6 part git-am fix for 3.6.next.

Adding Volker for a second review.
Comment 22 Jeremy Allison 2012-04-09 20:55:19 UTC
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).
Comment 23 Jeremy Allison 2012-04-09 21:18:43 UTC
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 24 Jeremy Allison 2012-04-09 21:19:40 UTC
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 25 Volker Lendecke 2012-04-13 19:56:26 UTC
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.
Comment 26 Jeremy Allison 2012-04-13 20:32:34 UTC
Thanks for the review. Re-assigning to Karolin for inclusion in 3.6.next.
Jeremy.
Comment 27 Karolin Seeger 2012-05-07 16:18:56 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!
Comment 28 John Mulligan 2012-05-15 17:13:27 UTC
*** Bug 8936 has been marked as a duplicate of this bug. ***