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

Add parse_uint function to improve integer overflow handling #706

Merged
merged 1 commit into from Jun 2, 2017

Conversation

Projects
None yet
2 participants
@dequis
Member

dequis commented May 17, 2017

Originally found by oss-fuzz (issue 525) in get_ansi_color using ubsan. After a lot of analysis I'm 99% sure this isn't security relevant so it's fine to handle this publicly.

The fix is mainly adding a function that does it right and use it everywhere. This is harder than it seems because the strtol() family of functions doesn't have the friendliest of interfaces.

Aside from get_ansi_color(), there were other pieces of code that used the same (out*10+(*in-'0')) pattern, like the parse_size() and parse_time_interval() functions, which are mostly used for settings. Those are interesting cases, since they multiply the parsed number (resulting in more overflows) and they write to a signed integer parameter (which can accidentally make the uints negative without UB)

Thanks to Pascal Cuoq for enlightening me about the undefined behavior of parse_size (and, in particular, the implementation-defined behavior of one of the WIP versions of this commit, where something like signed integer overflow happened, but it was legal). Also for writing tis-interpreter, which is better than ubsan to verify these things.

Add parse_uint function to improve integer overflow handling
Originally found by oss-fuzz (issue 525) in get_ansi_color using ubsan.
After a lot of analysis I'm 99% sure this isn't security relevant so
it's fine to handle this publicly.

The fix is mainly adding a function that does it right and use it
everywhere. This is harder than it seems because the strtol() family of
functions doesn't have the friendliest of interfaces.

Aside from get_ansi_color(), there were other pieces of code that used
the same (out*10+(*in-'0')) pattern, like the parse_size() and
parse_time_interval() functions, which are mostly used for settings.
Those are interesting cases, since they multiply the parsed number
(resulting in more overflows) and they write to a signed integer
parameter (which can accidentally make the uints negative without UB)

Thanks to Pascal Cuoq for enlightening me about the undefined behavior
of parse_size (and, in particular, the implementation-defined behavior
of one of the WIP versions of this commit, where something like signed
integer overflow happened, but it was legal). Also for writing
tis-interpreter, which is better than ubsan to verify these things.
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented May 22, 2017

well good that you did this and I think it's great -- except I don't feel like I can review it with a certainty. just fyi. maybe just merge and yolo?

@ailin-nemui ailin-nemui merged commit 31b9d11 into irssi:master Jun 2, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ailin-nemui added a commit to ailin-nemui/irssi that referenced this pull request Dec 7, 2017

Merge pull request irssi#706 from dequis/parse-uint
Add parse_uint function to improve integer overflow handling
(cherry picked from commit 31b9d11)

@ailin-nemui ailin-nemui added this to the 1.0.3 milestone Jan 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment