Quotes of the week

Posted Jun 3, 2010 18:20 UTC (Thu) by viro (subscriber, #7872)
In reply to: Quotes of the week by spender
Parent article: Quotes of the week

Learn to read, please. Both your own description and the patch in question.

1) program that blindly does open()/creat() on directory/something, etc. is certainly in for the world of hurt if directory is writable to somebody else and yes, there's a looong list of exploits based on that.

2) in all these exploits shit hits the fan on the final pathname component.

3) if a program is broken enough to attempt such open()/creat() when it can't even rely on _earlier_ steps in pathname resolution, it's _really_ in trouble; it can be attacked in a lot more ways. Fortunately, that tends to be less frequent.

4) posted patch attempts to apply checks to *ALL* symlink traversals, final or not.

5) for the cases that fall under (2), the patch is an overkill and misplaced; the proper place for these checks is in do_filp_open() loop where we deal with trailing symlink case.

6) for really broken cases (i.e. (3)), it's not going to be anywhere near enough, for all the obvious reasons.

In other words, if that patch claims to deal with attacker-controllable symlinks on *ALL* steps of pathname resolution, the checks are insufficient. If it cares only about the attacker-controllable symlinks on the final step, the checks are misplaced.

Should I spell that in further details or would that suffice?

to post comments

Quotes of the week

Posted Jun 3, 2010 19:26 UTC (Thu) by spender (guest, #23067) [Link] (5 responses)

I'm not the original author of the algorithm (nor is Solar Designer I think -- he credits Andrew Tridgell with it), but I don't believe it was ever meant to cover 3). That it does more checks than it needs to is a minor performance/possible compatibility issue (which hasn't cropped up as an issue in a decade or so).

I didn't see any list of references for 3), you admit it's less frequent -- I'd say much less frequent/scarce.

Tossing around "security theatre" and "BDSM fetishism" as descriptors for the feature is unhelpful, especially when your criticism is only that it makes unnecessary checks and doesn't protect against a severe specific brokenness you can't provide examples of (unlike the case it does protect against, which you agree there's a "looong list of exploits" for).

Might I remind you of mmap_min_addr, which is a workaround for a specific case of a larger class of vulnerabilities? That didn't stop the developers from committing it. And unlike your example, I can point to plenty of OOPs reports showing exploitable uses of poisoned pointers, direct userland access in vgaarb, the vmsplice vuln on amd64, etc, that mmap_min_addr would have been helpless against.

If you can come up with something that covers 3) as well, more power to you, but I don't see how (and don't think you can reasonably justify how) it not covering something it wasn't meant to (that you can't provide even a handful of examples of) means it falls under "security theatre."

If you want to talk about actual security theatre going on in the kernel, we can have a completely different discussion about that.

-Brad

Quotes of the week

