Fix false positive on SQLi EOL comments (#330)

DataDog · Aug 2, 2024 · 9332b9d · 9332b9d
1 parent 87d3e77
commit 9332b9d
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 4 deletions.
diff --git a/src/condition/sqli_detector.cpp b/src/condition/sqli_detector.cpp
@@ -80,6 +80,7 @@ std::string strip_literals(std::string_view statement, std::span<sql_token> toke
 
 bool is_query_comment(const std::vector<sql_token> &resource_tokens,
  std::span<sql_token> param_tokens,
+ std::size_t param_index /* the position of the injection on the resource */,
  std::size_t param_tokens_begin /* first index of param_tokens in resource_tokens */)
 
 {
@@ -97,7 +98,13 @@ bool is_query_comment(const std::vector<sql_token> &resource_tokens,
  */
  for (std::size_t i = 0; i < param_tokens.size(); ++i) {
  if (param_tokens[i].type == sql_token_type::eol_comment) {
- return i > 0 || param_tokens_begin < (resource_tokens.size() - 1);
+ if (param_tokens.size() == 1 && param_tokens_begin < (resource_tokens.size() - 1)) {
+ // If the first and only token is the comment, ensure that it was introduced
+ // by the injection itself, rather than it being a partial match
+ return param_index <= resource_tokens[param_tokens_begin].index;
+ }
+
+ return i > 0;
  }
  }
 
@@ -482,7 +489,8 @@ sqli_result sqli_impl(std::string_view resource, std::vector<sql_token> &resourc
  !is_benign_order_by_clause(resource_tokens, param_tokens, param_tokens_begin)) ||
  (param_tokens.size() < min_token_count &&
  (is_where_tautology(resource_tokens, param_tokens, param_tokens_begin) ||
- is_query_comment(resource_tokens, param_tokens, param_tokens_begin)))) {
+ is_query_comment(
+ resource_tokens, param_tokens, param_index, param_tokens_begin)))) {
  return matched_param{std::string(value), it.get_current_path()};
  }
  }

diff --git a/src/condition/sqli_detector.hpp b/src/condition/sqli_detector.hpp
@@ -46,7 +46,7 @@ bool is_where_tautology(const std::vector<sql_token> &resource_tokens,
  std::span<sql_token> param_tokens, std::size_t param_tokens_begin);
 
 bool is_query_comment(const std::vector<sql_token> &resource_tokens,
- std::span<sql_token> param_tokens, std::size_t param_tokens_begin);
+ std::span<sql_token> param_tokens, std::size_t param_index, std::size_t param_tokens_begin);
 
 } // namespace internal
 

diff --git a/tests/sqli_detector_internals_test.cpp b/tests/sqli_detector_internals_test.cpp
@@ -233,6 +233,9 @@ TEST(TestSqliDetectorInternals, IsQueryCommentSuccess)
  {R"(SELECT * FROM ships WHERE id=-- AND password=HASH('str')
  1 OR 1)",
  R"(-- )"},
+ {"-- thisisacomment\nSELECT * FROM ships WHERE id=paco", "-- thisisacomment"},
+ {"SELECT * FROM users WHERE user = 'admin'--' AND password = '' LIMIT 1", "admin'--'"},
+ {"SELECT * FROM Customers WHERE CustomerName = -- AND\nCity = 'Berlin';", "--"},
  };
 
  for (const auto &[statement, param] : samples) {
@@ -245,7 +248,30 @@ TEST(TestSqliDetectorInternals, IsQueryCommentSuccess)
  auto [param_tokens, param_index] =
  internal::get_consecutive_tokens(resource_tokens, param_begin, param_end);
 
- EXPECT_TRUE(internal::is_query_comment(resource_tokens, param_tokens, param_index));
+ EXPECT_TRUE(
+ internal::is_query_comment(resource_tokens, param_tokens, param_begin, param_index));
+ }
+}
+
+TEST(TestSqliDetectorInternals, IsQueryCommentFailure)
+{
+ std::vector<std::pair<std::string, std::string>> samples{
+ {"-- thisisacomment\nSELECT * FROM ships WHERE id=paco", "thisisacomment"},
+ {"SELECT * FROM user WHERE id = 1 -- thisisacomment", "thisisacomment"},
+ };
+
+ for (const auto &[statement, param] : samples) {
+ auto resource_tokens = tokenize(statement);
+
+ auto param_begin = statement.find(param);
+ ASSERT_NE(param_begin, std::string::npos);
+ auto param_end = param_begin + param.size();
+
+ auto [param_tokens, param_index] =
+ internal::get_consecutive_tokens(resource_tokens, param_begin, param_end);
+
+ EXPECT_FALSE(
+ internal::is_query_comment(resource_tokens, param_tokens, param_begin, param_index));
  }
 }