Hey... On Thu, Feb 07, 2019 at 11:19:26AM -0800, Russ Allbery wrote: > Nick Cleaton <n...@cleaton.net> writes: > > https://github.com/ncleaton/libcallfilt > > > It provides a second line of defense if you've tried to block all of the > > options that could exec arbitrary things but you may have missed > > something. > > Thank you -- that's an interesting option! > > BTW, you probably want to add posix_spawn, posix_spawnp, and execveat. > (Although a nice property of this approach is that it doesn't rely on > finding every possible system call, only covering the ones that the > legitimate program you're trying to spawn might use.)
...assuming the program is legitimate, and hasn't been replaced
somehow by a more nefarious version...
This seems potentially useful, but I'm concerned this is just another
example of something that seems like a good idea in principle (like
rssh), but perhaps depending on the exact use cases, is hard to do
in practice, and Russ hints at some of the why. If someone is serious
about bypassing your access restrictions, they're not going to stick
to doing what a well-behaved program might do. To get this right, in
the general case, you need to know all of the possible ways to execute
a program from system code on all of your target platforms. And you
probably don't. And there are probably platform-dependent ways that
it could be done using inline assembly that would be hard or
impossible for you to block even if you did...
As a quick example, in Linux most system calls can be called
indirectly via the syscall() system call, which AFAICT would bypass
your filter. I'm not sure it's practical to block syscall() as glibc
probably uses it directly, and while it would be uncommon, some system
utilities you might want to use may use it also (I personally have
written code that calls it). There's probably something equivalent on
some other platforms, and it's probably called something other than
syscall()...
You also have to think very hard about what is being executed, and
whether it provides any means for an attacker to upload/execute
arbitrary program code. Having been through that, I can tell you
there are some pretty obscure features of SSH, and of the underlying
OS and its utilities, that make that task very difficult. And new
ones get added over time. So no matter how simple the task might seem
when you start, it's not, and it's never finished.
As for your code... [Some of this will be nit-picky, some will
definitely not be. I think being nit-picky in security contexts is
REQUIRED.]
At a quick glance, I note that you use EACCES, but FWIW I think the
more appropriate errno here would be EPERM. EACCES is usually used
for things like "you don't have appropriate ACL bits to do that"
whereas EPERM is usually more like "Letting you do that could be
dangerous, so I'm not gonna." I suppose it's a judgement call, and
TBH we probably didn't need both to begin with.
Also I note you don't check the return value of setenv(), which could
result in complete bypass of the filter. You also do not check the
return value from your snprintf() calls. It doesn't look exploitable,
but if LD_PRELOAD was already set to something long, and you actually
needed that, it would likely break. That'd be rare and unlikely to be
an issue for most installations, but nevertheless it's an unnecessary
limitation caused by a bug. And you don't cast the return value of
malloc(). Not in and of itself a problem, but you should always
compile security-sensitive programs with -Wall -Werror, so you don't
miss anything stupid, and if you do that I believe this will fail to
compile.
I think the better approach would be:
...
/* don't do expensive malloc unless we need to. Usually we don't need to.
*/
char buf[48]; // Use a macro...
char *mem = buf;
int size = snprintf(buf, 48, ...); // Use the macro, Luke!
if (size > 48){ // Use the macro, Luke!
mem = (char *)malloc(++size);
int s = snprintf(mem, size, ...);
/* impossible, but sometimes impossible things happen! */
if (s > size) {
explode();
}
}
if (setenv(var, mem ...) == -1) explode();
// either don't bother with free(), or set some flag so you know you
// need to. Since you're about to execvp(), it's superfluous.
...
Obviously for brevity, I left out a few of the details you already
included. Some bits of the above (e.g. ++size) might feel more clever
than clear; if so rewrite accordingly. Hopefully it's clear that
explode() is shorthand for "something went wrong; don't continue to do
what the user asked for..."
One last point: I generally recommend(ed) that people use rssh in a
chroot, statically linked, unless they had a good reason not to. One
reason for static linking in security-sensitive applications is to
prevent LD_PRELOAD and similar from being an attack vector, but making
use of that fact also eliminates the potential usefulness of a filter
like this entirely. To that point: If your attackers had any sort of
ability to control what was being executed--intentionally or not--they
could simply execute a statically-linked version that bypasses your
filter.
Security is hard... :(
--
Derek D. Martin
http://www.pizzashack.org/
GPG Key ID: 0x81CFE75D
pgpju9csLtEHA.pgp
Description: PGP signature
_______________________________________________ rssh-discuss mailing list rssh-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rssh-discuss
