Exploiting the Linux kernel via packet sockets (Project Zero)

[Posted May 10, 2017 by corbet]

The Project Zero site has a detailed exploration of how to exploit CVE-2017-7308, a vulnerability in the kernel's packet socket implementation. "Let’s see how we can exploit this vulnerability. I’m going to be targeting x86-64 Ubuntu 16.04.2 with 4.8.0-41-generic kernel version with KASLR, SMEP and SMAP enabled. Ubuntu kernel has user namespaces available to unprivileged users (CONFIG_USER_NS=y and no restrictions on [its] usage), so the bug can be exploited to gain root privileges by an unprivileged user. All of the exploitation steps below are performed from within a user namespace."

Signedness bugs in C

Posted May 11, 2017 14:10 UTC (Thu) by epa (subscriber, #39769) [Link] (3 responses)

Does gcc have a set of warnings that would catch signedness bugs like this?

The rules for signed-unsigned promotion and comparison in C have always seemed crazy to me, but surely by 2017 we have compilers that will tell you when you're about to run into them?

Signedness bugs in C

Posted May 11, 2017 16:00 UTC (Thu) by excors (subscriber, #95769) [Link] (2 responses)

I don't think there's any funny promotion or comparison going on here. It looks like the problem is that the code has unsigned int a, b and says "(int)(a - b) <= 0" when it means "a <= b". If e.g. a=0x10000000 and b=0xf0000000, then a-b = 0x20000000 (because of unsigned overflow). (int)0x20000000 is trivially 0x20000000. Unfortunately that is >0, even though a<=b.

The only potentially surprising part is the unsigned overflow, which is well-defined behaviour in C (unlike signed overflow which is undefined).

(On the other hand, if a=0xf0000000 and b=0x10000000 then a-b = 0xe0000000 and (int)(a-b) is implementation-defined behaviour. In practice you'll probably get (int)(a-b) = -0x20000000, which is <=0 even though a>b, but that's probably not a security vulnerability in this context, and anyway it looks like the attacker only controls b.)

I don't see how the compiler could usefully warn about the potential unsigned overflow, without warning about pretty much every single arithmetic operation in the whole program, since it's not smart enough to know the possible values of all the variables. I'm not sure how it could warn about the explicit unsigned-to-signed cast without warning about every such cast. A run-time checker like UBSan wouldn't help since none of the behaviour is undefined. A run-time checker could detect unsigned overflow, but only if you're sure no other code in the kernel intentionally uses the well-defined overflow behaviour.

Signedness bugs in C

Posted May 11, 2017 19:01 UTC (Thu) by epa (subscriber, #39769) [Link] (1 responses)

I should have spoken more carefully - it's the arithmetic rules which are the gotcha. You can't subtract two unsigned quantities, in general, and get a sane result. It's well-defined behaviour, but really quite unhelpful behaviour as the default case. (I'd be very happy to see a modulo-subtraction operator provided by the language, but probably not as the default - operator. But far too late to change it now.)

You're probably right that given the limitations of C there is not much the compiler can do. I think the bounded integer types provided by some languages (such as Ada, I think) would address a large part of the problem. There doesn't seem to be much appetite for them among C programmers, despite decades of bugs such as this one.

Signedness bugs in C

Posted May 16, 2017 4:55 UTC (Tue) by wahern (subscriber, #37304) [Link]

IME, in the context of bounds checking, subtracting unsigned values can absolutely give you sane results. Just like with addition, any overflow will always give you a value larger than either operand. Which means if after each subtraction you either a) check for overflow or b) check your bounds constraint, you're good.

The error here was thinking too much. Secondarily, I'd argue the error was checking for overflow of the operation rather than simply checking for out-of-bounds, which with unsigned values is a single comparison. That's exactly what the final fix does--a simple bounds check against the object size.

I only check for arithmetic overflow, specifically, when doing multiplication or when doing a series of additions or subtractions, and intermediate overflow actually matters. Usually this is limited to allocation, as you're not going to go back and check the malloc'd size against the width, stride, and alignment. In general the simplest and most fool-proof thing you can do is check the bounds at the precise location you dereference. The more abstract rule is that trying to vet and sanitize data is an invitation for bugs. Usually you shouldn't care whether an operation overflowed. Garbage in is garbage out. You usually should only care that garbage data doesn't corrupt the state of your program. Checking for arithmetic overflow is an indirect and often convoluted way to address the real concern, which can be expressed with simpler code. Thus I don't care whether something overflows or how it overflows, only that it does consistently so that my bounds check is sound. Unsigned subtraction gives you that.

I always used unsigned integers (typically size_t, but sometimes uint64_t is called for) when encoding and manipulating data structures in C. Signed arithmetic has no place in memory management code. Signed values are abnormal in the context of data structure memory management. Even when a signed value seems convenient, it's usually not worth the risk because of the cognitive switch necessary analyzing the code. I've often wished that pointer arithmetic in C was unsigned, too, as I think it would simplify writing fail-safe code. That is, often times juggling two pointers (start and end addresses) is more elegant than a pointer and a bound (address and size), but signed operations just don't compose as safely as unsigned operations, and so I often resort to the latter when the former could be clearer and more concise.

Returning to the more general rule about sanity checking: even though the immediate issue was fixed, one of the reasons Linux and other C programs have so many bugs is because programmers try to separate code which sanity checks input from the code that actually uses the input. Often as a performance optimization, sometimes because of an interface deficiency. (Same pathology with SQL and shell injection.) But this is a recipe for disaster. Code locality matters, and encoding rules in two separate blocks of code is an invitation for divergence.