From 6aa5076d7b9ec84f4091041758d9e532ad9de3de Mon Sep 17 00:00:00 2001 From: shan-tanu-joshi-3010 Date: Sat, 17 Jan 2026 23:51:16 +0530 Subject: [PATCH 1/2] Fix heap-buffer-overflow in query_string for malformed '%' --- include/crow/query_string.h | 26 +++-- test_fix.cpp | 198 +++++++++++++++++++++++++++++++++++ tests/query_string_tests.cpp | 33 ++++++ 3 files changed, 250 insertions(+), 7 deletions(-) create mode 100644 test_fix.cpp diff --git a/include/crow/query_string.h b/include/crow/query_string.h index a694b4caf3..9fb1ae8c0a 100644 --- a/include/crow/query_string.h +++ b/include/crow/query_string.h @@ -64,23 +64,33 @@ inline int qs_strncmp(const char * s, const char * qs, size_t n) if ( u1 == '+' ) { u1 = ' '; } if ( u1 == '%' ) // easier/safer than scanf { - unyb = static_cast(*s++); - lnyb = static_cast(*s++); - if ( CROW_QS_ISHEX(unyb) && CROW_QS_ISHEX(lnyb) ) + // Check that next two chars exist and are valid hex before reading + if ( CROW_QS_ISHEX(s[0]) && CROW_QS_ISHEX(s[1]) ) + { + unyb = static_cast(*s++); + lnyb = static_cast(*s++); u1 = (CROW_QS_HEX2DEC(unyb) * 16) + CROW_QS_HEX2DEC(lnyb); + } else + { u1 = '\0'; + } } if ( u2 == '+' ) { u2 = ' '; } if ( u2 == '%' ) // easier/safer than scanf { - unyb = static_cast(*qs++); - lnyb = static_cast(*qs++); - if ( CROW_QS_ISHEX(unyb) && CROW_QS_ISHEX(lnyb) ) + // Check that next two chars exist and are valid hex before reading + if ( CROW_QS_ISHEX(qs[0]) && CROW_QS_ISHEX(qs[1]) ) + { + unyb = static_cast(*qs++); + lnyb = static_cast(*qs++); u2 = (CROW_QS_HEX2DEC(unyb) * 16) + CROW_QS_HEX2DEC(lnyb); + } else + { u2 = '\0'; + } } if ( u1 != u2 ) @@ -150,7 +160,9 @@ inline int qs_decode(char * qs) if ( qs[j] == '+' ) { qs[i] = ' '; } else if ( qs[j] == '%' ) // easier/safer than scanf { - if ( ! CROW_QS_ISHEX(qs[j+1]) || ! CROW_QS_ISHEX(qs[j+2]) ) + // Check bounds before reading: ensure j+1 and j+2 are within string + if ( qs[j+1] == '\0' || qs[j+2] == '\0' || + ! CROW_QS_ISHEX(qs[j+1]) || ! CROW_QS_ISHEX(qs[j+2]) ) { qs[i] = '\0'; return i; diff --git a/test_fix.cpp b/test_fix.cpp new file mode 100644 index 0000000000..fcd270a552 --- /dev/null +++ b/test_fix.cpp @@ -0,0 +1,198 @@ +#include +#include + +// Inline the relevant macros and functions from query_string.h for standalone testing +#define CROW_QS_ISHEX(x) ((((x)>='0'&&(x)<='9') || ((x)>='A'&&(x)<='F') || ((x)>='a'&&(x)<='f')) ? 1 : 0) +#define CROW_QS_HEX2DEC(x) (((x)>='0'&&(x)<='9') ? (x)-48 : ((x)>='A'&&(x)<='F') ? (x)-55 : ((x)>='a'&&(x)<='f') ? (x)-87 : 0) +#define CROW_QS_ISQSCHR(x) ((((x)=='=')||((x)=='#')||((x)=='&')||((x)=='\0')) ? 0 : 1) + +// FIXED version of qs_strncmp - with bounds checking +int qs_strncmp_fixed(const char * s, const char * qs, size_t n) +{ + unsigned char u1, u2, unyb, lnyb; + + while(n-- > 0) + { + u1 = static_cast(*s++); + u2 = static_cast(*qs++); + + if ( ! CROW_QS_ISQSCHR(u1) ) { u1 = '\0'; } + if ( ! CROW_QS_ISQSCHR(u2) ) { u2 = '\0'; } + + if ( u1 == '+' ) { u1 = ' '; } + if ( u1 == '%' ) // easier/safer than scanf + { + // FIX: Check that next two chars exist and are valid hex before reading + if ( CROW_QS_ISHEX(s[0]) && CROW_QS_ISHEX(s[1]) ) + { + unyb = static_cast(*s++); + lnyb = static_cast(*s++); + u1 = (CROW_QS_HEX2DEC(unyb) * 16) + CROW_QS_HEX2DEC(lnyb); + } + else + { + u1 = '\0'; + } + } + + if ( u2 == '+' ) { u2 = ' '; } + if ( u2 == '%' ) // easier/safer than scanf + { + // FIX: Check that next two chars exist and are valid hex before reading + if ( CROW_QS_ISHEX(qs[0]) && CROW_QS_ISHEX(qs[1]) ) + { + unyb = static_cast(*qs++); + lnyb = static_cast(*qs++); + u2 = (CROW_QS_HEX2DEC(unyb) * 16) + CROW_QS_HEX2DEC(lnyb); + } + else + { + u2 = '\0'; + } + } + + if ( u1 != u2 ) + return u1 - u2; + if ( u1 == '\0' ) + return 0; + } + if ( CROW_QS_ISQSCHR(*qs) ) + return -1; + else + return 0; +} + +// FIXED version of qs_decode - with bounds checking +int qs_decode_fixed(char * qs) +{ + int i=0, j=0; + + while( CROW_QS_ISQSCHR(qs[j]) ) + { + if ( qs[j] == '+' ) { qs[i] = ' '; } + else if ( qs[j] == '%' ) // easier/safer than scanf + { + // FIX: Check bounds before reading: ensure j+1 and j+2 are within string + if ( qs[j+1] == '\0' || qs[j+2] == '\0' || + ! CROW_QS_ISHEX(qs[j+1]) || ! CROW_QS_ISHEX(qs[j+2]) ) + { + qs[i] = '\0'; + return i; + } + qs[i] = (CROW_QS_HEX2DEC(qs[j+1]) * 16) + CROW_QS_HEX2DEC(qs[j+2]); + j+=2; + } + else + { + qs[i] = qs[j]; + } + i++; j++; + } + qs[i] = '\0'; + + return i; +} + +bool test_malformed_percent() +{ + std::cout << "Testing malformed percent encoding...\n"; + bool passed = true; + + // Test 1: String ending with % + { + std::cout << " Test 1: String ending with '%'... "; + char test1[] = "%"; + int result = qs_decode_fixed(test1); + // Should return 0 and terminate gracefully + if (result == 0 && test1[0] == '\0') { + std::cout << "PASSED\n"; + } else { + std::cout << "FAILED\n"; + passed = false; + } + } + + // Test 2: String with % followed by single hex + { + std::cout << " Test 2: String with '%2' (single hex)... "; + char test2[] = "%2"; + int result = qs_decode_fixed(test2); + // Should return 0 and terminate gracefully + if (result == 0 && test2[0] == '\0') { + std::cout << "PASSED\n"; + } else { + std::cout << "FAILED\n"; + passed = false; + } + } + + // Test 3: String with % followed by invalid hex + { + std::cout << " Test 3: String with '%GG' (invalid hex)... "; + char test3[] = "%GG"; + int result = qs_decode_fixed(test3); + // Should return 0 and terminate gracefully + if (result == 0 && test3[0] == '\0') { + std::cout << "PASSED\n"; + } else { + std::cout << "FAILED\n"; + passed = false; + } + } + + // Test 4: Valid percent encoding still works + { + std::cout << " Test 4: Valid '%20' encoding... "; + char test4[] = "%20"; + int result = qs_decode_fixed(test4); + // Should decode to space character + if (result == 1 && test4[0] == ' ' && test4[1] == '\0') { + std::cout << "PASSED\n"; + } else { + std::cout << "FAILED (got result=" << result << ", char=" << (int)test4[0] << ")\n"; + passed = false; + } + } + + // Test 5: Valid key=value%20more + { + std::cout << " Test 5: 'hello%20world' encoding... "; + char test5[] = "hello%20world"; + int result = qs_decode_fixed(test5); + std::string expected = "hello world"; + if (result == (int)expected.length() && std::string(test5) == expected) { + std::cout << "PASSED\n"; + } else { + std::cout << "FAILED (got '" << test5 << "')\n"; + passed = false; + } + } + + // Test 6: qs_strncmp with malformed percent + { + std::cout << " Test 6: qs_strncmp with trailing '%'... "; + const char* s1 = "%"; + const char* s2 = "test"; + // Should not crash, just compare + int result = qs_strncmp_fixed(s1, s2, 4); + std::cout << "PASSED (no crash, result=" << result << ")\n"; + } + + return passed; +} + +int main() +{ + std::cout << "=== Heap Overflow Fix Verification (Issue #1126) ===\n\n"; + + bool all_passed = test_malformed_percent(); + + std::cout << "\n"; + if (all_passed) { + std::cout << "SUCCESS: All tests passed! The fix prevents heap overflow.\n"; + return 0; + } else { + std::cout << "FAILURE: Some tests failed.\n"; + return 1; + } +} diff --git a/tests/query_string_tests.cpp b/tests/query_string_tests.cpp index 94e98f8633..42209bff9e 100644 --- a/tests/query_string_tests.cpp +++ b/tests/query_string_tests.cpp @@ -42,4 +42,37 @@ TEST_CASE( "query string keys" ) return key == entry.first;}); REQUIRE(exist == true); } +} + +TEST_CASE( "malformed percent encoding - trailing percent" ) +{ + // URL ending with '%' should not cause heap overflow (Issue #1126) + crow::query_string qs1("https://example.com/asdf?%"); + auto keys1 = qs1.keys(); + // Should handle gracefully without crashing + if (!keys1.empty()) { + (void)qs1.get(keys1[0]); + } + + // URL with single hex digit after % + crow::query_string qs2("https://example.com/?key=%2"); + auto keys2 = qs2.keys(); + if (!keys2.empty()) { + (void)qs2.get(keys2[0]); + } + + // URL with % followed by non-hex characters + crow::query_string qs3("https://example.com/?key=%GG"); + auto keys3 = qs3.keys(); + if (!keys3.empty()) { + (void)qs3.get(keys3[0]); + } + + // Valid percent encoding should still work + crow::query_string qs4("https://example.com/?key=%20value"); + auto val = qs4.get("key"); + REQUIRE(val != nullptr); + REQUIRE(std::string(val) == " value"); + + REQUIRE(true); // Test passes if no crash/sanitizer error } \ No newline at end of file From 8347d6343870356966e99659c674aac0f96dad27 Mon Sep 17 00:00:00 2001 From: shan-tanu-joshi-3010 Date: Mon, 19 Jan 2026 12:39:43 +0530 Subject: [PATCH 2/2] Removed intermediate debug file test_fix.cpp --- test_fix.cpp | 198 --------------------------------------------------- 1 file changed, 198 deletions(-) delete mode 100644 test_fix.cpp diff --git a/test_fix.cpp b/test_fix.cpp deleted file mode 100644 index fcd270a552..0000000000 --- a/test_fix.cpp +++ /dev/null @@ -1,198 +0,0 @@ -#include -#include - -// Inline the relevant macros and functions from query_string.h for standalone testing -#define CROW_QS_ISHEX(x) ((((x)>='0'&&(x)<='9') || ((x)>='A'&&(x)<='F') || ((x)>='a'&&(x)<='f')) ? 1 : 0) -#define CROW_QS_HEX2DEC(x) (((x)>='0'&&(x)<='9') ? (x)-48 : ((x)>='A'&&(x)<='F') ? (x)-55 : ((x)>='a'&&(x)<='f') ? (x)-87 : 0) -#define CROW_QS_ISQSCHR(x) ((((x)=='=')||((x)=='#')||((x)=='&')||((x)=='\0')) ? 0 : 1) - -// FIXED version of qs_strncmp - with bounds checking -int qs_strncmp_fixed(const char * s, const char * qs, size_t n) -{ - unsigned char u1, u2, unyb, lnyb; - - while(n-- > 0) - { - u1 = static_cast(*s++); - u2 = static_cast(*qs++); - - if ( ! CROW_QS_ISQSCHR(u1) ) { u1 = '\0'; } - if ( ! CROW_QS_ISQSCHR(u2) ) { u2 = '\0'; } - - if ( u1 == '+' ) { u1 = ' '; } - if ( u1 == '%' ) // easier/safer than scanf - { - // FIX: Check that next two chars exist and are valid hex before reading - if ( CROW_QS_ISHEX(s[0]) && CROW_QS_ISHEX(s[1]) ) - { - unyb = static_cast(*s++); - lnyb = static_cast(*s++); - u1 = (CROW_QS_HEX2DEC(unyb) * 16) + CROW_QS_HEX2DEC(lnyb); - } - else - { - u1 = '\0'; - } - } - - if ( u2 == '+' ) { u2 = ' '; } - if ( u2 == '%' ) // easier/safer than scanf - { - // FIX: Check that next two chars exist and are valid hex before reading - if ( CROW_QS_ISHEX(qs[0]) && CROW_QS_ISHEX(qs[1]) ) - { - unyb = static_cast(*qs++); - lnyb = static_cast(*qs++); - u2 = (CROW_QS_HEX2DEC(unyb) * 16) + CROW_QS_HEX2DEC(lnyb); - } - else - { - u2 = '\0'; - } - } - - if ( u1 != u2 ) - return u1 - u2; - if ( u1 == '\0' ) - return 0; - } - if ( CROW_QS_ISQSCHR(*qs) ) - return -1; - else - return 0; -} - -// FIXED version of qs_decode - with bounds checking -int qs_decode_fixed(char * qs) -{ - int i=0, j=0; - - while( CROW_QS_ISQSCHR(qs[j]) ) - { - if ( qs[j] == '+' ) { qs[i] = ' '; } - else if ( qs[j] == '%' ) // easier/safer than scanf - { - // FIX: Check bounds before reading: ensure j+1 and j+2 are within string - if ( qs[j+1] == '\0' || qs[j+2] == '\0' || - ! CROW_QS_ISHEX(qs[j+1]) || ! CROW_QS_ISHEX(qs[j+2]) ) - { - qs[i] = '\0'; - return i; - } - qs[i] = (CROW_QS_HEX2DEC(qs[j+1]) * 16) + CROW_QS_HEX2DEC(qs[j+2]); - j+=2; - } - else - { - qs[i] = qs[j]; - } - i++; j++; - } - qs[i] = '\0'; - - return i; -} - -bool test_malformed_percent() -{ - std::cout << "Testing malformed percent encoding...\n"; - bool passed = true; - - // Test 1: String ending with % - { - std::cout << " Test 1: String ending with '%'... "; - char test1[] = "%"; - int result = qs_decode_fixed(test1); - // Should return 0 and terminate gracefully - if (result == 0 && test1[0] == '\0') { - std::cout << "PASSED\n"; - } else { - std::cout << "FAILED\n"; - passed = false; - } - } - - // Test 2: String with % followed by single hex - { - std::cout << " Test 2: String with '%2' (single hex)... "; - char test2[] = "%2"; - int result = qs_decode_fixed(test2); - // Should return 0 and terminate gracefully - if (result == 0 && test2[0] == '\0') { - std::cout << "PASSED\n"; - } else { - std::cout << "FAILED\n"; - passed = false; - } - } - - // Test 3: String with % followed by invalid hex - { - std::cout << " Test 3: String with '%GG' (invalid hex)... "; - char test3[] = "%GG"; - int result = qs_decode_fixed(test3); - // Should return 0 and terminate gracefully - if (result == 0 && test3[0] == '\0') { - std::cout << "PASSED\n"; - } else { - std::cout << "FAILED\n"; - passed = false; - } - } - - // Test 4: Valid percent encoding still works - { - std::cout << " Test 4: Valid '%20' encoding... "; - char test4[] = "%20"; - int result = qs_decode_fixed(test4); - // Should decode to space character - if (result == 1 && test4[0] == ' ' && test4[1] == '\0') { - std::cout << "PASSED\n"; - } else { - std::cout << "FAILED (got result=" << result << ", char=" << (int)test4[0] << ")\n"; - passed = false; - } - } - - // Test 5: Valid key=value%20more - { - std::cout << " Test 5: 'hello%20world' encoding... "; - char test5[] = "hello%20world"; - int result = qs_decode_fixed(test5); - std::string expected = "hello world"; - if (result == (int)expected.length() && std::string(test5) == expected) { - std::cout << "PASSED\n"; - } else { - std::cout << "FAILED (got '" << test5 << "')\n"; - passed = false; - } - } - - // Test 6: qs_strncmp with malformed percent - { - std::cout << " Test 6: qs_strncmp with trailing '%'... "; - const char* s1 = "%"; - const char* s2 = "test"; - // Should not crash, just compare - int result = qs_strncmp_fixed(s1, s2, 4); - std::cout << "PASSED (no crash, result=" << result << ")\n"; - } - - return passed; -} - -int main() -{ - std::cout << "=== Heap Overflow Fix Verification (Issue #1126) ===\n\n"; - - bool all_passed = test_malformed_percent(); - - std::cout << "\n"; - if (all_passed) { - std::cout << "SUCCESS: All tests passed! The fix prevents heap overflow.\n"; - return 0; - } else { - std::cout << "FAILURE: Some tests failed.\n"; - return 1; - } -}