classification
Title: Add failover for follow_symlinks and effective_ids where possible
Type: behavior Stage: resolved
Components: Versions:
process
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.

Files
File name Uploaded Description Edit
larry.kinder.gentler.follow_symlinks.1.diff 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.
History
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