Bug 14000 - become_root()/unbecome_root() call pairs should not change directory.
Summary: become_root()/unbecome_root() call pairs should not change directory.
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
Depends on:
Reported: 2019-06-17 16:20 UTC by Jeremy Allison
Modified: 2020-03-23 13:25 UTC (History)
2 users (show)

See Also:

git-am fix for master. (1.37 KB, patch)
2019-06-17 19:58 UTC, Jeremy Allison
no flags Details
WIP patch for master (9.06 KB, text/plain)
2019-06-18 12:12 UTC, Stefan Metzmacher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2019-06-17 16:20:05 UTC
This got introduced as a part of code reuse, which is a good thing :-). However it introduced an additional vfs_ChDir($cwd_fname) in unbecome_root() due to consolidating the restore code in pop_sec_ctx().

This is unexpected behavior (although harmless as it's doing a chdir to where the calling code thinks it already is).

become_root()/unbecome_root() should only change security contexts, not filesystem directory position - it's certainly not expected by callers of the API (and indeed no callers expect or depend on this).

Discussed with Metze at SambaXP. Patch to follow.
Comment 1 Jeremy Allison 2019-06-17 19:58:19 UTC
Created attachment 15253 [details]
git-am fix for master.

Metze, this is what I went through with you at SambaXP.


Comment 2 Stefan Metzmacher 2019-06-18 11:38:57 UTC
Comment on attachment 15253 [details]
git-am fix for master.

I just noticed that we have

        current_user.need_chdir = false;
        current_user.done_chdir = false;

in smbd_change_to_root_user().

I think we should add that also at the end
of smbd_become_root(), which would have the same
effect as your patch.

In addition I'd like to see asserts that we never
call change_to_{user,user_by_fsp,guest,root_user}()
from within a become_{user,root}() section.

And a single become_root() would generate an error
if vfs_ChDir() or smb_vfs_call_chdir() is called
before the matching unbecome_root().

I'll post some draft patches shortly.
global 'bool  that is set by
Comment 3 Stefan Metzmacher 2019-06-18 12:12:12 UTC
Created attachment 15254 [details]
WIP patch for master

Can you have a look at these patches? Having assertions for the
invalid API usage feels be much better (at least for master).

I haven't tested the patches, so maybe there are off by one errors...

Can you fill the TODO's to give useful debug messages, so that it's
easy for us and external vfs module writers to debug things when
they hit an assertion?

Comment 4 Jeremy Allison 2019-06-19 23:48:52 UTC
OK, will be looking over them tomorrow. Might add lots more comments so I can understand exactly what is going on :-).


Comment 5 Ralph Böhme 2020-03-23 13:25:59 UTC
Fixed by the 9 commits leading up to 35cd91ee4d8. Big changes, master only (which became 4.12), no backports.