From 6b815bd0b94446b2675e43dc1f0d61b4086be571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Fabianski?= Date: Fri, 15 Dec 2023 09:41:20 +0100 Subject: [PATCH] improve gosec sqli --- rules/go/gosec/sql/concat_sqli.yml | 21 ++++- .../concat_sqli/__snapshots__/test.js.snap | 94 +++++++++++++------ .../go/gosec/sql/concat_sqli/testdata/main.go | 14 +++ 3 files changed, 96 insertions(+), 33 deletions(-) diff --git a/rules/go/gosec/sql/concat_sqli.yml b/rules/go/gosec/sql/concat_sqli.yml index 415b58163..223540b14 100644 --- a/rules/go/gosec/sql/concat_sqli.yml +++ b/rules/go/gosec/sql/concat_sqli.yml @@ -5,11 +5,14 @@ patterns: $.Query($) filters: - variable: INPUT - detection: go_gosec_sql_concat_sqli_sanitized_input + detection: go_gosec_sql_concat_sqli_unsanitized_input - not: variable: INPUT detection: go_gosec_sql_concat_sqli_input_sprintf_sanitizer - either: + - variable: DB + detection: go_gosec_sql_concat_sqli_sql_db_init + scope: cursor - variable: DB detection: go_gosec_sql_concat_sqli_sql_open scope: cursor @@ -20,7 +23,7 @@ patterns: $.QueryContext($<...>$) filters: - variable: INPUT - detection: go_gosec_sql_concat_sqli_sanitized_input + detection: go_gosec_sql_concat_sqli_unsanitized_input - either: - variable: DB detection: go_gosec_sql_concat_sqli_sql_open @@ -38,7 +41,7 @@ auxiliary: - id: go_gosec_sql_concat_sqli_input_sanitizer patterns: - pattern: $<_>.QuoteIdentifier($$<_>) - - id: go_gosec_sql_concat_sqli_sanitized_input + - id: go_gosec_sql_concat_sqli_unsanitized_input sanitizer: go_gosec_sql_concat_sqli_input_sanitizer patterns: - pattern: $ @@ -46,6 +49,18 @@ auxiliary: - variable: INPUT detection: go_shared_lang_dynamic_request_input scope: cursor + - id: go_gosec_sql_concat_sqli_sql_db_init + patterns: + - pattern: var $$<_> $.DB + filters: + - variable: SQL + detection: go_gosec_sql_concat_sqli_sql_init + scope: cursor + - pattern: var $$<_> *$.DB + filters: + - variable: SQL + detection: go_gosec_sql_concat_sqli_sql_init + scope: cursor - id: go_gosec_sql_concat_sqli_sql_db_begin patterns: - pattern: $.Begin() diff --git a/tests/go/gosec/sql/concat_sqli/__snapshots__/test.js.snap b/tests/go/gosec/sql/concat_sqli/__snapshots__/test.js.snap index 39529ac83..9a9b96387 100644 --- a/tests/go/gosec/sql/concat_sqli/__snapshots__/test.js.snap +++ b/tests/go/gosec/sql/concat_sqli/__snapshots__/test.js.snap @@ -11,27 +11,27 @@ exports[`go_gosec_sql_concat_sqli test 1`] = ` "title": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')", "description": "## Description\\n\\nSQL Injection represents a severe vulnerability that can culminate in the compromise of data or entire systems. When SQL query strings are crafted dynamically based on user inputs, there's potential for malicious users to manipulate the logic of the SQL statement. Such tampering could provide adversaries unauthorized access to sensitive data or even allow them to execute system-level operations or code.\\n\\n## Remediations\\n\\n✅ Use Parameterized Queries\\n\\nAlways opt for parameterized queries over dynamically generated SQL queries to prevent SQL injection.\\n\\n\`\`\`go\\nrows, err := db.Query(\\"SELECT * FROM users WHERE userName = ?\\", userName)\\nif err != nil {\\n return nil, err\\n}\\ndefer rows.Close()\\nfor rows.Next() {\\n // ... process rows\\n}\\n\`\`\`\\n\\n✅ Avoid Direct User Input in Dynamic Queries\\n\\nIf there's an absolute need to formulate dynamic queries, ensure that direct user input is never utilized. Instead, leverage a map or dictionary containing valid values and determine them through a user-provided key.\\n\\nFor instance, certain database drivers do not support parameterized queries for operators like \`>\` or \`<\`. Instead of directly using user-input values, allow users to provide substitutes like \`gt\` for \`>\` and \`lt\` for \`<\`. Subsequently, use these alphabetical inputs to retrieve the actual operators for your query. Implement a similar approach for queries requiring non-parameterizable column or table names.\\n\\n## Resources\\n\\n- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)\\n", "documentation_url": "https://docs.bearer.com/reference/rules/go_gosec_sql_concat_sqli", - "line_number": 14, + "line_number": 15, "full_filename": "/tmp/bearer-scan/main.go", "filename": ".", "source": { - "start": 14, - "end": 14, + "start": 15, + "end": 15, "column": { "start": 15, "end": 71 } }, "sink": { - "start": 14, - "end": 14, + "start": 15, + "end": 15, "column": { "start": 15, "end": 71 }, "content": "db.Query(\\"SELECT * FROM foo WHERE name = \\" + os.Args[1])" }, - "parent_line_number": 14, + "parent_line_number": 15, "snippet": "db.Query(\\"SELECT * FROM foo WHERE name = \\" + os.Args[1])", "fingerprint": "ca1a3415f7df18be8dec85c9d74eb890_0", "old_fingerprint": "3350ddd230bfd667e45a89b42b9b1b39_0", @@ -45,27 +45,27 @@ exports[`go_gosec_sql_concat_sqli test 1`] = ` "title": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')", "description": "## Description\\n\\nSQL Injection represents a severe vulnerability that can culminate in the compromise of data or entire systems. When SQL query strings are crafted dynamically based on user inputs, there's potential for malicious users to manipulate the logic of the SQL statement. Such tampering could provide adversaries unauthorized access to sensitive data or even allow them to execute system-level operations or code.\\n\\n## Remediations\\n\\n✅ Use Parameterized Queries\\n\\nAlways opt for parameterized queries over dynamically generated SQL queries to prevent SQL injection.\\n\\n\`\`\`go\\nrows, err := db.Query(\\"SELECT * FROM users WHERE userName = ?\\", userName)\\nif err != nil {\\n return nil, err\\n}\\ndefer rows.Close()\\nfor rows.Next() {\\n // ... process rows\\n}\\n\`\`\`\\n\\n✅ Avoid Direct User Input in Dynamic Queries\\n\\nIf there's an absolute need to formulate dynamic queries, ensure that direct user input is never utilized. Instead, leverage a map or dictionary containing valid values and determine them through a user-provided key.\\n\\nFor instance, certain database drivers do not support parameterized queries for operators like \`>\` or \`<\`. Instead of directly using user-input values, allow users to provide substitutes like \`gt\` for \`>\` and \`lt\` for \`<\`. Subsequently, use these alphabetical inputs to retrieve the actual operators for your query. Implement a similar approach for queries requiring non-parameterizable column or table names.\\n\\n## Resources\\n\\n- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)\\n", "documentation_url": "https://docs.bearer.com/reference/rules/go_gosec_sql_concat_sqli", - "line_number": 26, + "line_number": 27, "full_filename": "/tmp/bearer-scan/main.go", "filename": ".", "source": { - "start": 26, - "end": 26, + "start": 27, + "end": 27, "column": { "start": 15, "end": 71 } }, "sink": { - "start": 26, - "end": 26, + "start": 27, + "end": 27, "column": { "start": 15, "end": 71 }, "content": "db.Query(\\"select * from foo where name = \\" + os.Args[1])" }, - "parent_line_number": 26, + "parent_line_number": 27, "snippet": "db.Query(\\"select * from foo where name = \\" + os.Args[1])", "fingerprint": "ca1a3415f7df18be8dec85c9d74eb890_1", "old_fingerprint": "3350ddd230bfd667e45a89b42b9b1b39_1", @@ -79,27 +79,27 @@ exports[`go_gosec_sql_concat_sqli test 1`] = ` "title": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')", "description": "## Description\\n\\nSQL Injection represents a severe vulnerability that can culminate in the compromise of data or entire systems. When SQL query strings are crafted dynamically based on user inputs, there's potential for malicious users to manipulate the logic of the SQL statement. Such tampering could provide adversaries unauthorized access to sensitive data or even allow them to execute system-level operations or code.\\n\\n## Remediations\\n\\n✅ Use Parameterized Queries\\n\\nAlways opt for parameterized queries over dynamically generated SQL queries to prevent SQL injection.\\n\\n\`\`\`go\\nrows, err := db.Query(\\"SELECT * FROM users WHERE userName = ?\\", userName)\\nif err != nil {\\n return nil, err\\n}\\ndefer rows.Close()\\nfor rows.Next() {\\n // ... process rows\\n}\\n\`\`\`\\n\\n✅ Avoid Direct User Input in Dynamic Queries\\n\\nIf there's an absolute need to formulate dynamic queries, ensure that direct user input is never utilized. Instead, leverage a map or dictionary containing valid values and determine them through a user-provided key.\\n\\nFor instance, certain database drivers do not support parameterized queries for operators like \`>\` or \`<\`. Instead of directly using user-input values, allow users to provide substitutes like \`gt\` for \`>\` and \`lt\` for \`<\`. Subsequently, use these alphabetical inputs to retrieve the actual operators for your query. Implement a similar approach for queries requiring non-parameterizable column or table names.\\n\\n## Resources\\n\\n- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)\\n", "documentation_url": "https://docs.bearer.com/reference/rules/go_gosec_sql_concat_sqli", - "line_number": 38, + "line_number": 39, "full_filename": "/tmp/bearer-scan/main.go", "filename": ".", "source": { - "start": 38, - "end": 38, + "start": 39, + "end": 39, "column": { "start": 15, "end": 98 } }, "sink": { - "start": 38, - "end": 38, + "start": 39, + "end": 39, "column": { "start": 15, "end": 98 }, "content": "db.QueryContext(context.Background(), \\"select * from foo where name = \\"+os.Args[1])" }, - "parent_line_number": 38, + "parent_line_number": 39, "snippet": "db.QueryContext(context.Background(), \\"select * from foo where name = \\"+os.Args[1])", "fingerprint": "ca1a3415f7df18be8dec85c9d74eb890_2", "old_fingerprint": "3350ddd230bfd667e45a89b42b9b1b39_2", @@ -113,27 +113,27 @@ exports[`go_gosec_sql_concat_sqli test 1`] = ` "title": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')", "description": "## Description\\n\\nSQL Injection represents a severe vulnerability that can culminate in the compromise of data or entire systems. When SQL query strings are crafted dynamically based on user inputs, there's potential for malicious users to manipulate the logic of the SQL statement. Such tampering could provide adversaries unauthorized access to sensitive data or even allow them to execute system-level operations or code.\\n\\n## Remediations\\n\\n✅ Use Parameterized Queries\\n\\nAlways opt for parameterized queries over dynamically generated SQL queries to prevent SQL injection.\\n\\n\`\`\`go\\nrows, err := db.Query(\\"SELECT * FROM users WHERE userName = ?\\", userName)\\nif err != nil {\\n return nil, err\\n}\\ndefer rows.Close()\\nfor rows.Next() {\\n // ... process rows\\n}\\n\`\`\`\\n\\n✅ Avoid Direct User Input in Dynamic Queries\\n\\nIf there's an absolute need to formulate dynamic queries, ensure that direct user input is never utilized. Instead, leverage a map or dictionary containing valid values and determine them through a user-provided key.\\n\\nFor instance, certain database drivers do not support parameterized queries for operators like \`>\` or \`<\`. Instead of directly using user-input values, allow users to provide substitutes like \`gt\` for \`>\` and \`lt\` for \`<\`. Subsequently, use these alphabetical inputs to retrieve the actual operators for your query. Implement a similar approach for queries requiring non-parameterizable column or table names.\\n\\n## Resources\\n\\n- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)\\n", "documentation_url": "https://docs.bearer.com/reference/rules/go_gosec_sql_concat_sqli", - "line_number": 55, + "line_number": 56, "full_filename": "/tmp/bearer-scan/main.go", "filename": ".", "source": { - "start": 55, - "end": 55, + "start": 56, + "end": 56, "column": { "start": 15, "end": 98 } }, "sink": { - "start": 55, - "end": 55, + "start": 56, + "end": 56, "column": { "start": 15, "end": 98 }, "content": "tx.QueryContext(context.Background(), \\"select * from foo where name = \\"+os.Args[1])" }, - "parent_line_number": 55, + "parent_line_number": 56, "snippet": "tx.QueryContext(context.Background(), \\"select * from foo where name = \\"+os.Args[1])", "fingerprint": "ca1a3415f7df18be8dec85c9d74eb890_3", "old_fingerprint": "3350ddd230bfd667e45a89b42b9b1b39_3", @@ -147,31 +147,65 @@ exports[`go_gosec_sql_concat_sqli test 1`] = ` "title": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')", "description": "## Description\\n\\nSQL Injection represents a severe vulnerability that can culminate in the compromise of data or entire systems. When SQL query strings are crafted dynamically based on user inputs, there's potential for malicious users to manipulate the logic of the SQL statement. Such tampering could provide adversaries unauthorized access to sensitive data or even allow them to execute system-level operations or code.\\n\\n## Remediations\\n\\n✅ Use Parameterized Queries\\n\\nAlways opt for parameterized queries over dynamically generated SQL queries to prevent SQL injection.\\n\\n\`\`\`go\\nrows, err := db.Query(\\"SELECT * FROM users WHERE userName = ?\\", userName)\\nif err != nil {\\n return nil, err\\n}\\ndefer rows.Close()\\nfor rows.Next() {\\n // ... process rows\\n}\\n\`\`\`\\n\\n✅ Avoid Direct User Input in Dynamic Queries\\n\\nIf there's an absolute need to formulate dynamic queries, ensure that direct user input is never utilized. Instead, leverage a map or dictionary containing valid values and determine them through a user-provided key.\\n\\nFor instance, certain database drivers do not support parameterized queries for operators like \`>\` or \`<\`. Instead of directly using user-input values, allow users to provide substitutes like \`gt\` for \`>\` and \`lt\` for \`<\`. Subsequently, use these alphabetical inputs to retrieve the actual operators for your query. Implement a similar approach for queries requiring non-parameterizable column or table names.\\n\\n## Resources\\n\\n- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)\\n", "documentation_url": "https://docs.bearer.com/reference/rules/go_gosec_sql_concat_sqli", - "line_number": 70, + "line_number": 71, "full_filename": "/tmp/bearer-scan/main.go", "filename": ".", "source": { - "start": 70, - "end": 70, + "start": 71, + "end": 71, "column": { "start": 15, "end": 75 } }, "sink": { - "start": 70, - "end": 70, + "start": 71, + "end": 71, "column": { "start": 15, "end": 75 }, "content": "db.Query(\\"SELECT * FROM foo\\" + \\"WHERE name = \\" + os.Args[1])" }, - "parent_line_number": 70, + "parent_line_number": 71, "snippet": "db.Query(\\"SELECT * FROM foo\\" + \\"WHERE name = \\" + os.Args[1])", "fingerprint": "ca1a3415f7df18be8dec85c9d74eb890_4", "old_fingerprint": "3350ddd230bfd667e45a89b42b9b1b39_4", "code_extract": "\\trows, err := db.Query(\\"SELECT * FROM foo\\" + \\"WHERE name = \\" + os.Args[1])" + }, + { + "cwe_ids": [ + "89" + ], + "id": "go_gosec_sql_concat_sqli", + "title": "Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')", + "description": "## Description\\n\\nSQL Injection represents a severe vulnerability that can culminate in the compromise of data or entire systems. When SQL query strings are crafted dynamically based on user inputs, there's potential for malicious users to manipulate the logic of the SQL statement. Such tampering could provide adversaries unauthorized access to sensitive data or even allow them to execute system-level operations or code.\\n\\n## Remediations\\n\\n✅ Use Parameterized Queries\\n\\nAlways opt for parameterized queries over dynamically generated SQL queries to prevent SQL injection.\\n\\n\`\`\`go\\nrows, err := db.Query(\\"SELECT * FROM users WHERE userName = ?\\", userName)\\nif err != nil {\\n return nil, err\\n}\\ndefer rows.Close()\\nfor rows.Next() {\\n // ... process rows\\n}\\n\`\`\`\\n\\n✅ Avoid Direct User Input in Dynamic Queries\\n\\nIf there's an absolute need to formulate dynamic queries, ensure that direct user input is never utilized. Instead, leverage a map or dictionary containing valid values and determine them through a user-provided key.\\n\\nFor instance, certain database drivers do not support parameterized queries for operators like \`>\` or \`<\`. Instead of directly using user-input values, allow users to provide substitutes like \`gt\` for \`>\` and \`lt\` for \`<\`. Subsequently, use these alphabetical inputs to retrieve the actual operators for your query. Implement a similar approach for queries requiring non-parameterizable column or table names.\\n\\n## Resources\\n\\n- [OWASP SQL Injection Prevention Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html)\\n", + "documentation_url": "https://docs.bearer.com/reference/rules/go_gosec_sql_concat_sqli", + "line_number": 142, + "full_filename": "/tmp/bearer-scan/main.go", + "filename": ".", + "source": { + "start": 142, + "end": 142, + "column": { + "start": 15, + "end": 38 + } + }, + "sink": { + "start": 142, + "end": 142, + "column": { + "start": 15, + "end": 38 + }, + "content": "DB.Query(getProfileSql)" + }, + "parent_line_number": 142, + "snippet": "DB.Query(getProfileSql)", + "fingerprint": "ca1a3415f7df18be8dec85c9d74eb890_5", + "old_fingerprint": "3350ddd230bfd667e45a89b42b9b1b39_5", + "code_extract": "\\trows, err := DB.Query(getProfileSql)" } ] }" diff --git a/tests/go/gosec/sql/concat_sqli/testdata/main.go b/tests/go/gosec/sql/concat_sqli/testdata/main.go index c4b38d2e6..f8f3fd519 100644 --- a/tests/go/gosec/sql/concat_sqli/testdata/main.go +++ b/tests/go/gosec/sql/concat_sqli/testdata/main.go @@ -3,6 +3,7 @@ package main import ( "context" "database/sql" + "fmt" "os" ) @@ -127,3 +128,16 @@ func foo9() { } defer rows.Close() } + +var DB *sql.DB +var err error + +func foo10(uid string) { + DB, err = database.Connect() + + getProfileSql := fmt.Sprintf(`SELECT p.user_id, p.full_name, p.city, p.phone_number + FROM Profile as p,Users as u + where p.user_id = u.id + and u.id=%s`, uid) //here is the vulnerable query + rows, err := DB.Query(getProfileSql) +}