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

Discard optimized containers with negative counts in UBJSON/BJData (#3491,#3492,#3490) #3500

Merged
merged 5 commits into from
May 18, 2022

Conversation

fangq
Copy link
Contributor

@fangq fangq commented May 17, 2022

This may partially address the fuzzer errors reported in #3491, #3492 and #3490.

Specifically, the above failed fuzzer errors appear to be triggered when an ND-array optimized header contains 0 in any of the dimensional vector elements. This causes partially written objects and array constructs.

Another issue this patch fixes is related to the implementation-spec mismatch discussed in #3492 (comment)

a negative count should result in an invalid UBJSON and BJData input.

@fangq fangq requested a review from nlohmann as a code owner May 17, 2022 21:45
@coveralls
Copy link

coveralls commented May 17, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling cbf72c6 on NeuroJSON:issue3492 into 6a73920 on nlohmann:develop.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

I checked the fix with the inputs of issues #3491, #3492, and #3490, and none encountered the assertions I added in #3498. I only have some smaller remarks.

tests/src/unit-bjdata.cpp Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
include/nlohmann/detail/input/binary_reader.hpp Outdated Show resolved Hide resolved
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this May 18, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone May 18, 2022
@nlohmann nlohmann merged commit 93c9e0c into nlohmann:develop May 18, 2022
@nlohmann
Copy link
Owner

I let the fuzzer run on the now-merged branch and see no issues after letting it run for half an hour. 👍

@nlohmann
Copy link
Owner

@fangq

I think I found another issue:

In the binary reader in function get_ubsize_size_value, we have

string_t key = "_ArraySize_";
if (JSON_HEDLEY_UNLIKELY(!sax->start_object(3) || !sax->key(key) || !sax->start_array(dim.size())))
{
    return false;
}

In an example input, I get the following SAX events:

<object size="3">
    <key key="_ArraySize_" />
    <array size="2">
        <number_integer val="2148925440" />
        <number_integer val="6701356247676222464" />
    </array>

However, the object that is started with sax->start_object(3) is never ended with a closing end_object() call. This yields issues down the road in the SAX-DOM parser, because, we when we call the return sax->end_array(); line at the end of the code block, we are still inside the unclosed object we started before.

Here is the respective input: crash.bjdata.zip

I will re-run the fuzzer with the enabled assertions from #3498 and see if I can find smaller examples.

@nlohmann
Copy link
Owner

Here is a 14 byte example:

00000000: 5b5b 235b 5500 5b55 ff69 5d5d 5d5d       [[#[U.[U.i]]]]

which produces these SAX events:

<array>
    <object size="3">
        <key key="_ArraySize_" />
        <array size="2">
            <number_integer val="255" />
            <number_integer val="93" />
        </array>
        <array size="0">
        </array>
    </array>

The more I look at it: why is there a hard-coded 3 for the object size?

minimized.bjdata.zip

@fangq
Copy link
Contributor Author

fangq commented May 18, 2022

@nlohmann, let me know if the patch if #3502 fixes the errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants