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

Issue 104 cannot exec setuids #106

Merged
merged 8 commits into from
Dec 21, 2014
Merged

Conversation

jquast
Copy link
Member

@jquast jquast commented Aug 25, 2014

Previously, misinterpreted that os.access(file, X_OK)
always returns True on Solaris. Yes, but only for
the uid of 0. Python issue #13706 closed "not a bug"
reads to "just use os.stat()", so we went to great
lengths to do so quite exhaustively.

But this is wrong -- only when root, should we check
the file modes -- os.access of X_OK works perfectly
fine for non-root users.

And, we should only check if any of the executable bits
are set. Alas, it is true, you may execute that which
you may not read -- because as root, you can always read
it anyway.

Verified similar solution in NetBSD test.c (/bin/test),
OpenBSD ksh for its built-in test, and what FreeBSD/Darwin
for their implementation of which.c.

Tested on OSX and Solaris, travis on Linux, of course.
Manually tested pexpect.which('sudo') on OSX and Solaris,
especially on Solaris as root and user nobody.

Previously, misinterpreted that os.access(file, X_OK)
always returns True on Solaris.  Yes, but only for
the uid of 0. Python issue #13706 closed "not a bug"
reads to "just use os.stat()", so we went to great
lengths to do so quite exhaustively.

But this is wrong -- *only* when root, should we check
the file modes -- os.access of X_OK works perfectly
fine for non-root users.

And, we should only check if any of the executable bits
are set.  Alas, it is true, you may execute that which
you may not read -- because as root, you can always read
it anyway.

Verified similar solution in NetBSD test.c (/bin/test),
OpenBSD ksh for its built-in test, and what FreeBSD/Darwin
for their implementation of which.c.
@takluyver
Copy link
Member

From what I've read recently, it's possible to have an executable but genuinely not readable binary.

```````````
* Fix regression that prevented executable, but unreadable files from
being found when not specified by absolute path -- such as
/usr/bin/sudo (:ghissue:`104`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this conflicts with PR #109, since I merged that. We may want to keep personal scratch files of the changelog, and merge them before release. For IPython, we keep a directory that PRs add snippet files into, and they're merged into release notes by a script, but that may be overkill for pexpect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for long delay, I should be able to address this.

I was aware of generating such conflict but not sure how to address -- as though we needed a header prior to commits, or some base 3.4-prepare branch to base both changes from.

@takluyver
Copy link
Member

+1 for thorough tests. Note that this needs a rebase because of the conflict in the change log.

@jquast
Copy link
Member Author

jquast commented Dec 19, 2014

oi! lost track of this old thing, merging and cleaning up now.

@jquast
Copy link
Member Author

jquast commented Dec 20, 2014

This is ready for review & merge @takluyver, thanks.

@takluyver
Copy link
Member

Thanks, looks good to me.

takluyver added a commit that referenced this pull request Dec 21, 2014
@takluyver takluyver merged commit 0565e6c into master Dec 21, 2014
@takluyver takluyver deleted the issue-104-cannot-exec-setuids branch December 21, 2014 00:46
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.

None yet

2 participants