
Cap'n'Proto remote vuln: pointer overflow check optimized away by compiler - fulafel
https://github.com/sandstorm-io/capnproto/blob/master/security-advisories/2017-04-17-0-apple-clang-elides-bounds-check.md
======
cperciva
Amusingly, the "technically-correct solution" is also incorrect, albeit
probably well-behaved on most widely used systems. Rather than comparing
pointers, they cast to uintptr_t and compared those, but as I commented on the
commit in github:

    
    
        This is not guaranteed to work. C guarantees that casting a pointer
        to a uintptr_t and back results in a pointer which compares equal
        to the original; but it does not guarantee that (uintptr_t)p + 1 ==
        (uintptr_t)((char *)p + 1), that p1 < p2 is equivalent to
        (uintptr_t)(p1) < (uintptr_t)(p2), or even that (uintptr_t)(p) ==
        (uintptr_t)(p); an evil but standard-compliant compiler could
        implement casting from pointer to uintptr_t as "stash the pointer
        in a table and return the table index" and casting back as "look
        up the index in the table".

~~~
unwind
Agreed. I don't see any comments on the commit though (assuming "the commit in
github" is the first link in the article, i.e. [https://github.com/sandstorm-
io/capnproto/commit/52bc956459a...](https://github.com/sandstorm-
io/capnproto/commit/52bc956459a5e83d7c31be95763ff6399e064ae4)). Is there
another commit?

Also, I'd be very interested in hearing a suggestion as to how this _should_
be implemented. Of course you might have mentioned that in the comment I can't
find, so don't repeat it if a link suffices of course.

I'd look into working explicitly with uint64_t, and stop doing pointer
subtractions.

 __EDIT __: Here 's the technically correct code from the bottom of the
advisory, that I failed to see: [https://github.com/sandstorm-
io/capnproto/commit/2ca8e41140e...](https://github.com/sandstorm-
io/capnproto/commit/2ca8e41140ebc618b8fb314b393b0a507568cf21).

~~~
cperciva
There's a link at the bottom of the article to "A technically-correct solution
has been implemented in the next commit".

 _Also, I 'd be very interested in hearing a suggestion as to how this should
be implemented_

I don't know enough about the problem space to say for certain, but in general
I'd say that "work with unsigned integers and convert them to pointers only
after performing all necessary sanitization" is good advice.

~~~
unwind
Ah, thanks! Didn't see that. That's some hairy code, for sure. :|

I don't even know if these things are the same in C and C++ any more (the code
in question is in C++).

/me runs back to C.

~~~
cperciva
I wondered about that, so I checked the latest C++ standard. What it says
about uintptr_t is basically "this should do what it does in C".

------
kutkloon7
For people claiming that this is a compiler bug, it really is not.

I am no expert, but what I understood is that the C standard defines some
situations (such as an overflow error) which result in "undefined behaviour".
In this situation, the compiler is free to do whatever he wants. This is in
fact what happens here.

This is clearly annoying to programmers. In this case, it is even hard to
avoid undefined behaviour even if the source of undefined behaviour (a pointer
overflow) is known.

Why is this done? This way the compiler can do some aggresive optimizations
which are only valid if there is no undefined behaviour (e.g. no overflow,
null pointer acces...).

There are various good articles about this.

To make the situation more complex for programmers, it is possible to write
programs which exhibit undefined behaviour but work fine in practice. Until
the compiler tries to do a specific optimization.

This is basically why you shouldn't use C. Long-time C programmers know a list
of operations which are undefined behaviour, and are usually able to point out
a few of these in a program written by newbie (ironically, these programs may
run and pass tests just fine).

