From: Oleksii Shumeiko -X (oshumeik - SOFTSERVE INC at Cisco) Date: Wed, 14 Dec 2022 14:38:06 +0000 (+0000) Subject: Pull request #3696: ips_options: fix offset related bug in byte_test eval() X-Git-Tag: 3.1.49.0~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ab564d8aa5384864162ad2f7c63b2e45170f019b;p=thirdparty%2Fsnort3.git Pull request #3696: ips_options: fix offset related bug in byte_test eval() Merge in SNORT/snort3 from ~ANOROKH/snort3:fix_byte_test to master Squashed commit of the following: commit 1ef48b5e9e068f1b67c187635f31fd4f63676379 Author: AnnaNorokh Date: Wed Dec 7 17:04:36 2022 +0200 ips_options: fix offset related bug in byte_test eval() * moved truncation of string from ips_byte_test eval() to extract data_extraction(), so all byte_ options have the same logic; * added unit tests to verify proper work with negative offset on the last byte of buffer; * added unit tests for all byte_ options to check situation when bytes_to_extract bigger then amount of bytes left in the buffer; * updated documentation and help option with info about string truncation; --- diff --git a/doc/user/byte_options.txt b/doc/user/byte_options.txt index 482c2b0e7..d07345773 100644 --- a/doc/user/byte_options.txt +++ b/doc/user/byte_options.txt @@ -148,3 +148,8 @@ Our full rule would be: content:"|00 01 87 88|", offset 12, depth 4; content:"|00 00 00 01 00 00 00 01|", offset 20, depth 8; byte_test:4,>,200,36; + +In case of using any byte_* option with "string" parameter, the amount of bytes +to be extracted from payload can be less than specified by user. +This might happen when the buffer has fewer bytes (from the cursor position) +than specified in the option. \ No newline at end of file diff --git a/src/ips_options/extract.cc b/src/ips_options/extract.cc index 2fba7a547..33a0eec10 100644 --- a/src/ips_options/extract.cc +++ b/src/ips_options/extract.cc @@ -307,8 +307,12 @@ int32_t data_extraction(const ByteData& settings, Packet* p, } else { - bytes_read = string_extract(settings.bytes_to_extract, settings.base, - ptr, start, end, &value); + unsigned len = end - ptr; + + if (len > settings.bytes_to_extract) + len = settings.bytes_to_extract; + + bytes_read = string_extract(len, settings.base, ptr, start, end, &value); if (bytes_read < 0) return IpsOption::NO_MATCH; } diff --git a/src/ips_options/ips_byte_extract.cc b/src/ips_options/ips_byte_extract.cc index 518fcee1b..6909d7284 100644 --- a/src/ips_options/ips_byte_extract.cc +++ b/src/ips_options/ips_byte_extract.cc @@ -245,7 +245,7 @@ static bool ByteExtractVerify(ByteExtractData* data) static const Parameter s_params[] = { { "~count", Parameter::PT_INT, "1:10", nullptr, - "number of bytes to pick up from the buffer" }, + "number of bytes to pick up from the buffer (string can pick less)" }, { "~offset", Parameter::PT_INT, "-65535:65535", nullptr, "number of bytes into the buffer to start processing" }, @@ -704,6 +704,70 @@ TEST_CASE("ByteExtractOption::eval valid", "[ips_byte_extract]") CHECK(res == 508); CHECK(c.get_pos() == 4); } + SECTION("bytes_to_extract bigger than amount of bytes left in the buffer") + { + ByteExtractData data; + c.set_pos(9); + INITIALIZE(data, 3, 0, 1, 0, 0, ENDIAN_BIG, 0, 1, 0, 0, name); + ByteExtractOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::NO_MATCH); + } + SECTION("String truncation") + { + ByteExtractData data; + c.set_pos(10); + INITIALIZE(data, 2, 0, 1, 1, 0, ENDIAN_BIG, 10, 1, 0, 0, name); + ByteExtractOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 5); + } + + SECTION("Negative offset") + { + SECTION("Cursor on last byte of buffers") + { + ByteExtractData data; + c.set_pos(11); + INITIALIZE(data, 1, -6, 1, 0, 0, ENDIAN_BIG, 10, 1, 0, 0, name); + ByteExtractOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 32); + } + SECTION("Cursor on last byte of buffers, bytes_to_extract is bigger than offset") + { + ByteExtractData data; + c.set_pos(11); + INITIALIZE(data, 4, -3, 1, 0, 0, ENDIAN_BIG, 0, 1, 0, 0, name); + ByteExtractOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::NO_MATCH); + } + SECTION("Cursor on the last byte of buffer with string flag") + { + ByteExtractData data; + c.set_pos(11); + INITIALIZE(data, 1, -2, 1, 1, 0, ENDIAN_BIG, 10, 1, 0, 0, name); + ByteExtractOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 4); + } + SECTION("String truncation") + { + ByteExtractData data; + c.set_pos(11); + INITIALIZE(data, 3, -2, 1, 1, 0, ENDIAN_BIG, 10, 1, 0, 0, name); + ByteExtractOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 45); + } + } } TEST_CASE("ByteExtractOption::eval invalid", "[ips_byte_extract]") diff --git a/src/ips_options/ips_byte_jump.cc b/src/ips_options/ips_byte_jump.cc index 11a73e5aa..cb5b2fe2a 100644 --- a/src/ips_options/ips_byte_jump.cc +++ b/src/ips_options/ips_byte_jump.cc @@ -278,7 +278,7 @@ IpsOption::EvalStatus ByteJumpOption::eval(Cursor& c, Packet* p) static const Parameter s_params[] = { { "~count", Parameter::PT_INT, "0:10", nullptr, - "number of bytes to pick up from the buffer" }, + "number of bytes to pick up from the buffer (string can pick less)" }, { "~offset", Parameter::PT_STRING, nullptr, nullptr, "variable name or number of bytes into the buffer to start processing" }, @@ -811,6 +811,130 @@ TEST_CASE("ByteJumpOption test", "[ips_byte_jump]") ByteJumpOption test_5_1(byte_jump); REQUIRE((test_5_1.eval(current_cursor, &test_packet)) == MATCH); } + + SECTION("byte_to_extract bigger than bytes left in buffer") + { + uint8_t buff[] = "Hello world long input"; + current_cursor.set("hello_world_long_name", buff, 22); + current_cursor.set_pos(22); + byte_jump.bytes_to_extract = 1; + byte_jump.from_beginning_flag = 0; + byte_jump.from_end_flag = 0; + byte_jump.offset = 0; + byte_jump.offset_var = -1; + byte_jump.post_offset = 0; + byte_jump.post_offset_var = -1; + byte_jump.relative_flag = 1; + byte_jump.string_convert_flag = 0; + ByteJumpOption test_6(byte_jump); + REQUIRE((test_6.eval(current_cursor, &test_packet)) == NO_MATCH); + } + + SECTION("String truncation") + { + uint8_t buff[] = "Hello world long input 000"; + current_cursor.set("hello_world_long_name", buff, 26); + current_cursor.set_pos(24); + byte_jump.bytes_to_extract = 3; + byte_jump.from_beginning_flag = 0; + byte_jump.from_end_flag = 0; + byte_jump.offset = 0; + byte_jump.offset_var = -1; + byte_jump.post_offset = 0; + byte_jump.post_offset_var = -1; + byte_jump.relative_flag = 1; + byte_jump.string_convert_flag = 1; + byte_jump.base = 10; + byte_jump.bitmask_val = 0; + byte_jump.align_flag = 0; + ByteJumpOption test_7(byte_jump); + REQUIRE((test_7.eval(current_cursor, &test_packet)) == MATCH); + } + + SECTION("Negative offset") + { + SECTION("Cursor on the last byte of buffer") + { + uint8_t buff[] = "Hello world long input"; + current_cursor.set("hello_world_long_name", buff, 22, true); + current_cursor.set_pos(22); + byte_jump.bytes_to_extract = 1; + byte_jump.from_beginning_flag = 0; + byte_jump.from_end_flag = 0; + byte_jump.offset = -6; + byte_jump.offset_var = -1; + byte_jump.post_offset = 0; + byte_jump.post_offset_var = -1; + byte_jump.relative_flag = 1; + byte_jump.string_convert_flag = 0; + byte_jump.align_flag = 0; + byte_jump.bitmask_val = 0; + ByteJumpOption test_8(byte_jump); + REQUIRE((test_8.eval(current_cursor, &test_packet)) == NO_MATCH); + REQUIRE(current_cursor.awaiting_data()); + } + + SECTION("Cursor on the last byte of buffer, bytes_to_extract is bigger than offset") + { + uint8_t buff[] = "Hello world long input"; + current_cursor.set("hello_world_long_name", buff, 22); + current_cursor.set_pos(22); + byte_jump.bytes_to_extract = 3; + byte_jump.from_beginning_flag = 0; + byte_jump.from_end_flag = 0; + byte_jump.offset = -2; + byte_jump.offset_var = -1; + byte_jump.post_offset = 0; + byte_jump.post_offset_var = -1; + byte_jump.relative_flag = 1; + byte_jump.string_convert_flag = 0; + byte_jump.align_flag = 0; + ByteJumpOption test_9(byte_jump); + REQUIRE((test_9.eval(current_cursor, &test_packet)) == NO_MATCH); + } + + SECTION("Cursor on the last byte of buffer with string flag") + { + uint8_t buff[] = "Hello world long input 000"; + current_cursor.set("hello_world_long_name", buff, 26); + current_cursor.set_pos(26); + byte_jump.bytes_to_extract = 1; + byte_jump.from_beginning_flag = 0; + byte_jump.from_end_flag = 0; + byte_jump.offset = -2; + byte_jump.offset_var = -1; + byte_jump.post_offset = 0; + byte_jump.post_offset_var = -1; + byte_jump.relative_flag = 1; + byte_jump.string_convert_flag = 1; + byte_jump.base = 10; + byte_jump.bitmask_val = 0; + byte_jump.align_flag = 0; + ByteJumpOption test_10(byte_jump); + REQUIRE((test_10.eval(current_cursor, &test_packet)) == MATCH); + } + + SECTION("String truncation") + { + uint8_t buff[] = "Hello world long input 000"; + current_cursor.set("hello_world_long_name", buff, 26); + current_cursor.set_pos(26); + byte_jump.bytes_to_extract = 3; + byte_jump.from_beginning_flag = 0; + byte_jump.from_end_flag = 0; + byte_jump.offset = -2; + byte_jump.offset_var = -1; + byte_jump.post_offset = 0; + byte_jump.post_offset_var = -1; + byte_jump.relative_flag = 1; + byte_jump.string_convert_flag = 1; + byte_jump.base = 10; + byte_jump.bitmask_val = 0; + byte_jump.align_flag = 0; + ByteJumpOption test_11(byte_jump); + REQUIRE((test_11.eval(current_cursor, &test_packet)) == MATCH); + } + } } } diff --git a/src/ips_options/ips_byte_math.cc b/src/ips_options/ips_byte_math.cc index 707ddbca2..8b4b0b347 100644 --- a/src/ips_options/ips_byte_math.cc +++ b/src/ips_options/ips_byte_math.cc @@ -282,7 +282,7 @@ static void parse_endian(uint8_t value, ByteMathData& idx) static const Parameter s_params[] = { { "bytes", Parameter::PT_INT, "1:10", nullptr, - "number of bytes to pick up from the buffer" }, + "number of bytes to pick up from the buffer (string can pick less)" }, { "offset", Parameter::PT_STRING, nullptr, nullptr, "number of bytes into the buffer to start processing" }, @@ -915,6 +915,86 @@ TEST_CASE("ByteMathOption::eval valid", "[ips_byte_math]") GetVarValueByIndex(&res, 1); CHECK(res == 222); } + + SECTION("bytes_to_extract bigger than amount of bytes left in the buffer") + { + SetVarValueByIndex(1, 0); + c.set_pos(10); + ByteMathData data; + INITIALIZE(data, 2, 2, 0, 0, name, BM_MULTIPLY, + 1, 0, 0, ENDIAN_BIG, 0, IPS_OPTIONS_NO_VAR, -1); + ByteMathOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::NO_MATCH); + } + + SECTION("String truncation") + { + SetVarValueByIndex(1, 0); + c.set_pos(10); + ByteMathData data; + INITIALIZE(data, 2, 2, 0, 0, name, BM_MULTIPLY, + 1, 1, 0, ENDIAN_BIG, 0, IPS_OPTIONS_NO_VAR, -1); + ByteMathOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 10); + } + + SECTION("Negative offset") + { + SECTION("Cursor on the last byte of buffer") + { + SetVarValueByIndex(1, 0); + c.set_pos(11); + ByteMathData data; + INITIALIZE(data, 1, 2, -6, 0, name, BM_MULTIPLY, + 1, 0, 0, ENDIAN_BIG, 0, IPS_OPTIONS_NO_VAR, -1); + ByteMathOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 64); + } + SECTION("Cursor on the last byte of buffer, bytes_to_extract is bigger than offset") + { + SetVarValueByIndex(1, 0); + c.set_pos(11); + ByteMathData data; + INITIALIZE(data, 3, 2, -2, 0, name, BM_MULTIPLY, + 1, 0, 0, ENDIAN_BIG, 0, IPS_OPTIONS_NO_VAR, -1); + ByteMathOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::NO_MATCH); + } + + SECTION("Cursor on the last byte of buffer with string flag") + { + SetVarValueByIndex(1, 0); + c.set_pos(11); + ByteMathData data; + INITIALIZE(data, 2, 2, -2, 0, name, BM_MULTIPLY, + 1, 1, 0, ENDIAN_BIG, 0, IPS_OPTIONS_NO_VAR, -1); + ByteMathOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 90); + } + + SECTION("String truncation") + { + SetVarValueByIndex(1, 0); + c.set_pos(11); + ByteMathData data; + INITIALIZE(data, 2, 2, -1, 0, name, BM_MULTIPLY, + 1, 1, 0, ENDIAN_BIG, 0, IPS_OPTIONS_NO_VAR, -1); + ByteMathOption opt(data); + CHECK(opt.eval(c, &p) == IpsOption::MATCH); + uint32_t res = 0; + GetVarValueByIndex(&res, 0); + CHECK(res == 10); + } + } } TEST_CASE("ByteMathOption::eval invalid", "[ips_byte_math]") diff --git a/src/ips_options/ips_byte_test.cc b/src/ips_options/ips_byte_test.cc index 98c9d58fe..dc6bbf7dd 100644 --- a/src/ips_options/ips_byte_test.cc +++ b/src/ips_options/ips_byte_test.cc @@ -295,12 +295,7 @@ IpsOption::EvalStatus ByteTestOption::eval(Cursor& c, Packet* p) else offset = btd->offset; - unsigned len = btd->relative_flag ? c.length() : c.size(); - if (len > btd->bytes_to_extract) - len = btd->bytes_to_extract; - ByteTestData extract_config = *btd; - extract_config.bytes_to_extract = len; extract_config.offset = offset; uint32_t value = 0; @@ -386,7 +381,7 @@ static void parse_operator(const char* oper, ByteTestData& idx) static const Parameter s_params[] = { { "~count", Parameter::PT_INT, "1:10", nullptr, - "number of bytes to pick up from the buffer" }, + "number of bytes to pick up from the buffer (string can pick less)" }, { "~operator", Parameter::PT_STRING, nullptr, nullptr, "operation to perform to test the value" }, @@ -906,8 +901,117 @@ TEST_CASE("ByteTestOption test", "[ips_byte_test]") ByteTestOption test_5(byte_test); REQUIRE((test_5.eval(current_cursor, &test_packet)) == MATCH); } - } + SECTION("bytes_to_extract bigger than amount of bytes left in the buffer") + { + byte_test.offset = 0; + byte_test.offset_var = -1; + byte_test.bytes_to_extract = 3; + byte_test.string_convert_flag = 0; + byte_test.relative_flag = 1; + uint8_t buff[] = "Hello world long input"; + current_cursor.set("hello_world_long_name", buff, 22); + current_cursor.set_pos(20); + ByteTestOption test_6(byte_test); + REQUIRE((test_6.eval(current_cursor, &test_packet)) == NO_MATCH); + } + + SECTION("String truncation") + { + byte_test.cmp_value = 123; + byte_test.cmp_value_var = -1; + byte_test.bytes_to_extract = 10; + byte_test.opcode = ByteTestOper(0); + byte_test.offset = 0; + byte_test.offset_var = -1; + byte_test.string_convert_flag = 1; + byte_test.relative_flag = 1; + byte_test.bitmask_val = 0; + byte_test.not_flag = 0; + byte_test.base = 10; + uint8_t buff[] = "Hello world long input 123"; + current_cursor.set("hello_world_long_name", buff, 26); + current_cursor.set_pos(23); + ByteTestOption test_7(byte_test); + REQUIRE((test_7.eval(current_cursor, &test_packet)) == MATCH); + } + + SECTION("Negative offset") + { + SECTION("Cursor on the last byte of buffer") + { + byte_test.cmp_value = 32; + byte_test.cmp_value_var = -1; + byte_test.bytes_to_extract = 1; + byte_test.opcode = ByteTestOper(0); + byte_test.offset = -6; + byte_test.offset_var = -1; + byte_test.string_convert_flag = 0; + byte_test.relative_flag = 1; + byte_test.bitmask_val = 0; + byte_test.not_flag = 0; + uint8_t buff[] = "Hello world long input"; + current_cursor.set("hello_world_long_name", buff, 22); + current_cursor.set_pos(22); + ByteTestOption test_8(byte_test); + REQUIRE((test_8.eval(current_cursor, &test_packet)) == MATCH); + } + + SECTION("Cursor on the last byte of buffer, bytes_to_extract is bigger than offset") + { + byte_test.bytes_to_extract = 4; + byte_test.offset = -3; + byte_test.offset_var = -1; + byte_test.relative_flag = 1; + byte_test.string_convert_flag = 0; + uint8_t buff[] = "Hello world long input"; + current_cursor.set("hello_world_long_name", buff, 22); + current_cursor.set_pos(22); + ByteTestOption test_9(byte_test); + REQUIRE((test_9.eval(current_cursor, &test_packet)) == NO_MATCH); + } + + SECTION("Cursor on the last byte of buffer with string flag") + { + byte_test.cmp_value = 123; + byte_test.cmp_value_var = -1; + byte_test.bytes_to_extract = 3; + byte_test.opcode = ByteTestOper(0); + byte_test.offset = -3; + byte_test.offset_var = -1; + byte_test.string_convert_flag = 1; + byte_test.relative_flag = 1; + byte_test.bitmask_val = 0; + byte_test.not_flag = 0; + byte_test.base = 10; + uint8_t buff[] = "Hello world long input 123"; + current_cursor.set("hello_world_long_name", buff, 26); + current_cursor.set_pos(26); + ByteTestOption test_10(byte_test); + REQUIRE((test_10.eval(current_cursor, &test_packet)) == MATCH); + } + + SECTION("String truncation") + { + byte_test.cmp_value = 123; + byte_test.cmp_value_var = -1; + byte_test.bytes_to_extract = 10; + byte_test.opcode = ByteTestOper(0); + byte_test.offset = -3; + byte_test.offset_var = -1; + byte_test.string_convert_flag = 1; + byte_test.relative_flag = 1; + byte_test.bitmask_val = 0; + byte_test.not_flag = 0; + byte_test.base = 10; + uint8_t buff[] = "Hello world long input 123"; + current_cursor.set("hello_world_long_name", buff, 26); + current_cursor.set_pos(26); + ByteTestOption test_11(byte_test); + REQUIRE((test_11.eval(current_cursor, &test_packet)) == MATCH); + } + } + } } TEST_CASE("ByteTestModule test", "[ips_byte_test]")