ipautil.run: Remove hardcoded environ PATH value

[MartinBasti]

This was introduced in commit d0ea0bb
as F14 compatibility. PATH should be always inherited from from
os.environ and then amended also this is platform specific and should not
be in core code

[rcritten]

NACK. I'd be fine with changing the PATH to remove cruft but the primary purpose is to prevent an attacker from providing their own PATH with unknown executables. For those few places where one must control PATH then env can be (and is) passed in.

No ticket?

[MartinBasti]

Can you elaborate more about that attack? Do you have any links to share?
If an attacker has permission to set a user environment variables, IMO the user has already lot of problems and it is too late to save that situation.

I did git archaeology and this was the commit where it was added, so it was hard to find reason why it was added.

[rcritten]

PATH is untrustworthy because there is no knowing what is in it, or the order. It could easily have /usr/local/bin first and some rogue version of a program installed there, or it could have something in ~/bin. Calling exec() is dangerous by its very nature so we opted to be paranoid.

Your archaeology is right, this wasn't exactly documented. Perhaps it was discussed on IRC in relation to the bug but I remember talking to Simo about this.

[MartinBasti]

PATH is untrustworthy because there is no knowing what is in it, or the order. It could easily have /usr/local/bin first and some rogue version of a program installed there, or it could have something in ~/bin. Calling exec() is dangerous by its very nature so we opted to be paranoid.

/usr/bin is untrostworthy in the same way, you dont know if an attacker changed some binary files, should we have fingerprints and check before exec?

AFAIK path is the standard way how to say programs where should check for binarries if they are installed in nonstandard directory

In case that enviroment variables are really considered to be an security risk in a way you are saying, then I have bad news:

• our custom path can be overriden by attacker
• this kind of attack can be currently done directly from python we don't need anything else in IPA, so our ipautil.run() cannot save users
• you can easily DOS a user of IPA

And this should be platform dependent, so we should move path to ipaplatform

Your archaeology is right, this wasn't exactly documented. Perhaps it was discussed on IRC in relation to the bug but I remember talking to Simo about this.

It wasn't documented.
That is not nice if this is a security feature

[rcritten]

This isn't about replacing existing binaries, it's about putting binaries into unexpected places that are in the default PATH (e.g. ~/bin or /usr/local/bin).

PATH cannot be overridden by an attacker without making code changes, in which case it's already game over (or it shouldn't, I didn't look for every execution of ipautil.run() where env is passed in.

I don't disagree on being platform dependent.

As for documentation, it just got missed. It's not an excuse, just the reality.

It is generally accepted best-practice to not trust user input, including environment variables. See https://www.securecoding.cert.org/confluence/display/c/ENV03-C.+Sanitize+the+environment+when+invoking+external+programs

This isn't followed completely, but at least the environment by default is wiped and PATH is controlled for the most part.

Originally the commands were called explicitly, e.g. /usr/kerberos/sbin/kadmin.local, but because of the Fedora 14 issue we had to rely on PATH (see d0ea0bb).

[pspacek]

The approach with wiping env adds another layer of problems, e.g. inability to use KRB5_TRACE environment variable for debugging etc.

IMHO we should use absolute paths whenever we call an external program and let the env be. If an attacker is controling env the game is already over. He could mess with LD_PRELOAD or any other other current or future sensitive variables.

[MartinBasti]

https://fedorahosted.org/freeipa/ticket/6449

[rcritten]

+1 on using absolute paths.

I don't recall any cases where KRB5_TRACE was needed so is this a theoretical use case or an actual one?

Yes, LD_PRELOAD or PYTHONPATH can be tweaked but this just proves my point: the environment is untrustworthy.

[MartinBasti]

Closing this PR, how to handle environment variables must be discussed and designed first.