3 ms·

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/ca

by 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.

by unwind 10y ago

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.