5 ms·

I submitted this PR long time ago: https://github.com/dagwieers/dstat/pull/50 https://github.com/dagwieers/dstat/pull/50 that fixed an important installation is

by certik 7y ago

I submitted this PR long time ago: https://github.com/dagwieers/dstat/pull/50 https://github.com/dagwieers/dstat/pull/50 that fixed an important installation issue, and it took the author 5 months to look at it and simply close it without providing any hints how to fix the problem in some other way. So I was discouraged to contribute further to the project.

rurban 7y ago
You added a security problem. The fix is obvious for everybody working in security, but it's questionable if it should be discussed there.
- lstamour 7y ago
  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?
  - Kalium 7y ago
    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.
    - lstamour 7y ago
      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 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...
      - Kalium 7y ago
        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.
        lstamour 7y ago
        If absolute share/ access were the problem, the line below wouldn’t hard-code it twice: https://github.com/dagwieers/dstat/pull/50/files 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.
  - mehrdadn 7y ago
    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.
    - contras1970 7y ago
      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.
      - mehrdadn 7y ago
        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.
      - mehrdadn 7y ago
        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.
    - 7y ago

by certik 7y ago

rurban 7y ago

You added a security problem. The fix is obvious for everybody working in security, but it's questionable if it should be discussed there.

lstamour 7y ago
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?
- Kalium 7y ago
  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.
  - lstamour 7y ago
    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 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...
    - Kalium 7y ago
      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.
      - lstamour 7y ago
        If absolute share/ access were the problem, the line below wouldn’t hard-code it twice: https://github.com/dagwieers/dstat/pull/50/files 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.
- mehrdadn 7y ago
  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.
  - contras1970 7y ago
    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.
    - mehrdadn 7y ago
      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.
    - mehrdadn 7y ago
      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.
  - 7y ago