Hacker News Reader

Comment by unwind | Hacker News Reader

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

View on HN

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.

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.

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.

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

Reply on news.ycombinator.com