Hacker News Reader

Comment by lstamour | Hacker News Reader

I can see how the original CVE patch removed using the current working directory for loading plugins, but it seems like less of a security issue to simply look up one directory relative to the current binary install, assuming the binary is stored in ./bin after being built and plugins would be stored in ./shared ... Now, it might be inconsiderate to look up a folder when one could be preconfigured based on a common root or home folder, but it doesn’t necessarily seem as bad to trust the parent folder of a binary as trusting the current working directory’s contents?

View on HN

This isn't relative to the binary install, it's relative to argv[0]. It's the same security issue -- that argument is under user control. You can make a symlink to your target wherever you want and then argv[0] will be the symlink location. I don't think there is any secure way to get the current script path in Python.

so you install dstat in /usr/bin/dstat + /usr/share/dstat (because you are root), and an attacker creates /home/eve/bin/dstat with /home/eve/share/dstat/evil.py. why would you run /home/eve/bin/dstat? if eve can get you to run dstat from here ~/bin, why wouldn't she just have ~/bin/dstat with completely different contents?

i'm still convinced this is cargocult security.

You're right that this wouldn't matter if the program is run with the same privileges as the caller. I was imagining if that wasn't the case (e.g. the user gives dstat setuid permissions), then letting it load arbitrary code could produce a security hole. Admittedly I didn't think too hard about whether/why something like this might be done -- if it wouldn't be, then never mind. As a matter of general safe practice I err on the side of caution, i.e. not depending on argv[0] to have any particular value for the correctness of the program, because otherwise you have to think through all the possible attack/error scenarios and likely document them for the user, etc... and I thought in any case it was worth pointing out that argv[0] did not necessarily correspond to the program location regardless.

Edit/Update: You're right; I'm wrong -- I don't think this is the concern. Namely, I completely missed that the bug was already there in another line, and that the home directory was also being scanned. So it can't be a privilege escalation issue. Moreover I forgot that setuid doesn't work for scripts, so that shouldn't be the issue either. Not sure what's going on now either; I'm confused too.

Would something based on __file__ be more preferred, then?

If so, it could be argued the bug already existed and this patch simply uses the existing behaviour...

I don't think __file__ would be any different... it's not even an absolute path necessarily. That's why I said I don't think there's any secure way to do this in Python.

You're right though, I didn't see the issue was still there. The home directory should also not be there either. I think I looked at this too hastily but now I'm confused too.

    os.path.realpath(__file__)

That will give you the absolute path of the current python script, dereferencing any symlink and the .. notation.

It will also be insecure due to a race condition.

How do you arrive at the conclusion that looking at `./plugins/` is bad, but `../shared/dstat` is more acceptable? At first blush, the attack vector would seem to be exactly the same. Adding the extra layer of indirection does not seem, to my eyes, to add any significant extra layers of security. Both seem essentially untrustable from the perspective of the binary. Can you help me understand what I've missed in your explanation?

I can also see where someone might not want to endlessly re-debate an old security decision.

Sure. The original security bug was lines 31-32, but the new patch referring to the parent directory is a variation of line 30, untouched here: https://bugs.gentoo.org/attachment.cgi?id=210509&action=diff

CWD is far worse a risk, because it changes every time the command is run. By comparison it could be assumed that a secure install of the software is in a trusted location, and this just gets the parent root of that location. Of course, it’s better if it’s documented, but I see no difference between this and $JAVA_HOME’s approach of ./bin, ./share, etc. It could be argued this is better—no environment variables can override the default. If you install the app to /tmp/bin that’s your own fault...

Ah! Thank you for clarifying.

That said, going up a directory would seem to imply that dstat could easily be loading completely arbitrary code from a directory it has no exclusive claim to and thus cannot meaningfully trust. Indeed, as I understand `share/` that is exactly the point. This really doesn't strike me as being a safe thing to do, and I can readily see why the author would reject it on security grounds. This seems to me to be a distinction with little meaningful difference, though I can see where some people might disagree.

As I tell PMs, designers, and engineers I work with far too often for my own comfort, someone else's poor security decisions are no excuse for our own.

If absolute share/ access were the problem, the line below wouldn’t hard-code it twice: https://github.com/dagwieers/dstat/pull/50/files

The only bug in this PR that I can see is it might assume the system plugins are more important than the local overrides. Generally you’d put this sort of path in the least priority, to allow for /usr/local to override. That said, obviously hard-coding paths is worse than simply having a default, presumably relative to the current binary/file, and allowing end users and packagers to override/configure the defaults to suit their security preferences, with the default being that the entire package is unzipped with expected relative paths to files... or that’s how I would look at it. The only way to greater security would be to ship a Docker container, Snap package, or similar and mount your own filesystem overlays. :) Or, perhaps keeping a trusted list of plugins somewhere.

My read is that it's more that access control of `../share` in an unknown part of the filesystem is a matter for some concern. Given that the binary can be put basically anywhere, it would seem to be perilously close to CWD.

no, it's just cargocult security.

edit after your reply: if you install in /tmp, you'll end up with /tmp/bin/dstat and /tmp/share/dstat. you're concerned that an attacker could smuggle something into /tmp/share/dstat, but /tmp/bin/dstat is of no worry? what exactly is the threat here?

> access control of `../share` in an unknown part of the filesystem is a matter for some concern. Given that the binary can be put basically anywhere, it would seem to be perilously close to CWD.

another edit since i cannot reply to you: do you have any examples of the "threat model [which] includes that you can't trust every part of the filesystem you're working from"? something concrete, specific. a particular install prefix that would let you create $prefix/bin/dstat but $prefix/share/dstat would be dangerous.

aaand, see my reply at https://news.ycombinator.com/item?id=19989237

I don't know. If your threat model includes that you can't trust every part of the filesystem you're working from, being extra paranoid about it isn't the most unreasonable thing in the world.

EDIT: A sibling points out the issue in more detail - https://news.ycombinator.com/item?id=19989237

Reply on news.ycombinator.com