remap: bound the plugin argument arrays - #13633
Draft
brbzull0 wants to merge 1 commit into
Draft
Conversation
BUILD_TABLE_INFO sizes argv and paramv at BUILD_TABLE_MAX_ARGS (2048), but remap_load_plugin() declared its new_argv and pargv locals at 1024, so a remap.config line carrying more than 1024 arguments overflowed them. The tokenizer in remap_parse_config_bti() also appended to bti->argv without a bound, checking the limit only after the loop. Size the locals from BUILD_TABLE_MAX_ARGS, bound the jump_to_argc copy by argc rather than trusting a nullptr terminator, and reject an over-long line while tokenizing.
Contributor
Author
|
[approve ci ubuntu] |
Contributor
Author
|
[approve ci autest 2] |
Contributor
Author
|
[approve ci freebsd clang-analyzer autest 2] |
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.
BUILD_TABLE_INFOsizes its argument arrays atBUILD_TABLE_MAX_ARGS, which is2048 (
include/proxy/http/remap/RemapConfig.h:31,62-63):remap_load_plugin()declared its own locals at half that:so a
remap.configline carrying more than 1024 arguments overflowed them. In abuild with
ink_assertenabled the existing assertion atRemapConfig.cc:912catches it and aborts; without it the writes simply run offthe end of two stack arrays.
Two further bounds issues in the same path:
jump_to_argccopy loop waswhile (argv[i + jump_to_argc]), i.e. itwalked until it found a null entry rather than stopping at
argc.remap_parse_config_bti()appended tobti->argv/bti->paramvinside thetokenizer loop but only compared against
BUILD_TABLE_MAX_ARGSafter the loophad finished, so
bti->argccould pass 2048 before anything checked.Change
new_argvandpargvfromBUILD_TABLE_MAX_ARGS + 1, so they can holdthe full limit plus the nullptr sentinel the copy loop reserves.
argcand bycountof(new_argv) - 1.the error, instead of after the fact.
Test
Adds
tests/gold_tests/remap/remap_plugin_argument_limits.test.py, with anear-limit rule that must load and serve, and an over-limit rule that must be
rejected. There is no existing test in
tests/gold_tests/remap/covering theargument count, and no collision with the 32 entries already there.
Confirmed it is a regression test: against unpatched
RemapConfig.ccit failswith
so
traffic_serveraborts while parsingremap.config. With the change thetest passes.