Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cobs_decode: fix buffer overflow #25

Merged

Conversation

oreparaz
Copy link
Contributor

A malformed encoding may have the overhead byte pointing to a location outside the buffer contents. This malformed encoding causes an out of bounds memory access. This commit explicitly checks the invalid condition doesn’t happen, and bails if so.

Such malformed encoding will never be produced by a proper encoder, but someone messing with the framer doesn’t need to restrict themselves to feed only compliant encodings.

A malformed encoding may have the overhead byte pointing to a location outside
the buffer contents. This malformed encoding causes an out of bounds memory
access. This commit explicitly checks the invalid condition doesn’t happen, and
bails if so.
@charlesnicholson
Copy link
Owner

Ah great catch! Thanks for the patch; i'm embarrassed that I haven't gotten around to unit-testing cobs_decode at all :-D

You might have noticed that the _inplace flavors don't use pointer math but instead compare indices for out-of-bounds detection. I mean to migrate cobs_encode and cobs_decode to that same style, but haven't had a chance yet.

See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf section 6.5.6/8:

If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined.

So I think your PR (and other parts of my code) are technically UB. I think the only formally portable way to do this work in general is to use indices for testing these out-of-bounds checks.

Anyway, of course your PR improves what's there so I'll gladly take it! Thanks so much for pitching in, I appreciate it!

@charlesnicholson
Copy link
Owner

Oh this is funny, the CI tests that need docker are failing because your "github.actor" obviously can't use my password to pull the docker image?

Could I ask you to try updating your PR to use charlesnicholson instead of ${{ github.actor }} in the .github/workflows/presubmit.yml file? If not I can take a crack at it sooner or later...

@charlesnicholson
Copy link
Owner

Sorry, 6.5.8/5 is more relevant:

When two pointers are compared, the result depends on the relative locations in the address space of the objects pointed to. If two pointers to object types both point to the same object, or both point one past the last element of the same array object, they compare equal. If the objects pointed to are members of the same aggregate object, pointers to structure members declared later compare greater than pointers to members declared earlier in the structure, and pointers to array elements with larger subscript values compare greater than pointers to elements of the same array with lower subscript values. All pointers to members of the same union object compare equal. If the expression P points to an element of an array object and the expression Q points to the last element of the same array object, the pointer expression Q+1 compares greater than P. In all other cases, the behavior is undefined.

@oreparaz
Copy link
Contributor Author

ah great point on UB doing raw pointer arithmetic/comparison, of course! ✅ ✅ ✅ wondering also why the standard makes the exception for the last + 1 element being a non-UB pointer value...

will try changing ${{ github.actor }} to your username in a sec 🤞

@charlesnicholson
Copy link
Owner

Huh, I have no idea what's going on here with the permissions; this is frustrating. I'll just bonk it in and figure it out later :) Thanks again!

@charlesnicholson charlesnicholson merged commit 91088ca into charlesnicholson:main May 19, 2021
@charlesnicholson
Copy link
Owner

Ahhh https://docs.github.com/en/actions/reference/encrypted-secrets makes it look like I need to name the token GITHUB_TOKEN and then it'll be passed to forks. It sure would be nice if that error were explicit?

@charlesnicholson charlesnicholson mentioned this pull request May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants