Title: Add failover for follow_symlinks and effective_ids where possible
Type: behavior Stage: resolved
Components: Versions:
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: larry Nosy List: georg.brandl, hynek, larry, neologix, pitrou, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-06-24 05:06 by larry, last changed 2012-06-24 11:02 by larry. This issue is now closed.

File name Uploaded Description Edit larry, 2012-06-24 05:06 Patch #1 implementing graceful failover for follow_symlinks and effective_ids. review
Messages (4)
msg163713 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-06-24 05:06
Serhiy Storchaka suggested (in private email, not on tracker or python-dev): why not make follow_symlinks and effective_ids failover where possible?

Let's take the example of effective_ids first, that's simpler.  Let's say the user calls
  os.access("x", os.F_OK, effective_ids=True)
But they doesn't have faccessat() for some reason.  IF euid==uid, and egid==gid, then it's harmless to ignore the effective_ids flag and just use normal access().

Supporting this is easy: if effective_ids=True, and !defined(HAVE_FACCESSAT), but we have all four of the get{e|}{u|g}id() functions, do the above comparison and if it is just call access().

It's a bit more complicated with follow_symlinks.  Let's say they call
  os.chmod("x", 0o644, follow_symlinks=False)
As it happens, they're on Linux so they don't have lchmod() and their fchmodat() doesn't support AT_SYMLINK_NOFOLLOW.  But!  "x" isn't a symbolic link!  In this case normal chmod would be fine fine.

How do we detect that the file is a symbolic link?  That's easy, call lstat().  On Windows, if they gave us a wide path, call win32_lstat_w().  If they passed in a non-default dir_fd, call fstatat() (if available).

The one place where we can't fail over gracefully: os.stat()  If we don't have the appropriate native stat function (lstat or fstatat), then obviously we can't stat nofollow the file to see if it's not a symbolic link and call normal stat().  Sad face.

The attached patch implements all of the above.  It passes the regression test suite on Linux 64-bit (with and without pydebug) and Windows 32-bit (Debug and Release).
msg163750 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-06-24 07:59
It also passes OS X.

There are no patch specific tests though. And alas, I don't have any platform at hand that would benefit from these additions. :(

The idea sounds good to me and the code LGTM though. However I can't really say much as I couldn't actually run it.
msg163761 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-06-24 10:31
I don't like this idea. Normally the system calls wrapped by the os module are fairly atomic. Here you're introducing the possibility for potentially nasty race conditions and exploits.
msg163767 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-06-24 11:02
I think you're right.  As Antoine pointed out in irc, for POSIX platforms, modules in os are almost exclusively atomic.  This is a useful (if undocumented) feature from a security viewpoint, and we should not break it lightly.

Closing as wontfix.  If someone thinks it's salvageable and wants to resurrect it, please discuss here.
Date User Action Args
2012-06-24 11:02:57larrysetstatus: open -> closed
resolution: wont fix
messages: + msg163767

stage: patch review -> resolved
2012-06-24 10:31:32pitrousetnosy: + neologix
messages: + msg163761
2012-06-24 07:59:50hyneksetnosy: + hynek
messages: + msg163750
2012-06-24 05:06:12larrycreate