From ce4647a50761c0866edfa5bea4462f1b72d7ab86 Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Tue, 23 Jan 2024 16:40:03 +0100 Subject: [PATCH] BDAT: Harden parse_bdat_arg() There implementation assumed that arg is null terminated. Due to the ContentLineAnalyzer wrongly being in plain delivery mode, this assumption was violated. It shouldn't happen anymore, but protect from this anyhow. --- src/analyzer/protocol/smtp/BDAT.cc | 51 +++++++++++++++++-- .../weird.log | 2 +- .../weird.log | 2 +- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/analyzer/protocol/smtp/BDAT.cc b/src/analyzer/protocol/smtp/BDAT.cc index 999b4dfbcb..bd1f8fadf1 100644 --- a/src/analyzer/protocol/smtp/BDAT.cc +++ b/src/analyzer/protocol/smtp/BDAT.cc @@ -10,18 +10,36 @@ namespace zeek::analyzer::smtp::detail { struct BDATCmd parse_bdat_arg(int length, const char* arg) { - const char* arg_end = arg + length; struct BDATCmd r = {0}; + // UINT64_MAX followed by " LAST" is the most we can deal with + // and anyway this would be really weird for a client to use. + // strlen("18446744073709551615 LAST") + constexpr int max_arg_len = 25; + + if ( length <= 0 || length > max_arg_len ) { + r.error = "BDAT argument bad length"; + return r; + } + if ( *arg == '\0' || ! isdigit(*arg) ) { r.error = "BDAT not followed by a valid chunk-size"; return r; } + // Ensure arg is NULL terminated by copying the + // input into a new std::string object so we can use + // strtoull() properly. We do have zeek::util::atoi_n, + // but it's not handling overflows. + // + // The size is bounded, see max_arg_len above. + std::string arg_copy = {arg, static_cast(length)}; + const char* arg_end = arg_copy.c_str() + length; + errno = 0; char* chunk_size_end = nullptr; - uint64_t chunk_size = strtoull(arg, &chunk_size_end, 10); - if ( *chunk_size_end != ' ' && chunk_size_end != arg_end ) { + uint64_t chunk_size = strtoull(arg_copy.c_str(), &chunk_size_end, 10); + if ( *chunk_size_end != ' ' && *chunk_size_end != '\0' ) { r.error = "BDAT chunk-size not valid"; return r; } @@ -239,6 +257,33 @@ TEST_CASE("negative chunk size") { CHECK(error == std::string("BDAT not followed by a valid chunk-size")); } +TEST_CASE("non null terminated") { + // Regression test for buffer overread triggered by non-null + // terminated input from a wrongly configured ContentLineAnalyzer. + const char* input_data = "7777777777"; + auto data = std::make_unique(strlen(input_data)); + memcpy(data.get(), input_data, strlen(input_data)); + + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(strlen(input_data), data.get()); + REQUIRE(error == nullptr); + CHECK(chunk_size == 7777777777); +} + +TEST_CASE("maximum length") { + std::string line = "18446744073709551615 LAST"; + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(line.size(), line.c_str()); + REQUIRE(error == nullptr); + CHECK(chunk_size == 18446744073709551615UL); + CHECK(is_last_chunk == true); +} + +TEST_CASE("maximum length exceeded") { + std::string line = "184467440737095516150 LAST"; + const auto& [chunk_size, is_last_chunk, error] = parse_bdat_arg(line.size(), line.c_str()); + REQUIRE(error != nullptr); + CHECK(error == std::string("BDAT argument bad length")); +} + TEST_SUITE_END(); TEST_SUITE_BEGIN("bdat line analyzer"); diff --git a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log index 2129c89e16..efc3169f89 100644 --- a/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log +++ b/testing/btest/Baseline/scripts.base.protocols.bdat-chunk-size-overflow2/weird.log @@ -7,5 +7,5 @@ #open XXXX-XX-XX-XX-XX-XX #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer source #types time string addr port addr port string string bool string string -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 33762 127.0.0.1 25 smtp_invalid_bdat_command BDAT chunk-size too large F zeek SMTP +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 33762 127.0.0.1 25 smtp_invalid_bdat_command BDAT argument bad length F zeek SMTP #close XXXX-XX-XX-XX-XX-XX diff --git a/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.log b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.log index 541c903010..783e430c0f 100644 --- a/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.log +++ b/testing/btest/Baseline/scripts.base.protocols.smtp.bdat-cmd-invalid/weird.log @@ -7,7 +7,7 @@ #open XXXX-XX-XX-XX-XX-XX #fields ts uid id.orig_h id.orig_p id.resp_h id.resp_p name addl notice peer source #types time string addr port addr port string string bool string string -XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 33472 127.0.0.1 25 smtp_invalid_bdat_command BDAT not followed by a valid chunk-size F zeek SMTP +XXXXXXXXXX.XXXXXX CHhAvVGS1DHFjwGM9 127.0.0.1 33472 127.0.0.1 25 smtp_invalid_bdat_command BDAT argument bad length F zeek SMTP XXXXXXXXXX.XXXXXX ClEkJM2Vm5giqnMf4h 127.0.0.1 52364 127.0.0.1 25 smtp_invalid_bdat_command BDAT chunk-size followed by junk F zeek SMTP XXXXXXXXXX.XXXXXX C4J4Th3PJpwUYZZ6gc 127.0.0.1 46862 127.0.0.1 25 smtp_invalid_bdat_command BDAT not followed by a valid chunk-size F zeek SMTP #close XXXX-XX-XX-XX-XX-XX