Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid false negatice permission denied error message (#9046) #12507

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andkit
Copy link

@andkit andkit commented Apr 13, 2024

The unix implementation of cd fails with a permission error when classical unix permissions deny access to a directory, but additional mechanisms like ACLs actually grant the user access

Fix this by miroring the windows implementation: if we can read_dir() without error => allow the cd to proceed.

Addtionally we also handle the case where it is the other way round: unix permissions grant access but additional security modules (SElinux, apparmor, etc.) deny the current user/context.

fixes #9046

The unix implementation of cd fails with a permission error when
classical unix permissions deny access to a directory, but additional mechanisms like
ACLs actually grant the user access

Fix this by miroring the windows implementation: if we can read_dir()
without error => allow the cd to proceed.

Addtionally we also handle the case where it is the other way round:
unix permissions grant access but additional security modules (SElinux,
apparmor, etc.) deny the current user/context.
@sholderbach
Copy link
Member

That strategy of checking for failure instead sounds more sound to me.

Our cd is already not atomic so the existing TOCTOU bug risk doesn't change from the previous implementation but if we can get the check into a more realistic state all the better.

Can you look into which tests now fail?

@andkit
Copy link
Author

andkit commented Apr 15, 2024

Hmm, well, looks like a case of "Damn, not as easy as it appeared at first".

The failing test removes the execute permission from a directory and expects cd to fail. My patch however uses a successful readdir() as indication that a cd should be fine too, but that's not really true either (readdir succeeds, but any actual chdir will fail later, but by that time nushells cd command is long finished).

Doing the operation we actually want (chdir) instead of an approximate opertion is not a good idea either as it messes up the process state (and forking a child just for that seems very inefficient)

I'll have to think about the some more, maybe there is another trick we can try

@sholderbach
Copy link
Member

Thanks for looking into this. That indeed sounds a bit unfortunate. As we may want to spawn external processes from different threads at any time, changing the process working directory seems indeed like a bad idea (if not carefully controlled).

For reference fish can get by with open of the directory fd and then fchdir as they act more posixy in that regard https://github.com/fish-shell/fish-shell/blob/326d986186b206139da1fee4b35091d40b2ed224/src/builtins/cd.rs#L90-L125

(Marking the PR as draft)

@sholderbach sholderbach marked this pull request as draft April 16, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot cd into directory when permissions are granted via ACLs instead of traditional unix permissions
2 participants