diff --git a/HISTORY.md b/HISTORY.md index 62fc2ed6..c1d54bf8 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,9 @@ # Release History +## 1.4.0 + +* Added [DJG101](doc/checks/DJG101.md) Using quoted, parametrized literal will bypass Django SQL Injection protection + ## 1.3.0 * Added [TRY100](doc/checks/TRY100.md) check for try..except..pass statements diff --git a/build.gradle b/build.gradle index bfc8f9e4..47fb9e5a 100644 --- a/build.gradle +++ b/build.gradle @@ -6,7 +6,7 @@ plugins { } group 'org.tonybaloney.security' -version '1.3.0' +version '1.4.0' repositories { mavenCentral() @@ -36,14 +36,9 @@ intellij { patchPluginXml { changeNotes """ -

1.3.0

+

1.4.0

""" } diff --git a/doc/bandit.md b/doc/bandit.md index 9be79f71..321b10f1 100644 --- a/doc/bandit.md +++ b/doc/bandit.md @@ -18,8 +18,8 @@ List of bandit plugins that have equivalent checks in pycharm-security: |-----------------------------------------------------------|----------------------------------|----------------------------| | app_debug.py (Flask Debug) | Yes | [FLK100](checks/FLK100.md) | | asserts.py | Yes | [AST100](checks/AST100.md) | -| crypto_request_no_cert_validation.py (requests no verify) | Yes, for both httpx and requests | [RQ100](checks/RQ100.md)) | -| django_sql_injection.py | No | | +| crypto_request_no_cert_validation.py (requests no verify) | Yes, for both httpx and requests | [RQ100](checks/RQ100.md) | +| django_sql_injection.py | Yes | [DJG101](checks/DJG101.md) | | django_xss.py | No | | | exec.py | Yes | [EX100](checks/EX100.md) | | general_bad_file_permissions.py | Yes | [OS100](checks/OS100.md) | diff --git a/doc/checks/DJG101.md b/doc/checks/DJG101.md new file mode 100644 index 00000000..325c9afc --- /dev/null +++ b/doc/checks/DJG101.md @@ -0,0 +1,57 @@ +# DJG101 + +This check looks for ways that Django SQL Injection protection is being bypassed, by using quoted parameters. + +The check looks at the following use cases: + +* Use of `RawSQL()` constructor directly +* Use of `cursor.execute()` +* Use of `raw()` on a `Manager` instance + +Whilst the methods support parametrized queries, if the `%s` value is quoted with single-quotes, the value is still vulnerable to SQL injection. + +## Example + +The first example is using the RawSQL constructor directly and annotating a query set: + +```python +from django.db.models.expressions import RawSQL + +qs.annotate(val=RawSQL("select col from sometable where othercol = '%s'", (someparam,))) # this is bad! +``` + +Another example is using the `raw()` method on a manager to filter results, exposing SQL injection: + +```python +from django import things +from .models import User + +def my_view(self): + User.objects.raw("SELECT * FROM myapp_person WHERE last_name = '%s'", [lname]) # this is also bad! +``` + +Cursors can also be exploited using the same technique: + +```python +from django.db import connection + +def my_custom_sql(self): + with connection.cursor() as cursor: + cursor.execute("UPDATE bar SET foo = 1 WHERE baz = %s", [self.baz]) + cursor.execute("SELECT foo FROM bar WHERE baz = '%s'", [self.baz]) + row = cursor.fetchone() + return row +``` + +## Fixes + +Remove the quotations from the string values: + +```python +("UPDATE bar SET foo = 1 WHERE baz = %s", [self.baz]) # good +("UPDATE bar SET foo = 1 WHERE baz = '%s'", [self.baz]) # bad! +``` + +## See Also + +* [Official Documentation](https://docs.djangoproject.com/en/3.0/ref/models/expressions/#django.db.models.expressions.RawSQL) \ No newline at end of file diff --git a/doc/checks/index.rst b/doc/checks/index.rst index 9f048ba7..ea014ad0 100644 --- a/doc/checks/index.rst +++ b/doc/checks/index.rst @@ -41,6 +41,7 @@ Django Web Framework :maxdepth: 1 DJG100 : Setting `DEBUG = True` in a `settings.py` file (assumed Django project settings) + DJG101 : Using quoted, parametrized literal will bypass Django SQL Injection protection DJG200 : Django middleware is missing `CsrfViewMiddleware`, which blocks cross-site request forgery DJG201 : Django middleware is missing `XFrameOptionsMiddleware`, which blocks clickjacking. diff --git a/src/main/java/security/Checks.kt b/src/main/java/security/Checks.kt index 609922a5..2531fd6b 100644 --- a/src/main/java/security/Checks.kt +++ b/src/main/java/security/Checks.kt @@ -10,6 +10,7 @@ object Checks { val SubprocessShellCheck = CheckType("PR100", "Calling subprocess commands with shell=True can leave the host shell open to local code execution or remote code execution attacks.") val TempfileMktempCheck = CheckType("TMP100", "The way that tempfile.mktemp creates temporary files is insecure and leaves it open to attackers replacing the file contents. Use tempfile.mkstemp instead.") val DjangoDebugModeCheck = CheckType("DJG100", "Running Django in Debug mode is highly insecure and should only be used for local development purposes.") + val DjangoRawSqlCheck = CheckType("DJG101", "Using quoted, parametrized literal will bypass Django SQL Injection protection.") val DjangoCsrfMiddlewareCheck = CheckType("DJG200", "Django middleware is missing CsrfViewMiddleware, which blocks cross-site request forgery.") val DjangoClickjackMiddlewareCheck = CheckType("DJG201", "Django middleware is missing XFrameOptionsMiddleware, which blocks clickjacking.") val InsecureHashAlgorithms = CheckType("HL100", "MD4, MD5, SHA, and SHA1 hashing algorithms have cryptographically weak algorithms and should not be used for obfuscating or protecting data.") diff --git a/src/main/java/security/helpers/ImportValidators.kt b/src/main/java/security/helpers/ImportValidators.kt new file mode 100644 index 00000000..c05b05cd --- /dev/null +++ b/src/main/java/security/helpers/ImportValidators.kt @@ -0,0 +1,16 @@ +package security.helpers + +import com.jetbrains.python.psi.PyFile + +object ImportValidators { + fun hasImportedNamespace(file: PyFile, match: String) : Boolean { + val imports = file.importBlock ?: return false + if (imports.isEmpty()) return false + for (imp in imports){ + if (imp.fullyQualifiedObjectNames.isEmpty()) continue + if (imp.fullyQualifiedObjectNames.first() == match) return true + if (imp.fullyQualifiedObjectNames.first().startsWith("$match.")) return true + } + return false + } +} \ No newline at end of file diff --git a/src/main/java/security/validators/DjangoRawSqlInspection.kt b/src/main/java/security/validators/DjangoRawSqlInspection.kt new file mode 100644 index 00000000..50f40de3 --- /dev/null +++ b/src/main/java/security/validators/DjangoRawSqlInspection.kt @@ -0,0 +1,51 @@ +package security.validators + +import com.intellij.codeInspection.LocalInspectionToolSession +import com.intellij.codeInspection.ProblemsHolder +import com.intellij.psi.PsiElementVisitor +import com.jetbrains.python.inspections.PyInspection +import com.jetbrains.python.inspections.PyInspectionVisitor +import com.jetbrains.python.psi.PyCallExpression +import com.jetbrains.python.psi.PyFile +import com.jetbrains.python.psi.PyStringLiteralExpression +import security.Checks +import security.helpers.ImportValidators.hasImportedNamespace + +class DjangoRawSqlInspection : PyInspection() { + val check = Checks.DjangoClickjackMiddlewareCheck; + + override fun getStaticDescription(): String? { + return check.getDescription() + } + + override fun buildVisitor(holder: ProblemsHolder, + isOnTheFly: Boolean, + session: LocalInspectionToolSession): PsiElementVisitor = Visitor(holder, session) + + private class Visitor(holder: ProblemsHolder, session: LocalInspectionToolSession) : PyInspectionVisitor(holder, session) { + val methodNames = arrayOf("RawSQL", "raw", "execute") + override fun visitPyCallExpression(node: PyCallExpression?) { + if (node == null) return + val calleeName = node.callee?.name ?: return + if (!listOf(*methodNames).contains(calleeName)) return + + if (node.containingFile !is PyFile) return + if (!hasImportedNamespace(node.containingFile as PyFile, "django")) return + + val sqlStatement = node.arguments.first() ?: return + if (sqlStatement !is PyStringLiteralExpression) return + val param = Regex("%s") + val paramMatches = param.findAll(sqlStatement.stringValue) + for (match in paramMatches){ + try { + if (sqlStatement.stringValue.substring(match.range.start - 1, match.range.start) != "'") return + if (sqlStatement.stringValue.substring(match.range.last + 1, match.range.last + 2) != "'") return + } catch (oobe: StringIndexOutOfBoundsException){ + // End or beginning of string, so this SQL injection technique wouldn't be possible. + return + } + holder?.registerProblem(node, Checks.DjangoRawSqlCheck.getDescription()) + } + } + } +} \ No newline at end of file diff --git a/src/main/java/security/validators/FlaskDebugModeInspection.kt b/src/main/java/security/validators/FlaskDebugModeInspection.kt index 9407d30e..b7360fcf 100644 --- a/src/main/java/security/validators/FlaskDebugModeInspection.kt +++ b/src/main/java/security/validators/FlaskDebugModeInspection.kt @@ -7,8 +7,10 @@ import com.jetbrains.python.inspections.PyInspection import com.jetbrains.python.inspections.PyInspectionVisitor import com.jetbrains.python.psi.PyBoolLiteralExpression import com.jetbrains.python.psi.PyCallExpression +import com.jetbrains.python.psi.PyFile import com.jetbrains.python.psi.PyReferenceExpression import security.Checks +import security.helpers.ImportValidators.hasImportedNamespace class FlaskDebugModeInspection : PyInspection() { val check = Checks.FlaskDebugModeCheck; @@ -25,6 +27,8 @@ class FlaskDebugModeInspection : PyInspection() { override fun visitPyCallExpression(node: PyCallExpression) { val calleeName = node.callee?.name ?: return if (calleeName != "run") return + if (node.containingFile !is PyFile) return + if (!hasImportedNamespace(node.containingFile as PyFile, "flask")) return if (node.firstChild !is PyReferenceExpression) return if ((node.firstChild as PyReferenceExpression).asQualifiedName().toString() != "app.run") return val debugArg = node.getKeywordArgument("debug") ?: return diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml index 27f67114..b2b8fb6e 100644 --- a/src/main/resources/META-INF/plugin.xml +++ b/src/main/resources/META-INF/plugin.xml @@ -31,6 +31,7 @@ + diff --git a/src/test/java/security/helpers/ImportValidatorsTest.kt b/src/test/java/security/helpers/ImportValidatorsTest.kt new file mode 100644 index 00000000..b1f52ab8 --- /dev/null +++ b/src/test/java/security/helpers/ImportValidatorsTest.kt @@ -0,0 +1,67 @@ +package security.helpers + +import com.intellij.openapi.application.ApplicationManager +import com.jetbrains.python.PythonFileType +import com.jetbrains.python.psi.PyFile +import org.junit.jupiter.api.AfterAll +import org.junit.jupiter.api.BeforeAll +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.TestInstance +import security.SecurityTestTask +import security.helpers.ImportValidators.hasImportedNamespace + +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class ImportValidatorsTest: SecurityTestTask() { + + @BeforeAll + override fun setUp() { + super.setUp() + } + + @AfterAll + override fun tearDown(){ + super.tearDown() + } + + @Test + fun `test empty file`(){ + var code = """ + + """.trimIndent() + assertFalse(testHasImport(code, "django")) + } + + @Test + fun `test file with no imports`(){ + var code = """ + x = 1 + """.trimIndent() + assertFalse(testHasImport(code, "django")) + } + + @Test + fun `test simple import`(){ + var code = """ + import django + """.trimIndent() + assertTrue(testHasImport(code, "django")) + } + + @Test + fun `test from import`(){ + var code = """ + from django.db import DbConnection + """.trimIndent() + assertTrue(testHasImport(code, "django")) + } + + private fun testHasImport(code: String, importName: String): Boolean{ + var hasImport: Boolean = false + ApplicationManager.getApplication().runReadAction { + val testFile = this.createLightFile("test.py", PythonFileType.INSTANCE.language, code); + assertNotNull(testFile) + hasImport = hasImportedNamespace(testFile as PyFile, importName) + } + return hasImport + } +} \ No newline at end of file diff --git a/src/test/java/security/validators/DjangoRawSqlInspectionTest.kt b/src/test/java/security/validators/DjangoRawSqlInspectionTest.kt new file mode 100644 index 00000000..0aaea1a1 --- /dev/null +++ b/src/test/java/security/validators/DjangoRawSqlInspectionTest.kt @@ -0,0 +1,108 @@ +package security.validators + +import org.junit.jupiter.api.AfterAll +import org.junit.jupiter.api.BeforeAll +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.TestInstance +import security.Checks +import security.SecurityTestTask + +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class DjangoRawSqlInspectionTest: SecurityTestTask() { + @BeforeAll + override fun setUp() { + super.setUp() + } + + @AfterAll + override fun tearDown(){ + super.tearDown() + } + + @Test + fun `verify description is not empty`(){ + assertFalse(DjangoRawSqlInspection().staticDescription.isNullOrEmpty()) + } + + @Test + fun `test quoted string`(){ + var code = """ + import django.db.models.expressions + x = "injectable string" + django.db.models.expressions.RawSQL("SELECT * FROM foo WHERE ID = '%s'", (x,)) + """.trimIndent() + testCodeCallExpression(code, 1, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } + + @Test + fun `test format non quoted string`(){ + var code = """ + import django.db.models.expressions + x = "injectable string" + django.db.models.expressions.RawSQL("SELECT * FROM foo WHERE ID = %s", (x,)) + """.trimIndent() + testCodeCallExpression(code, 0, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } + + @Test + fun `test some other rawsql method`(){ + var code = """ + RawSQL("SELECT * FROM foo WHERE ID = '%s'", (x,)) + """.trimIndent() + testCodeCallExpression(code, 0, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } + + @Test + fun `test cursor execute with no quotes`(){ + var code = """ + from django.db import connection + + def my_custom_sql(self): + with connection.cursor() as cursor: + cursor.execute("UPDATE bar SET foo = 1 WHERE baz = %s", [self.baz]) + cursor.execute("SELECT foo FROM bar WHERE baz = %s", [self.baz]) + row = cursor.fetchone() + return row + """.trimIndent() + testCodeCallExpression(code, 0, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } + + @Test + fun `test cursor execute with quotes`(){ + var code = """ + from django.db import connection + + def my_custom_sql(self): + with connection.cursor() as cursor: + cursor.execute("UPDATE bar SET foo = 1 WHERE baz = %s", [self.baz]) + cursor.execute("SELECT foo FROM bar WHERE baz = '%s'", [self.baz]) + row = cursor.fetchone() + return row + """.trimIndent() + testCodeCallExpression(code, 1, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } + + @Test + fun `test model raw with quotes`(){ + var code = """ + from django.db import connection + from .models import User + + def my_view(self): + User.objects.raw("SELECT * FROM myapp_person WHERE last_name = '%s'", [lname]) + """.trimIndent() + testCodeCallExpression(code, 1, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } + + @Test + fun `test model raw with no quotes`(){ + var code = """ + from django.db import connection + from .models import User + + def my_view(self): + User.objects.raw("SELECT * FROM myapp_person WHERE last_name = %s", [lname]) + """.trimIndent() + testCodeCallExpression(code, 0, Checks.DjangoRawSqlCheck, "test.py", DjangoRawSqlInspection()) + } +} \ No newline at end of file