~~~
GTP
I'm no expert too, but please read this opinion about the problem [0]. I think
he has a good point saying that undefined behavior != the compiler has the
right to do wathewer it wants. [0]
[http://article.gmane.org/gmane.os.plan9.general/76989](http://article.gmane.org/gmane.os.plan9.general/76989)

~~~
kutkloon7
undefined behavior = the compiler has the right to do wathever it wants

This is _exactly_ what the C standard says. And yes, that is problematic,
especially since there is no way to detect undefined behaviour. I consider
this to be the main problem. If C compilers would simply check for undefined
behaviour in the debug build, there would be a lot less problems.

~~~
vertex-four
The issue there is that you pretty much _have_ to use undefined behaviour in C
and certain low-level applications of C++. C is therefore a broken language
IMHO, but... there we have it.

~~~
_pmf_
That's wrong. All firmware I worked with used assembly for early
initialization (startup.s) in those cases where the only alternative would be
to rely on UB. I've worked with firmware that only worked with -O0 and failed
with -O2, but these were always bad implementations, not due to some inherent
requirement to rely on UB (most common example: failing to recognize that
using volatile does not guarantee a memory barrier).

~~~
vertex-four
> I've worked with firmware that only worked with -O0 and failed with -O2

But as we can see in this vuln, there's functionality that works in the normal
case but where important checks are elided, and compilers might decide not to
do anything with a piece of undefined behaviour in one version but not in the
next. Whether a piece of code works in -O2 doesn't prove it doesn't contain
undefined behaviour.

------
nickm12
This isn't the first security vulnerability caused by optimizing compilers
assuming programs don't exhibit undefined behavior and it won't be the last.
Some relevant literature:

[https://people.csail.mit.edu/nickolai/papers/wang-
undef-2012...](https://people.csail.mit.edu/nickolai/papers/wang-
undef-2012-08-21.pdf)

[http://www.complang.tuwien.ac.at/kps2015/proceedings/KPS_201...](http://www.complang.tuwien.ac.at/kps2015/proceedings/KPS_2015_submission_29.pdf)

~~~
mpweiher
And the claim that this is _not_ a bug in the compiler is laughable. All this
is stuff that used to work. The compiler changed and now removes code. That's
a compiler bug.

~~~
frederikvs
The standard says the compiler is allowed to make that change. The standard
says the programmer writing that piece of code is wrong. So no, it's not a bug
in the compiler, the compiler is complying with the standard.

You could make a case that it's a bug in the standard though :-)

~~~
to3m
It would be complying with the standard if it compiled the whole program into
one single call to abort! And yet it didn't do that. I wonder why not.

~~~
caf
The code in question is not ill-defined over all inputs - it has well-defined
behaviour as long as the offset in question doesn't exceed the size of the
object that it is being used to address.

~~~
lawnchair_larry
Actually, invoking undefined behavior anywhere in the code, even unreachable
code, means the entire program is invalid. Including for all inputs. The
reason it doesn't simply abort is because compiler authors don't go out of
their way to do extra work for no reason.

~~~
zAy0LfpBZLC8mAC
> Actually, invoking undefined behavior anywhere in the code, even unreachable
> code, means the entire program is invalid.

Erm ... nope. If code is unreachable, it can, by definition, not exhibit
undefined behaviour.

> The reason it doesn't simply abort is because compiler authors don't go out
> of their way to do extra work for no reason.

Well, true, but the primary reason is that there are parts of the input domain
for which the behaviour of the code is actually defined, and it's not defined
to mean a call to abort(). The mere possibility to call some code at runtime
with values for which the behaviour is undefined does not make the code's
semantics completely undefined.

------
azinman2
It'd be nice if there was a way that when compilers "re-write" or eliminate
parts, those could be highlighted or otherwise nicely conveyed in our IDEs.

I don't know why we expect phones and apps to have modern great UX, solving
all kinds of problems, but our text editors should remain essentially the same
as in the 70s.

~~~
chrisseaton
The 70s era bit of this is the thinking that compilers 'sometimes re-write
parts' of your program. Really they completely break your entire program down
and then build it up from the ground again. Everything would show up as having
been rewritten.

~~~
johncolanduoni
Most C/C++ compilers support outputting line number information even for
optimized code (DWARF for example has a specific mechanism for identifying
inlined code in stack traces). That's more than enough location data to
provide something useful.

~~~
MaulingMonkey
Seeing e.g. the original source in the disassembly window is useful. And the
same info lets you do things like:
[https://gcc.godbolt.org/](https://gcc.godbolt.org/)

Taking that information into any sort of "yeah, this still represents the
original intent of the author" highlight/lack of highlight/summary as it
sounds like azinman2 wants might be a little more difficult. (After all, if
you can implement that, why not have your compiler only emit 'sane' code in
the first place?)

------
jononor
We need a -Wundefined-behavior, which can be used in combination with -Werror.
It is a practically impossible job to be so aware of the standard while
working to get this right without assistance.

~~~
masklinn
> We need a -Wundefined-behavior, which can be used in combination with
> -Werror.

The problem is that you probably won't be able to use any optimising compiler
if you do that. As a result of a bunch of optimisation (mainly inlining) the
compiler "uncovers" lots of UBs which it can use to pare down the code, that's
why inlining is one of the most important intermediate/early optimisation.

Here's an old example by mikeash:

    
    
        int ComputeStuff(int *value) {
            if(value == NULL) {
                long and complex computation for a NULL value
                return result
            } else {
                long and complex computation using the data pointed to by value
                return result
            }
        }
    
        void DoStuff(int *value) {
            int pointedTo = *value; // value *must* be non-NULL
            // do some work with pointedTo
            int computedResult = ComputeStuff(value);
            // do some more work with whatever
        }
    

Here the compiler can inline ComputeStuff, at which point it can rely on UB to
assert that half the inlined code is dead and can remove it. What would it
even do on "-Werror -Wundefined-behavior", fault on the original DoStuff? On
the inlined one? On both?

Would -Wundefined-behaviour forbid assuming a pointer is non-null ever,
requiring (and outputting) a check before each use of a pointer in a given
scope/lifetime?

~~~
jononor
For such inlining optimization it is the compiler that 'introduces' the UB in
its intermediate step. That is not a fault of the input code, so no warning
should be emitted?

If the compiler can know that the pointer is non-null, why would it need to
rely on UB to optimize it? Problem I guess is the ever-present possibility of
pointer aliasing and such nearly allowing anything to change anything else at
any time... Not easy making things more sound on such a foundation.

~~~
derf_
_If the compiler can know that the pointer is non-null, why would it need to
rely on UB to optimize it?_

You've got this backwards. The compilers "knows" the pointer is non-null
because you dereferenced it, and dereferencing null would have been undefined
behavior. That's what allows the compiler to assume that didn't happen.

There are many operations which will produce undefined behavior for some
inputs (e.g., every pointer dereference, every signed integer arithmetic
operation). Figuring out if it is possible for a program to encounter such
inputs _at runtime_ is equivalent to the halting problem.

This is why any attempt to craft a -Wundefined-behavior that works on non-
trivial programs is doomed to failure. It _may_ be possible to prove a program
will definitely invoke undefined behavior in some useful subset of cases.
Compilers already do this in some of them, even. But those cases (and many
more, besides) would also be caught with ubsan and a unit test.

~~~
jononor
You're right, it is an intractable problem in general. I guess just use Rust
if one cares about this..

------
lmm
I thought sandstorm was supposed to be a security-focused project? But they're
writing new C code and connecting it to the Internet. And surprise surprise,
this happens.

Fuzzers will catch a certain proportion of this type of problem, sure. But for
a ground-up project like this there's really no excuse not to use a better
language.

~~~
kentonv
Dynamic languages are susceptible to their own kind of security bugs. I've
found lots of bugs in Node apps where they accepted some JSON from the client
and then passed it directly into a Mongo query without type-checking it. Or
look at Ruby's YAML security issues.

Nearly every language is vulnerable to integer overflow. C++ is one of the few
languages (possibly the only popular language) where you can reasonably check
for overflows at compile time, as Cap'n Proto now does:
[https://capnproto.org/news/2015-03-02-security-advisory-
and-...](https://capnproto.org/news/2015-03-02-security-advisory-and-integer-
overflow-protection.html)

So, I don't accept the assertion that C++ is inherently a security problem.

In any case, Sandstorm's low-level container management bits pretty much had
to be written in C/C++ since they interact closely with the operating system.
Or if we were starting over today, Rust might now be an option, but it wasn't
when we started.

~~~
educar
I would love to see capnproto rewritten in rust. Is there an ongoing project
for this?

~~~
kentonv
Cap'n Proto has a high-quality Rust implementation:
[https://github.com/dwrensha/capnproto-
rust](https://github.com/dwrensha/capnproto-rust)

------
m_eiman
Is there a way to make the compiler list the applied optimizations for a
specific function, in e.g. the .lst output? Would be very helpful when the
compiler is doing something creative and you can't figure out how to make it
do what you intended.

~~~
adrianN
You could start by not relying on undefined behavior ;)

~~~
mpweiher
Compiler writers could/should start by not treating undefined behavior as
blank checks.

This is so idiotic, and no, it's not a bug in Cap'n'Proto, it is very
definitely a bug in the compiler (and/or the current version of the C spec. if
it actually allows this).

~~~
unwind
So rewriting code in order to "do what I mean" makes sense, since not doing
what you mean is a bug in the language itself?

Not sure that's a very productive approach. The spec is what it is, and it's
of course rather deliberately done the way it is in order to make it possible
for compilers to optimize and extract more performance.

~~~
mpweiher
> "do what I mean"

Well, the code is right there, there is no magic "do what I mean" needed. Just
don't magically remove code that I wrote.

> spec is [..] rather deliberately done the way it is in order to make it
> possible for compilers to optimize and extract more performance

Yes, and it is wrong. Breaking existing programs with magic that simply
removes code that is clearly there in order to eke out a bit of performance is
wrong.

~~~
staticassertion
> Just don't magically remove code that I wrote.

Compile with O0.

> Yes, and it is wrong. Breaking existing programs with magic that simply
> removes code that is clearly there in order to eke out a bit of performance
> is wrong.

Well welcome to writing C? It's been this way for a long time and that won't
change - UB is critical to performance and doesn't always lead to unsafety. If
you're programming in C, don't blame the compiler when your UB leads to remote
code exec, you should have known what you were getting into.

~~~
mpweiher
> Well welcome to writing C?

I've been writing C for 30 years.

> UB is critical to performance

Not really, no. See

"What every compiler writer should know about programmers or “Optimization”
based on undefined behaviour hurts performance"

(referenced elsewhere in this thread)

[http://www.complang.tuwien.ac.at/kps2015/proceedings/KPS_201...](http://www.complang.tuwien.ac.at/kps2015/proceedings/KPS_2015_submission_29.pdf)

~~~
staticassertion
C optimizers _have_ and _will_ optimize around UB. Feel free to call it a bug
in C, the compiler, or the programmer, or the code - when you write C, when
you use a common compiler, you're asking for vulnerabilities.

Thanks for the paper. It's very interesting but I think it has no real impact
on my initial statement - these optimizations aren't going away, you can
continue to expect problems like this vuln in the future for the same reasons.

~~~
mpweiher
Having something wrong be or become reality doesn't change it being wrong.

~~~
staticassertion
Ok... so what? I don't care if you blame the compiler or the spec, and no one
else should either - the end result is the same, vulnerable code.

------
willvarfar
C/C++ coding standards I've seen always warned against using unsigned to mean
"cannot be negative", because of precisely this kind of bug.

The standards I'm recalling were in-house standards, but a quick Google shows
that Google have published their standard and they warn against unsigned too:
[https://google.github.io/styleguide/cppguide.html#Integer_Ty...](https://google.github.io/styleguide/cppguide.html#Integer_Types)

Bottom line: avoid unsigned for numbers. Use unsigned only for bags of bits.

~~~
esrauch
I think the situation is a lot more nuanced than that. The issue was that
additition overflow is undefined behavior for both signed ints and pointers
(but it is defined for unsigned ints).

The fix commit changes it to use an unsigned integer type instead, so it seems
like a concrete example for the exact opposite of your suggestion.

------
banachtarski
There is no undefined behavior here? Unsigned overflow wraps around and this
is correct. Always has been in C++ for as long as I can remember.

~~~
kentonv
Unsigned overflow wraps around, but signed and _pointer_ overflow are UB. In
this case, the code was depending on pointer overflow wrapping around.

------
raverbashing
I'm going to be honest here, whenever I read things like this more I'm
convinced C belongs in the trash can

I would accept that pointer overflow is implementation dependent. Allowing it
to be "undefined behaviour" is sincerely BS.

Every processor has a "pointer register". Increment it until it overflows.
What happens then? Most of the time it will roll over. I suspect this happens
in even early RISC architectures that faint if you look at it too hard.
There's your answer

It's right, it's not a compiler bug. C tries to solve all problems and cops
out on the real issues. The language is broken.

~~~
banachtarski
No, it's definitely a compiler bug here.

------
jbb67
This is a broken compiler. It might well meet the "standard", but it's still
broken because it doesn't do what any reasonable person would expect it to do.

~~~
kutkloon7
Which means the standard is broken, not the compiler.