Posted Jun 3, 2010 23:48 UTC (Thu) by viro (subscriber, #7872) [Link] (4 responses)

Sigh... Do learn to read, would you? _I_ never advocated trying to save (3) from their stupidity; Eric has and IMNSHO he's delusional. I have seen turds of that variety (in the "look what a power-luser has put in crontab" kind of post-mortems), but said turds had been FUBAR anyway.

The whole point is that checks must make sense. Slapping a heuristic that happens to trigger on what one wants to prohibit as well as on a bunch of other use cases and saying "oh, it prohibits more, so it's better by definition" is *not* a good way to get better security.

It's not a matter of overhead; it's not that large to start with. The real issue here is that you generally want the behaviour of system to make sense and its mental model to be understandable. I.e. the rationale for restrictions should be simple and specific; the more arbitrary they are, the worse.

Again, I have no serious objections against that kind of restrictions. open()/creat() and probably truncate() on the final step of pathname resolution. But it really needs to be done right.

BTW, as for the hardlinks... mount --bind /tmp /tmp is much better mitigation for that; you get an equivalent of /tmp being on a separate fs as far as link()/rename() prevention counts without changing the disk layout, etc. Race-free and with less overhead as well. Can be done at the same time when initscripts mount /proc et.al., transparently for all userland. Any software that breaks because of inability to rename() or link() between /tmp and root filesystem is (a) broken on a lot of boxen already and (b) very suspicious, while we are at it.

Quotes of the week

Posted Jun 4, 2010 1:50 UTC (Fri) by spender (guest, #23067) [Link] (3 responses)

Well, checking on following symlinks handles the case of /tmp/a where a is a symlink to a root-owned directory (a *.d directory perhaps) and the application blindly writes to /tmp/a/b. That won't be caught by checking only the last path component. Your example was /tmp/a/b/something, which I've never seen personally. /tmp/a is common, /tmp/a/b is rare, but /tmp/a/b/something where a is created by the non-root user seems like an academic exercise to me, so I wouldn't care if it's not protected (we could ask why /tmp/a/b/c/something, /tmp/a/b/c/d/something isn't protected either, but I don't think it's meaningful).

So I still disagree re: security theatre because it doesn't handle /tmp/a/b/something, but I agree that it should be done correctly with a heuristic based on evidence and not just prohibiting more, arbitrarily. Maybe some people on LKML have suggested such things (I'm not subscribed and the interfaces on lkml.org/gmane are horrible for following this stuff) but I don't feel that what's in grsecurity and Openwall for this feature fits that bill.

-Brad

Quotes of the week

Posted Jun 4, 2010 2:44 UTC (Fri) by viro (subscriber, #7872) [Link] (2 responses)

mkdir /tmp/c
ln -s `readlink /tmp/a`/b /tmp/c/b
mv /tmp/a /tmp/d
mv /tmp/c /tmp/a

will be allowed for attacker and result will have application that tries to open /tmp/a/b hitting exactly what it would hit before that sequence, without triggering the checks. Again, I'm *NOT* saying that we need to try saving such applications from themselves; just that patch as posted doesn't in fact protect them. All it really gives is protection for the final component, assuming that everything prior to that walks through secure places. Which it does, in all practically interesting cases.

BTW, "security theatre" was applied to attempts to handwave the patch through, without bothering with the analysis of what it really changed. "It mitigates the ugly holes we want to have covered, and it's surely better to be more thorough". Note that analysis hadn't been given _anywhere_ upthread on l-k ;-/

Anyway, I'm out of that discussion. Flogging the dead horses is boring and this one is a smear on the ground by now...

Quotes of the week

Posted Jun 4, 2010 12:28 UTC (Fri) by spender (guest, #23067) [Link] (1 responses)

Indeed that works, I mentioned that it only prevented the case where /tmp/a was a symlink. In your case it's a regular directory. If we assume a blind echo "blah" > /tmp/a/b then indeed the check is bypassed. But /tmp/a/b in the symlinked 'a' case isn't always equivalent to the case where 'a' is a normal directory.

Some simple examples that I don't think are too far-fetched:
If the name of 'b' can't be determined a priori or the directory can't be filled exhaustively with all possible names for 'b', then the symlinked 'a' is required.
A program using /tmp/a/b would have some logic for creating the directory if it doesn't exist and additional logic for when it does exist.
If it does exist, the app could do a number of things, two being:
* check the ownership of the directory (using stat())
* enter the directory and remove all the files inside it
In both of these cases the non-symlinked 'a' doesn't allow for an attack. With the symlinked 'a', the ownership of the directory (assuming we're targeting root) would be root, with the normal directory it'd have ownership of the attacker.
If the application goes in and removes all the files in the directory, in the symlinked 'a' case, it removes files in the directory the attacker is targeting. In the non-symlinked case it simply removes the attacker's symlinks.

I imagine we'll have to agree to disagree on the merits of these differences.

-Brad

Quotes of the week

Posted Jun 4, 2010 20:27 UTC (Fri) by vonbrand (subscriber, #4458) [Link]

So it gets to be "Let's save programmers form making dumb mistakes by asking them _try_ the dumb mistake blindly and then do it right if the first try fails"... I don't see the logic behind this.

I'm with Al Viro here: The only solution that fixes all troublesome cases and doesn't litter the landscape with all sort of weird special cases is to forbid symlinks completely. That one hurts way too much. The sane solution is just to disallow hard links to outside {/var,}/tmp... i.e., a separate partition (even tmpfs!) or just mount it over itself. Problem gone, no strange special cases, any sane use for the above behavior still works, no kernel changes.