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

SmileParser throws unexpected IOOBE for corrupt content #426

Closed
arthurscchan opened this issue Dec 18, 2023 · 1 comment
Closed

SmileParser throws unexpected IOOBE for corrupt content #426

arthurscchan opened this issue Dec 18, 2023 · 1 comment
Labels
2.17 fuzz Issue found by OssFuzz has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue smile
Milestone

Comments

@arthurscchan
Copy link
Contributor

In the SmileParser::nextTextValue() method, there is a line that uses the Integer ptr as an index to retrieve a byte from the _inputBuffer. But it is found that with some invalid input and repeat calling to the SmileParser::nextTextValue() method, it could cause ptr to be negative and trigger an unexpected ArrayIndexOutOfBoundsException.

     public String nextTextValue() throws IOException
    {
       ...
            int ptr = _inputPtr;
            if (ptr >= _inputEnd) {
               ...
            }
            _tokenOffsetForTotal = ptr;
            int ch = _inputBuffer[ptr++] & 0xFF;
       ...

The simplest fix is to add a bound check for the ptr before using it as the array index.

We found this issue by OSS-Fuzz and it is reported in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65126.

@cowtowncoder cowtowncoder added smile fuzz Issue found by OssFuzz 2.17 need-test-case To work on issue, a reproduction (ideally unit test) needed labels Dec 18, 2023
@cowtowncoder
Copy link
Member

The problem here is that this might not be directly due to anything within nextTextValue() itself but (possibly) in preceding steps. So it is necessary to see the sequence of things that lead to the problematic state, in which call to nextTextValue() (and likely other calls) would fail.

Unfortunately I don't think nextTextValue() can truly validate offset at that point, but rather whatever lead to invalid value needs to be fixed
(specifically: just because offset is within valid buffer does not mean it might not be corrupt -- it being off the buffer does indicate it is invalid, of course, but the goal is prevent the problem where it occurs).

It is very likely that this requires an invalid document being read; but it may also rely on specific accessors/iteration methods being called.

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue and removed need-test-case To work on issue, a reproduction (ideally unit test) needed labels Dec 19, 2023
@cowtowncoder cowtowncoder changed the title SmileParser throws unexpected IOOBE SmileParser throws unexpected IOOBE for corrupt content Dec 19, 2023
@cowtowncoder cowtowncoder added this to the 2.17.0 milestone Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 fuzz Issue found by OssFuzz has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue smile
Projects
None yet
Development

No branches or pull requests

2 participants