How Torch broke ls and made me vulnerable | Hacker News Reader

How Torch broke ls and made me vulnerable

joshumax.github.io

183 pointsjoshumax36 comments

Torch probably doesn't even need to set LD_LIBRARY_PATH. If LD_LIBRARY_PATH is only being set so that binaries distributed by torch work, then I'd strongly suggest they use RUNPATH instead with $ORIGIN.

There are examples in various places:

https://enchildfone.wordpress.com/2010/03/23/a-description-o... http://man7.org/linux/man-pages/man8/ld.so.8.html http://longwei.github.io/rpath_origin/

LD_LIBRARY_PATH is really only for a developer's local use; it should never be used for installed software.

Disclaimer: may not apply in some scenarios, I haven't used Torch, so this is merely a general observation.

This nice and clean solution is too little known I think. Far better than shipping shell scripts which are very hard to get right and most application developers are not shell scripting experts.

RPATH is very nice, but it's a huge pain to set in Makefiles because you have to reliably escape the "$ORIGIN"

(Don't do this) You could patch the binary after compilation with elfpatch.

Also,

test:

  echo "\$$ORIGIN"

outputs $ORIGIN. $$ translates to a literal dollar sign. \ escapes the dollar sign in the shell.

You can use elfedit instead, but really, it's not that much of a pain to just do it right in the first place.

I've wrestled with this in the past, and I eventually got things working to my satisfaction by reading the man page, but these blog posts bring a lot of clarity. Thanks for linking them!

How to safely prepend a directory to a PATH-like variable (in any POSIX-compliant shell):

export LD_LIBRARY_PATH=/opt/whatever/lib${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}

Pull request sent to https://github.com/torch/distro/pull/228.

Good fix and good for sending them a merge request.

I still find it kinda baffling glibc would have this behavior for a trailing colon (:). Like, I know it's probably legacy/comparability, but it feels like a security nightmare. ./ should be explicit, not implicit.

Also for leading colons. (but you probably knew that)

More concerning to me is that ld.so will interpret a trailing `:` in LD_LIBRARY_PATH to mean to include PWD.

Where is this documented? It's not indicated in ld.so's manpage:

http://man7.org/linux/man-pages/man8/ld.so.8.html

Sounds like a bug in GNU's ld.so more than anything.

It is documented in the elf specification (however I agree it should be documented or preferably excised from ld.so)

http://www.sco.com/developers/gabi/latest/ch5.dynamic.html#s...

The dynamic array tag DT_RUNPATH gives a string that holds a list of directories, separated by colons (:). For example, the string /home/dir/lib:/home/dir2/lib: tells the dynamic linker to search first the directory /home/dir/lib, then /home/dir2/lib, and then the current directory to find dependencies.

The following values would be equivalent to the previous example:

LD_LIBRARY_PATH=/home/dir/usr/lib:/home/dir2/usr/lib:

Note that this happens anytime you mistype a variable, e.g.

    export FOOBAR=/usr/local/lib
    export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$FOOBAL

So yeah, fix the source of the problem in ld.so, don't blame it on Torch.

Even if it was clearly documented, it's still a terrible idea. The OS should try to make it difficult, ideally impossible, to accidentally introduce vulnerabilities.

> Sounds like a bug in GNU's ld.so more than anything.

It's neither unique to glibc (AIX, Solaris) nor to LD_LIBRARY_PATH (PATH), nor trailing colons (leading colons, adjacent colons).

This de facto standard becomes a little more obvious when one considers a likely implementation (iterating over "strchr(arg, ':')" or whatever). Any of these sequences then will give up an empty string:

    PATH=:/foo
    PATH=/foo:
    PATH=/foo::/bar

And an empty string is equivalent to dot for chdir(2).

    zwp:/tmp$ cd ''
    zwp:/tmp$ pwd
    /tmp
    zwp:/tmp$

(This is not the same as plain "cd" (ie with no args), which is a special case that takes you $HOME, of course).

I agree it's surprising and potentially dangerous.

FWIW, the execp() functions hide a similar wtf. From the Linux man page:

    The file is sought in the colon-separated list of
    directory pathnames specified in the PATH envi‐
    ronment  variable. If this variable isn't defined,
    the path list defaults to the current directory
    followed by the list  of  directories returned by 
    confstr(_CS_PATH).

Security conscious programs that clear the environment and then call eg execlp() end up searching dot before the system path. Yay.

Buried in there is "Torch machine learning library", which then allowed me to figure out what software was being blamed. http://torch.ch/

At least you didn't read "ls " as "is" with a capital "i"...I only read it it to un-confuse myself!

Same. I was irritated enough by the badly formed headline to read the story to try and figure out what was going on.

What's wrong with something like, "Torch machine learning introduces vulnerability in loading of shared libraries." Whilst it doesn't tell the whole story it does at least give a flavour instead of just sowing confusion.

On most modern Mac OS installations, this is a non-issue. System Integrity Protection doesn't honor any changes to LD_LIBRARY_PATH, presumably for exactly this sort of reason. (Of course, one might have turned off SIP, in which case this is no longer true, but it's nice to know it's the default).

Very sensible. I wish OS X featured a even more "rootless" mode where "root" only gives you sandboxed write access, and read access to files it creates.

on a similar note, this (having '.' or $(pwd) on LD_LIBRARY_PATH) also broke the `ls` command (and a bunch of other stuff) in the TeamWin Recovery Project (TWRP) recovery on mobile devices when you were in `/system/lib` on a 64-bit machine.

https://github.com/omnirom/android_bootable_recovery/commit/...

you know, that's a pretty good plausibly-deniable backdoor, if you think about it...

sounds like a pretty good thing to disable in ld.so...

How is it a backdoor? System services don't typically source the user's bash profile before running, and even if they did, they don't run from attacker-controlled directories anyway. At best you could compromise someone by tricking them into cd'ing into a folder you provided, but that's not something that would generally be called a "backdoor". And if you can get them to run your install script, you've already "compromised" them anyway and LD_LIBRARY_PATH is completely unnecessary.

with several browsers happy to automatically download whatever is provided to them, making a user download a libblahblah.so is relatively easy, and unless the user was aware of the ramifications of just even having such a file in a folder, might ls, cat, grep, etc in that folder like they do any other day

However, most of the latest versions of the mainstream browsers now intentionally avoid writing out such files to the downloads folder unless the end user specifically OKs it (due to situations like this!)

> And if you can get them to run your install script

If your script is obviously malicious then you're reducing your chances. Such a change could seem innocuous[0], then, cloning a repo containing a so file in the middle of a long list and cd'ing would trigger payload execution. Distributing the maliciousness by chaining innocuously looking actions is both effective at bypassing human logical analysis and plausibly deniable (up to a point).

[0]: http://underhanded-c.org

If you clone a repo and cd into it, that's because you're going to actually do something in there. An install script that clones a repo, cd's into it, and then does nothing is extremely suspicious. But a script that clones a repo, cd's into it, and runs `make install` isn't particularly suspicious, so once again, there's no need for LD_LIBRARY_PATH.

It's a plausibly-deniable backdoor on top of LD_LIBRARY_PATH, just to clarify. As in - oops, I added the colon at the end, or if the LD_LIBRARY_PATH was programmatically generated, a sloppy loop can clearly do that...and now you're stuck with unexpected behavior which, while not directly unsafe, can be coopted in a nefarious manner. The $PWD behavior is niche, unexpected and likely to be missed in an audit. If you have software that relies of LD_LIBRARY_PATH and you distribute the OS, I'd change ld.so to avoid the $PWD behavior.

It's not plausible.

It's a very unreliable attack vector, but what makes it completely pointless is in order to even set it up, you already need permission to edit the user's bash profile, and if you can do that, then you don't need LD_LIBRARY_PATH.

Why is torchs install so weird? Why can't it use standard package management, and why does it need to install to my home directory? I'm not surprised to see it causes security issues.

So I'm less concerned about Torch and more concerned about GNU ld adding things unexpected. This sort of "magic" shouldn't occur.

It's not specific to GNU ld, it is standard UNIX behaviour. Always has been.

But you're right it should not occur.

Seems like this may almost have been better done through a disclosure channel with torch?

Indeed, first thing alert the devs and let them sort it out..

Maybe, but this particular issue has much wider scope, and is only incidentally a Torch issue. A disclosure by the Torch devs might have gone unnoticed by those who are not Torch users - I only read it because the HN title mentioned ls, and I thought "that looks odd...".

wow! thats a gotcha, trailing : appends $(PWD) to the LD_LIBRARY_PATH

Thanks for sharing!

Ha, shitty text-based configuration systems strike again. Ask yourself if this could have happened with Windows 10's PATH editor.

> Ask yourself if this could have happened with Windows 10's PATH editor.

Yes …? Well, I dunno; I don't know how Windows 10's PATH editor works. Nonetheless, the issue seems to be with magic interpretation of special configuration options, not with how those configuration options are entered. (Note also that the configuration was done programmatically, not by the user, so that there would have to be some kind of parse–deparse step anyway.)

Reply on news.ycombinator.com