5 ms·

Amusingly, the "technically-correct solution" is also incorrect, albeit probably well-behaved on most widely used systems. Rather than comparing pointers, they

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 10y ago
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/52bc956459a5e83d7c31be95763ff6399e064ae4 https://github.com/sandstorm-io/capnproto/commit/52bc956459a...). 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/2ca8e41140ebc618b8fb314b393b0a507568cf21 https://github.com/sandstorm-io/capnproto/commit/2ca8e41140e....
- cperciva 10y ago
  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 10y ago
    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 10y ago
      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".
- madmoose 10y ago
  For the simplified example given in the CVE: word* target = segmentStart + farPointer.offset; if (target < segmentStart || target >= segmentEnd) { throwBoundsError(); } doSomething(*target); Here's how I'd do it. (Caveat emptor, not knowing all the relevant types in the example, etc.) size_t segmentLength = segmentEnd - segmentStart; if (farPointer.offset >= segmentLength) { throwBoundsError(); } word* target = segmentStart + farPointer.offset; doSomething(*target); In general you can never compare pointers unless they point into the same array or object. In fact, even creating an invalid pointer is UB, you don't event have to compare or dereference it.
rbehrends 10y ago
A concrete example would be the Transputer architecture. Transputers had signed addresses (from -2^(k-1) to 2^(k-1)-1). The null pointer was in the middle of the address space (which already wasn't without problems) and an unsigned comparison would have created a wrong ordering of addresses. The compiler for such a machine does not even have to be evil.
- kentonv 10y ago
  The unsigned comparisons in this code snippet would work fine as long as the object bounds didn't cross the negative-positive boundary (i.e., zero), which it couldn't because the C/C++ standards require that zero is not a valid address for any object. So, I don't think there's a problem with the architecture you describe. (That said, as mentioned elsewhere, we're talking about a debug check in a non-security-sensitive code path...)
  - lomnakkus 10y ago
    > the C/C++ standards require that zero is not a valid address for any object. So, I don't think there's a problem with the architecture you describe. IIRC it's a bit more complicated than that...? Isn't it more along the lines that no object may occupy the NULL/nullptr 'address' and that 0 (literal) in a pointer context must be interpreted as NULL/nullptr? IIRC it doesn't specifically say anything about the address 0x00000000 (add bits to taste).
e12e 10y ago
That's not readily readable on a cellphone, unfortunately - this is: > 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".
kentonv 10y ago
Hi Colin, This line you've commented on is a debug assert, meant to catch a bug that used to exist in the code but doesn't anymore. This check is not required for security and could just as well have been deleted altogether. It is compiled out of opt builds.

unwind 10y ago

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/52bc956459a5e83d7c31be95763ff6399e064ae4 https://github.com/sandstorm-io/capnproto/commit/52bc956459a...). 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/2ca8e41140ebc618b8fb314b393b0a507568cf21 https://github.com/sandstorm-io/capnproto/commit/2ca8e41140e....

cperciva 10y ago
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 10y ago
  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 10y ago
    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".
madmoose 10y ago
For the simplified example given in the CVE: word* target = segmentStart + farPointer.offset; if (target < segmentStart || target >= segmentEnd) { throwBoundsError(); } doSomething(*target); Here's how I'd do it. (Caveat emptor, not knowing all the relevant types in the example, etc.) size_t segmentLength = segmentEnd - segmentStart; if (farPointer.offset >= segmentLength) { throwBoundsError(); } word* target = segmentStart + farPointer.offset; doSomething(*target); In general you can never compare pointers unless they point into the same array or object. In fact, even creating an invalid pointer is UB, you don't event have to compare or dereference it.

rbehrends 10y ago

A concrete example would be the Transputer architecture. Transputers had signed addresses (from -2^(k-1) to 2^(k-1)-1). The null pointer was in the middle of the address space (which already wasn't without problems) and an unsigned comparison would have created a wrong ordering of addresses. The compiler for such a machine does not even have to be evil.

kentonv 10y ago
The unsigned comparisons in this code snippet would work fine as long as the object bounds didn't cross the negative-positive boundary (i.e., zero), which it couldn't because the C/C++ standards require that zero is not a valid address for any object. So, I don't think there's a problem with the architecture you describe. (That said, as mentioned elsewhere, we're talking about a debug check in a non-security-sensitive code path...)
- lomnakkus 10y ago
  > the C/C++ standards require that zero is not a valid address for any object. So, I don't think there's a problem with the architecture you describe. IIRC it's a bit more complicated than that...? Isn't it more along the lines that no object may occupy the NULL/nullptr 'address' and that 0 (literal) in a pointer context must be interpreted as NULL/nullptr? IIRC it doesn't specifically say anything about the address 0x00000000 (add bits to taste).

e12e 10y ago

That's not readily readable on a cellphone, unfortunately - this is: > 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".

kentonv 10y ago

Hi Colin, This line you've commented on is a debug assert, meant to catch a bug that used to exist in the code but doesn't anymore. This check is not required for security and could just as well have been deleted altogether. It is compiled out of opt builds.