Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false positive for single or double quotes that are safely escaped #49

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
12 changes: 12 additions & 0 deletions src/helpers/find_all_matches.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
pub fn find_all_matches(haystack: &str, needle: &str) -> Vec<usize> {
let mut positions = Vec::new();
let mut start = 0;

while let Some(pos) = haystack[start..].find(needle) {
let match_pos = start + pos;
positions.push(match_pos);
start = match_pos + 1;
timokoessler marked this conversation as resolved.
Show resolved Hide resolved
}

positions
}
67 changes: 67 additions & 0 deletions src/helpers/find_all_matches_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#[cfg(test)]
mod tests {
use crate::helpers::find_all_matches::find_all_matches;

#[test]
fn test_find_all_matches() {
let haystack = "hello world hello world hello world";
let needle = "world";
let expected = vec![6, 18, 30];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}

#[test]
fn test_no_matches() {
let haystack = "hello world hello world hello world";
let needle = "foo";
let expected = vec![];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}

#[test]
fn test_empty_haystack() {
let haystack = "";
let needle = "world";
let expected = vec![];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}

#[test]
fn test_empty_needle() {
let haystack = "hello world hello world hello world";
let needle = "";
let expected = vec![];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}

#[test]
fn test_empty_haystack_and_needle() {
let haystack = "";
let needle = "";
let expected = vec![];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}

#[test]
fn test_single_char() {
let haystack = "hello world hello world hello world";
let needle = "w";
let expected = vec![6, 18, 30];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}

#[test]
fn test_overlapping_matches() {
let haystack = "aaa";
let needle = "aa";
let expected = vec![0, 1];
let result = find_all_matches(haystack, needle);
assert_eq!(result, expected);
}
}
1 change: 1 addition & 0 deletions src/helpers/mod.rs
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
pub mod diff_in_vec_len;
pub mod find_all_matches;
35 changes: 35 additions & 0 deletions src/sql_injection/detect_sql_injection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use super::have_comments_changed::have_comments_changed;
use super::is_common_sql_string::is_common_sql_string;
use super::tokenize_query::tokenize_query;
use crate::diff_in_vec_len;
use crate::helpers::find_all_matches::find_all_matches;
use sqlparser::tokenizer::Token;

const SPACE_CHAR: char = ' ';
Expand All @@ -26,6 +27,40 @@ pub fn detect_sql_injection_str(query: &str, userinput: &str, dialect: i32) -> b
return false;
}

// Special case for single or double quotes at start and/or end of user input
// Normally if the user input is properly escaped, we wouldn't find an exact match in the query
// However, if the user input is `'value` and the single quote is escaped with another single quote
// `'value` becomes `'''value'` in the query so we still find an exact match
// (vice versa for double quotes)
if userinput.contains("'") || userinput.contains('"') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

starts_with?

let mut matches = find_all_matches(query, userinput).len();

fn is_safely_escaped(input: &str, userinput: &str, quote: char) -> bool {
let quoted_start = format!("{quote}{userinput}");
let quoted_end = format!("{userinput}{quote}");
let fully_quoted = format!("{quote}{userinput}{quote}");

input == quoted_start || input == quoted_end || input == fully_quoted
}

for token in tokens.iter() {
match token {
Token::SingleQuotedString(s) if is_safely_escaped(s, userinput, '\'') => {
matches -= 1
}
Token::DoubleQuotedString(s) if is_safely_escaped(s, userinput, '"') => {
matches -= 1
}
_ => {}
}
}

if matches == 0 {
// All matches are safely escaped, not an injection.
return false;
}
}

// Remove leading and trailing spaces from userinput :
let trimmed_userinput = userinput.trim_matches(SPACE_CHAR);

Expand Down
25 changes: 25 additions & 0 deletions src/sql_injection/detect_sql_injection_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,4 +691,29 @@ mod tests {
);
is_injection!("SELECT * FROM users WHERE id IN (-1,571,639)", "-1,571,639");
}

#[test]
fn test_partial_match_single_quote() {
let starts_with = r#"
SELECT "foo" WHERE "bar" = '''; sleep 15 ;"'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add is injection cases

"#;

// r#"..."# is a raw string literal
not_injection!(starts_with, r#"'; sleep 15 ;""#);
}

#[test]
fn test_multiple_partial_match_single_quote() {
not_injection!("SELECT '''abc', '''abc' FROM table", "'abc");
not_injection!("SELECT 'abc''', 'abc''' FROM table", "abc'");
not_injection!("SELECT '''abc''', '''abc''' FROM table", "'abc'");
}

#[test]
fn test_multiple_partial_match_double_quote() {
// r#"..."# is a raw string literal
not_injection!(r#"SELECT """"abc", """"abc"" FROM table"#, r#""abc"#);
not_injection!(r#"SELECT "abc"""", "abc""" FROM table"#, r#"abc""#);
not_injection!(r#"SELECT """"abc""", """"abc"" FROM table"#, r#""abc"#);
}
}
Loading