Re: a utility for preventing exec() calls
Brought to you by:
xystrus
From: Derek M. <co...@pi...> - 2019-02-08 01:38:33
|
Hey... On Thu, Feb 07, 2019 at 11:19:26AM -0800, Russ Allbery wrote: > Nick Cleaton <ni...@cl...> 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 |