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

Relax decode bounds check #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wemeetagain
Copy link
Contributor

In #20, a check was made to prevent maliciously crafted input from resulting in NaN or Infinity decoded values.

The check only allows varints with bitlength <= 48 to be decoded.
This is strict, perhaps overly strict. It disallows this library to decode varint64, an extremely common usecase.

But this leads us to a difficult situation without a deeper and more invasive code change -- a tradeoff between allowing a useful range of input buffer lengths and ensuring correctness of all outputs:

  1. If we disallow varints with bitlength > 49, we cannot support uvarint64, but we can guarantee the correctness of all outputs (as we simply disallow decoding values > Number.MAX_SAFE_INTEGER).
  2. If we allow varints with bitlength > 49, we can support uvarint64, but we cannot guarantee the correctness of the output if the decoded value is > Number.MAX_SAFE_INTEGER.

#20, was written with the intent of 1.

Talking with other teams who use this library, it seems that 2. is the more practical, useful.
cc @vasco-santos

This PR keeps the core check of #20, but relaxes it to allow for varint64.

Copy link

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks for having a look @wemeetagain

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.

3 participants