Fix bounds checking pattern in parser #539

chfast · 2020-09-21T20:05:07Z

The previous pattern pos + size < end was incorrect leading to
possible undefined behavior and potential addition overflow allowing
the check bypass. The correct check is to compute the remaining size
and compare with the declared size. This way invalid pointer comparison
is replaced with correct pointer subtraction (pos and end are from the
same memory buffer) and integer comparison: (end - pos) < size.

This will be additionally check with extended ASan options in #469.

codecov · 2020-09-21T20:59:22Z

Codecov Report

Merging #539 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #539   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          58       58           
  Lines        8681     8703   +22     
=======================================
+ Hits         8571     8593   +22     
  Misses        110      110

chfast · 2020-09-22T11:10:39Z

Good refactoring/optimization opportunity after is to pass remaining_size instead of end pointer to parsing function.

gumb0 · 2020-09-22T11:20:47Z

lib/fizzy/parser.hpp

@@ -29,8 +29,8 @@ inline parser_result<uint8_t> parse_byte(const uint8_t* pos, const uint8_t* end)
 template <typename T>
 inline parser_result<T> parse_value(const uint8_t* pos, const uint8_t* end)
 {
-    constexpr auto size = sizeof(T);
-    if (pos + size > end)
+    static constexpr int size = sizeof(T);


why static?

And why int, isn't it then signed-unsigned comparison below?

Pointer subtraction returns signed integer. I will use ptrdiff_t for clarity if easily available.
For static I don't remember, I will remove it.

gumb0 · 2020-09-22T11:22:43Z

lib/fizzy/utf8.cpp

@@ -105,11 +105,12 @@ bool utf8_validate(const uint8_t* pos, const uint8_t* end) noexcept
        else
            return false;

+        assert(required_bytes > 1);


Could put this line after the comment below

Oh yeah, good point.

gumb0 · 2020-09-22T11:40:46Z

possible undefined behavior

when would it be UB?

chfast · 2020-09-22T12:24:25Z

when would it be UB?

The adversarial element is size (it comes from the outside).

The first issue is in pos + size expression. Information about "pointer and integer addition":

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 overﬂow; otherwise, the behavior is undeﬁned.

And later this provokes another issue in pointer comparison (pos + size) < end:

When two pointers are subtracted, both shall point to elements of the same array object, or one past the last element of the array object.

In practice, this is a security bug because in case there is a way to have a wasm module with size big enough to cause pos + size overflow the bounds check will be bypassed. Maybe not practical on 64-bit systems as I believe size is limited to 32-bits. Still worth exploring in context of other wasm binary readers.

Use smaller (often exact size) buffers for test cases of input bounds checking in parser. This way we can detect incorrect pointer comparisons with ASan's pointer-compare and pointer-subtract instrumentation.

The previous pattern `pos + size < end` was incorrect leading to possible undefined behavior and potential addition overflow allowing the check bypass. The correct check is to compute the remaining size and compare with the declared size. This way invalid pointer comparison is replaced with correct pointer subtraction (pos and end are from the same memory buffer) and integer comparison: `(end - pos) < size`.

See previous commit for explanation.

chfast requested review from axic and gumb0 September 21, 2020 20:05

chfast force-pushed the fix_parser_eof branch from 4b8e0a9 to 126d578 Compare September 21, 2020 20:46

chfast added the bug Something isn't working label Sep 22, 2020

gumb0 reviewed Sep 22, 2020

View reviewed changes

chfast force-pushed the fix_parser_eof branch from 126d578 to a07aa2c Compare September 22, 2020 12:44

gumb0 approved these changes Sep 22, 2020

View reviewed changes

axic approved these changes Sep 22, 2020

View reviewed changes

chfast added 3 commits September 22, 2020 21:20

test: More precise bounds checking test cases

e49b1cb

Use smaller (often exact size) buffers for test cases of input bounds checking in parser. This way we can detect incorrect pointer comparisons with ASan's pointer-compare and pointer-subtract instrumentation.

Fix bounds checking in utf8_validate()

56978eb

See previous commit for explanation.

chfast force-pushed the fix_parser_eof branch from a07aa2c to 56978eb Compare September 22, 2020 19:21

chfast merged commit dc8ecf3 into master Sep 22, 2020

chfast deleted the fix_parser_eof branch September 22, 2020 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bounds checking pattern in parser #539

Fix bounds checking pattern in parser #539

chfast commented Sep 21, 2020

codecov bot commented Sep 21, 2020 •

edited

Loading

chfast commented Sep 22, 2020

gumb0 Sep 22, 2020

gumb0 Sep 22, 2020

chfast Sep 22, 2020

gumb0 Sep 22, 2020

chfast Sep 22, 2020

gumb0 commented Sep 22, 2020

chfast commented Sep 22, 2020

Fix bounds checking pattern in parser #539

Fix bounds checking pattern in parser #539

Conversation

chfast commented Sep 21, 2020

codecov bot commented Sep 21, 2020 • edited Loading

Codecov Report

chfast commented Sep 22, 2020

gumb0 Sep 22, 2020

Choose a reason for hiding this comment

gumb0 Sep 22, 2020

Choose a reason for hiding this comment

chfast Sep 22, 2020

Choose a reason for hiding this comment

gumb0 Sep 22, 2020

Choose a reason for hiding this comment

chfast Sep 22, 2020

Choose a reason for hiding this comment

gumb0 commented Sep 22, 2020

chfast commented Sep 22, 2020

codecov bot commented Sep 21, 2020 •

edited

Loading