Repository navigation
Harden the decoder against malformed EXI input - #103
Open
prandogabriel wants to merge 1 commit into
Open
prandogabriel wants to merge 1 commit into
prandogabriel wants to merge 1 commit into
Conversation
Malformed EXI streams could crash the process instead of returning an error: out-of-bounds reads in the value tables, asserts reachable from the input, realloc() of static string tables, a use-after-free across streams that share a schema, and a value decoded outside any element. Add malformed input tests (bit flips, truncations and random corruption of existing test-set files, in schema-informed and schemaless modes, plus an EXI options header that grows the static options grammar).
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Malformed EXI input can crash the process (segfault or abort) instead of making the parser return an error. A service that decodes EXI from the network can be taken down with a single request. I found this while using EXIP to decode IEEE 2030.5 (SEP2) messages, and reproduced every case with the fixtures already in
tests/test-set.There are five root causes:
decodeStringValue): the local (vxTable) and global value partition indexes read from the stream were not checked against the table sizes.xsi:nil="true"(SE(*), SE(qname) and CH) and a QName value outsideAT(xsi:type)hitassert()and abort.realloc()of static memory: when a stream adds string table entries beyond the capacity of a static grammar (for example the EXI options grammarops_schemawhile it decodes header options),addEmptyDynEntrycallsEXIP_REALLOCon a static array.SE(*)creates a built-in element grammar and stores its index inLnEntry.elemGrammarof a schema local name.freeAllMemfrees the built-in grammars at the end of the stream, but the index stays in the schema. The next stream that uses the same schema follows the freed grammar.INDEX_MAX) and indexes the string table with it.Changes
decodeStringValue: reject a QName that is not in the string table, empty value tables and out-of-range local and global indexes withEXIP_INVALID_EXI_INPUT.decodeEventContent/decodeValueItem: the input-reachableasserts now returnEXIP_INVALID_EXI_INPUT.DynArray: newheapAllocatedmember.createDynArrayand the first allocation inaddEmptyDynEntryset it. Static grammars (exipg output,staticEXIOptions.c,staticXmlSchema.c) leave it zero because they use positional initializers with inner braces. When a static array has to grow, it is copied to the heap instead of passed torealloc.destroyDynArrayonly frees heap arrays. A failedreallocno longer overwrites the base pointer.freeAllMem: before freeing the built-in grammars, reset toINDEX_MAXeveryelemGrammarthat points to one of them.ABI note
Adding a member to
DynArray(include/procTypes.h) changes the size of the public structs that embed it. Sources that are compiled together, including generated static grammars, are not affected. Prebuilt objects compiled against the old header would need a rebuild. If you prefer to avoid that, I can move the flag elsewhere. Let me know which you'd rather have.Tests
New "Malformed input" test case in
tests/check_exip.c:test_malformed_schemaless:xsWhitespace/whitespace-instance.xml.exiwith every single-bit flip, every truncation and 50,000 seeded random corruptions.test_malformed_schema_informed: the same fortestStates/acceptance_a_01.exiandsimpleContentExt/test.xml.exiwith their schemas. All streams reuse one schema object, and the valid file must still decode afterwards.test_malformed_header_options: an EXI options header that grows the string table of the static options grammar.On
mainthe test binary aborts (Abort trap: 6). With this changemake TARGET=pc clean all check examples utilspasses on macOS (clang), with theexipsuite going from 10 to 13 checks, and the patch adds no new compiler warnings. I also checked that each test catches its fix: without thefreeAllMemchange the schema-informed test segfaults, and without theDynArraychange the header options test aborts.I did not test the Windows (MSVC) build. No new files were added, so the Visual Studio projects should not need changes.