diff --git a/HISTORY.md b/HISTORY.md index 1fed4147..a42f7aa6 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -5,6 +5,7 @@ * Added pickle load inspection [PIC100](doc/checks/PIC100.md) * Added django safe strings inspection [DJG102](doc/checks/DJG102.md) * Added hardcoded temp path read or write inspection [TMP101](doc/checks/TMP101.md) +* Added XML standard library DoS inspection [XML100](doc/checks/XML100.md) * Added XML RPC dotted paths inspection [XML200](doc/checks/XML200.md) ## 1.6.0 diff --git a/build.gradle b/build.gradle index f545f026..328cd54e 100644 --- a/build.gradle +++ b/build.gradle @@ -41,6 +41,7 @@ patchPluginXml {
  • Added pickle load inspection [PIC100]
  • Added django safe strings inspection [DJG102]
  • Added hardcoded temp path read or write inspection [TMP101]
    • +
  • Added XML std library exploit inspection [XML100]
  • Added XML RPC dotted paths inspection [XML200]
  • Added links to documentation in inspection details
    • diff --git a/doc/checks/XML100.md b/doc/checks/XML100.md new file mode 100644 index 00000000..b9a4653b --- /dev/null +++ b/doc/checks/XML100.md @@ -0,0 +1,36 @@ +# XML100 + +If your application ever loads and parses XML files, the odds are you are using one of the XML standard library modules. There are a few common attacks through XML. Mostly DoS-style (designed to crash systems instead of exfiltration of data). Those attacks are common, especially if you’re parsing external (ie non-trusted) XML files. + +One of those is called “billion laughs”, because of the payload normally containing a lot (billions) of “lols”. Basically, the idea is that you can do referential entities in XML, so when your unassuming XML parser tries to load this XML file into memory it consumes gigabytes of RAM. Try it out if you don’t believe me :-) + +```xml + + + + + + + + + + +]> +&lol9; +``` + +Another attack uses external entity expansion. XML supports referencing entities from external URLs, the XML parser would typically fetch and load that resource without any qualms. “An attacker can circumvent firewalls and gain access to restricted resources as all the requests are made from an internal and trustworthy IP address, not from the outside.” + +Another situation to consider is 3rd party packages you’re depending on that decode XML, like configuration files, remote APIs. You might not even be aware that one of your dependencies leaves itself open to these types of attacks. + +So what happens in Python? Well, the standard library modules, etree, DOM, xmlrpc are all wide open to these types of attacks. + +## Fix + +* Use [defusedxml](https://pypi.org/project/defusedxml/) as a drop-in replacement for the standard library modules. It adds safe-guards against these types of attacks. + +## See Also + +* [Official Documentation on XMl vulnerabilities](https://docs.python.org/3/library/xml.html#xml-vulnerabilities) +* [defusedxml package on PyPi](https://pypi.org/project/defusedxml/) \ No newline at end of file diff --git a/doc/checks/index.rst b/doc/checks/index.rst index c0c2fb54..1654f237 100644 --- a/doc/checks/index.rst +++ b/doc/checks/index.rst @@ -92,4 +92,5 @@ XML .. toctree:: :maxdepth: 1 + XML100 : Use of standard library XML modules that are vulnerable to entity expansion DoS XML200 : Use of `allow_dotted_names` with XML RPC Simple Server allows remote code execution diff --git a/src/main/java/security/Checks.kt b/src/main/java/security/Checks.kt index 9bb5e99f..20581de2 100644 --- a/src/main/java/security/Checks.kt +++ b/src/main/java/security/Checks.kt @@ -28,6 +28,7 @@ object Checks { val BindAllInterfacesCheck = CheckType("NET100", "Possible hardcoded binding to all network interfaces.") val ChmodInsecurePermissionsCheck = CheckType("OS100", "Modification of system files to allow execution.") val PickleLoadCheck = CheckType("PIC100", "Loading serialized data with the pickle module can expose arbitrary code execution using the __reduce__ method.") + val StandardLibraryXmlCheck = CheckType("XML100", "The xml modules in the Python standard library are not secure against maliciously constructed data.") val XmlRpcServerDottedNamesCheck = CheckType("XML200", "Using allow_dotted_names option may allow attackers to execute arbitrary code.") class CheckType(var Code: String, private var Message: String) { diff --git a/src/main/java/security/validators/StandardLibraryXmlInspection.kt b/src/main/java/security/validators/StandardLibraryXmlInspection.kt new file mode 100644 index 00000000..ebe61ccb --- /dev/null +++ b/src/main/java/security/validators/StandardLibraryXmlInspection.kt @@ -0,0 +1,44 @@ +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.PyFromImportStatement +import com.jetbrains.python.psi.PyImportStatement +import security.Checks + +class StandardLibraryXmlInspection : PyInspection() { + val check = Checks.StandardLibraryXmlCheck + + override fun getStaticDescription(): String? { + return check.getStaticDescription() + } + + override fun buildVisitor(holder: ProblemsHolder, + isOnTheFly: Boolean, + session: LocalInspectionToolSession): PsiElementVisitor = Visitor(holder, session) + + private class Visitor(holder: ProblemsHolder, session: LocalInspectionToolSession) : PyInspectionVisitor(holder, session) { + val vulnerableNamespaces = arrayOf("xml.sax", "xml.etree", "xml.dom.minidom", "xml.dom.pulldom", "xmlrpc.server") + + private fun match(qname: String): Boolean { + return (listOf(*vulnerableNamespaces).any{ qname.startsWith(it) }) + } + + override fun visitPyFromImportStatement(node: PyFromImportStatement) { + if (node.importSourceQName == null) return + if (node.importSourceQName!!.toString().isEmpty()) return + if (match(node.importSourceQName!!.toString())) + holder?.registerProblem(node, Checks.StandardLibraryXmlCheck.getDescription()) + } + + override fun visitPyImportStatement(node: PyImportStatement) { + if (node.fullyQualifiedObjectNames.isEmpty()) return + node.fullyQualifiedObjectNames + .filter { match(it) } + .forEach { holder?.registerProblem(node, Checks.StandardLibraryXmlCheck.getDescription()) } + } + } +} \ No newline at end of file diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml index a6a8078a..f9ec2198 100644 --- a/src/main/resources/META-INF/plugin.xml +++ b/src/main/resources/META-INF/plugin.xml @@ -1,15 +1,17 @@ org.tonybaloney.security.pycharm-security Python Security - Anthony Shaw + Anthony Shaw Reports on installed packages and any known security reports.
    + Also available for your CI/CD as a GitHub Action on the GitHub marketplace. +

    Features

      -
    • Over 20 builtin code checks giving your contextual security warnings in your code
      • +
    • Over 40 builtin code checks giving your contextual security warnings in your code
    • Misconfiguration warnings for Django and Flask web frameworks
    • Cross-Site-Scripting detection for both Jinja2 and Mako templating engines
    • SQL Injection detection in all Python string formats
      • @@ -53,6 +55,7 @@ + diff --git a/src/test/java/security/SecurityTestTask.kt b/src/test/java/security/SecurityTestTask.kt index a12024c1..35f21c33 100644 --- a/src/test/java/security/SecurityTestTask.kt +++ b/src/test/java/security/SecurityTestTask.kt @@ -21,7 +21,7 @@ import org.mockito.Mockito import security.helpers.QualifiedNames open class SecurityTestTask: BasePlatformTestCase() { - inline fun testCodeAssignmentStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + fun testCodeAssignmentStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ ApplicationManager.getApplication().runReadAction { val mockHolder = mock { on { registerProblem(any(), contains(check.Code), anyVararg()) } doAnswer {} @@ -79,7 +79,7 @@ open class SecurityTestTask: BasePlatformTestCase() { } } - inline fun testBinaryExpression(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + fun testBinaryExpression(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ ApplicationManager.getApplication().runReadAction { val mockHolder = mock { on { registerProblem(any(), contains(check.Code), anyVararg()) } doAnswer {} @@ -101,7 +101,7 @@ open class SecurityTestTask: BasePlatformTestCase() { } } - inline fun testStringLiteralExpression(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + fun testStringLiteralExpression(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ ApplicationManager.getApplication().runReadAction { val mockHolder = mock { on { registerProblem(any(), contains(check.Code), anyVararg()) } doAnswer {} @@ -123,7 +123,7 @@ open class SecurityTestTask: BasePlatformTestCase() { } } - inline fun testFormattedStringElement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + fun testFormattedStringElement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ ApplicationManager.getApplication().runReadAction { val mockHolder = mock { on { registerProblem(any(), contains(check.Code), anyVararg()) } doAnswer {} @@ -145,7 +145,7 @@ open class SecurityTestTask: BasePlatformTestCase() { } } - inline fun testAssertStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + fun testAssertStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ ApplicationManager.getApplication().runReadAction { val mockHolder = mock { on { registerProblem(any(), contains(check.Code), any()) } doAnswer {} @@ -167,7 +167,7 @@ open class SecurityTestTask: BasePlatformTestCase() { } } - inline fun testTryExceptStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + fun testTryExceptStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ ApplicationManager.getApplication().runReadAction { val mockHolder = mock { on { registerProblem(any(), contains(check.Code), any()) } doAnswer {} @@ -188,4 +188,48 @@ open class SecurityTestTask: BasePlatformTestCase() { Mockito.verify(mockLocalSession, Mockito.times(1)).file } } + + fun testImportStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + ApplicationManager.getApplication().runReadAction { + val mockHolder = mock { + on { registerProblem(any(), contains(check.Code), anyVararg()) } doAnswer {} + } + val testFile = this.createLightFile(filename, PythonFileType.INSTANCE.language, code); + val mockLocalSession = mock { + on { file } doReturn (testFile) + } + assertNotNull(testFile) + val testVisitor = instance.buildVisitor(mockHolder, true, mockLocalSession) as PyInspectionVisitor + + val expr: @NotNull MutableCollection = PsiTreeUtil.findChildrenOfType(testFile, PyImportStatement::class.java) + assertNotNull(expr) + expr.forEach { e -> + testVisitor.visitPyImportStatement(e) + } + Mockito.verify(mockHolder, Mockito.times(times)).registerProblem(any(), contains(check.Code), anyVararg()) + Mockito.verify(mockLocalSession, Mockito.times(1)).file + } + } + + fun testFromImportStatement(code: String, times: Int = 1, check: Checks.CheckType, filename: String = "test.py", instance: inspector){ + ApplicationManager.getApplication().runReadAction { + val mockHolder = mock { + on { registerProblem(any(), contains(check.Code), anyVararg()) } doAnswer {} + } + val testFile = this.createLightFile(filename, PythonFileType.INSTANCE.language, code); + val mockLocalSession = mock { + on { file } doReturn (testFile) + } + assertNotNull(testFile) + val testVisitor = instance.buildVisitor(mockHolder, true, mockLocalSession) as PyInspectionVisitor + + val expr: @NotNull MutableCollection = PsiTreeUtil.findChildrenOfType(testFile, PyFromImportStatement::class.java) + assertNotNull(expr) + expr.forEach { e -> + testVisitor.visitPyFromImportStatement(e) + } + Mockito.verify(mockHolder, Mockito.times(times)).registerProblem(any(), contains(check.Code), anyVararg()) + Mockito.verify(mockLocalSession, Mockito.times(1)).file + } + } } \ No newline at end of file diff --git a/src/test/java/security/validators/FlaskDebugModeInspectionTest.kt b/src/test/java/security/validators/FlaskDebugModeInspectionTest.kt index cfdecfab..b4ed4ca9 100644 --- a/src/test/java/security/validators/FlaskDebugModeInspectionTest.kt +++ b/src/test/java/security/validators/FlaskDebugModeInspectionTest.kt @@ -89,4 +89,24 @@ class FlaskDebugModeInspectionTest: SecurityTestTask() { """.trimIndent() testCodeCallExpression(code, 0, FlaskDebugModeCheck, "test.py", FlaskDebugModeInspection()) } + + @Test + fun `test flask not imported`(){ + var code = """ + from banana import Flask + + app = Flask() + blah.run() + """.trimIndent() + testCodeCallExpression(code, 0, FlaskDebugModeCheck, "test.py", FlaskDebugModeInspection()) + } + + @Test + fun `test not reference parent`(){ + var code = """ + from flask import Flask + "1".run() + """.trimIndent() + testCodeCallExpression(code, 0, FlaskDebugModeCheck, "test.py", FlaskDebugModeInspection()) + } } \ No newline at end of file diff --git a/src/test/java/security/validators/StandardLibraryXmlInspectionTest.kt b/src/test/java/security/validators/StandardLibraryXmlInspectionTest.kt new file mode 100644 index 00000000..b89c373a --- /dev/null +++ b/src/test/java/security/validators/StandardLibraryXmlInspectionTest.kt @@ -0,0 +1,83 @@ +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 StandardLibraryXmlInspectionTest: SecurityTestTask() { + @BeforeAll + override fun setUp() { + super.setUp() + } + + @AfterAll + override fun tearDown(){ + super.tearDown() + } + + @Test + fun `verify description is not empty`(){ + assertFalse(StandardLibraryXmlInspection().staticDescription.isNullOrEmpty()) + } + + @Test + fun `test invalid import`(){ + var code = """ + from import x + """.trimIndent() + testFromImportStatement(code, 0, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } + + @Test + fun `test from import`(){ + var code = """ + from xml.dom.minidom import parse, parseString + """.trimIndent() + testFromImportStatement(code, 1, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } + + @Test + fun `test normal import minidom`(){ + var code = """ + import xml.dom.minidom + """.trimIndent() + testImportStatement(code, 1, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } + + @Test + fun `test normal safe namespace`(){ + var code = """ + import defusedxml.minidom + """.trimIndent() + testImportStatement(code, 0, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } + + @Test + fun `test alias import`(){ + var code = """ + import xml.etree.ElementTree as ET + """.trimIndent() + testImportStatement(code, 1, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } + + @Test + fun `test multiple import`(){ + var code = """ + import xml.etree.ElementTree, xml.dom.minidom + """.trimIndent() + testImportStatement(code, 2, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } + + @Test + fun `test nested import`(){ + var code = """ + def foo(): + import xml.etree.ElementTree as ET + """.trimIndent() + testImportStatement(code, 1, Checks.StandardLibraryXmlCheck, "test_foo.py", StandardLibraryXmlInspection()) + } +} \ No newline at end of file