diff --git a/args.hxx b/args.hxx index 86a8672..39c6f17 100644 --- a/args.hxx +++ b/args.hxx @@ -3761,6 +3761,14 @@ namespace args } const char *begin = value.c_str(); + // The true end of the value, derived from its length rather than + // from the first NUL. strtoull/strtoll treat the buffer as a C + // string and stop at an embedded '\0', so checking `*end == '\0'` + // for "no trailing data" is defeated by a value like "12\0junk": + // end lands on the embedded NUL and the junk after it is silently + // accepted. Comparing against `stop` validates the whole string + // and matches the istringstream-based reader used for other types. + const char *const stop = begin + value.size(); // C++11-compatible: use strtoull/strtoll. Hardening retained from // the original from_chars draft (errno save/restore, ERANGE check, @@ -3779,11 +3787,11 @@ namespace args errno = saved_errno; return false; } - while (*end != '\0' && std::isspace(static_cast(*end))) + while (end != stop && std::isspace(static_cast(*end))) { ++end; } - if (*end != '\0' || errno == ERANGE || + if (end != stop || errno == ERANGE || parsed > static_cast(std::numeric_limits::max())) { errno = saved_errno; @@ -3800,11 +3808,11 @@ namespace args errno = saved_errno; return false; } - while (*end != '\0' && std::isspace(static_cast(*end))) + while (end != stop && std::isspace(static_cast(*end))) { ++end; } - if (*end != '\0' || errno == ERANGE || + if (end != stop || errno == ERANGE || parsed < static_cast(std::numeric_limits::min()) || parsed > static_cast(std::numeric_limits::max())) { diff --git a/test/embedded_nul_numeric.cxx b/test/embedded_nul_numeric.cxx new file mode 100644 index 0000000..c4bfd5f --- /dev/null +++ b/test/embedded_nul_numeric.cxx @@ -0,0 +1,82 @@ +// Regression test: the integer reader must not silently accept data that +// follows an embedded NUL byte in the value string. +// +// ValueReader::ParseNumericValue for integer types parses value.c_str() with +// std::strtoull/strtoll, which treat the buffer as a C string and stop at the +// first '\0'. The "no trailing data" guard used to test `*end == '\0'`, which +// an embedded NUL satisfies prematurely: "12\0garbage" was accepted as 12 and +// everything past the NUL was silently discarded (an input-validation bypass, +// CWE-158). The istringstream-based reader used for floating point already +// rejected such input, so the two paths disagreed. The guard now validates +// against the string's true length, so both paths reject trailing data +// regardless of embedded NULs. +// +// This test FAILS on the unfixed library (the integer cases parse as success) +// and passes once the length-based check is in place. + +#include "test_common.hxx" +#include +#include "test_helpers.hxx" +#include +#include + +template +static void expect_reject(const std::string &input) +{ + args::ValueReader reader; + T val = 0; + const bool success = reader.ParseNumericValue(input, val); + if (success) + { + std::cerr << "FAIL: value with embedded NUL (size " << input.size() + << ") was accepted as " << static_cast(val) + << " but should have been rejected" << std::endl; + exit(1); + } +} + +template +static void expect_accept(const std::string &input, T expected) +{ + args::ValueReader reader; + T val = 0; + const bool success = reader.ParseNumericValue(input, val); + if (!success) + { + std::cerr << "FAIL: valid value '" << input << "' was rejected" << std::endl; + exit(1); + } + if (val != expected) + { + std::cerr << "FAIL: value '" << input << "' parsed as " + << static_cast(val) << " (expected " + << static_cast(expected) << ")" << std::endl; + exit(1); + } +} + +int main() +{ + const std::string nul(1, '\0'); + + // Data after an embedded NUL must not be silently dropped. + expect_reject("12" + nul + "garbage"); + expect_reject("34" + nul + "99"); + expect_reject("12" + nul + "garbage"); + expect_reject("7" + nul + nul); + // Trailing whitespace followed by an embedded NUL and junk: the + // whitespace-tolerance loop must still stop at the true end. + expect_reject("12 " + nul + "x"); + // A NUL on its own is not a number. + expect_reject(nul); + + // Genuinely valid values, including the whitespace tolerance and the + // base-0 hex/octal forms, must keep parsing. + expect_accept("12", 12); + expect_accept(" -7 ", -7); + expect_accept("0x10", 16); + expect_accept("010", 8); + + std::cout << "embedded NUL numeric tests passed!" << std::endl; + return 0; +}