Fix #36, lint for insecure itsdangerous kwarg usage

CHANGELOG.md

@@ -5,6 +5,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Added
+- `DUO137`: lint for insecure itsdangerous kwarg usage ([#36](https://github.com/duo-labs/dlint/issues/36))
 
 ## [0.9.2] - 2019-11-21
 ### Fixed

dlint/linters/__init__.py

@@ -19,6 +19,7 @@
 from .bad_exec_use import BadExecUseLinter
 from .bad_hashlib_use import BadHashlibUseLinter
 from .bad_input_use import BadInputUseLinter
+from .bad_itsdangerous_kwarg_use import BadItsDangerousKwargUseLinter
 from .bad_marshal_use import BadMarshalUseLinter
 from .bad_onelogin_kwarg_use import BadOneLoginKwargUseLinter
 from .bad_onelogin_module_attribute_use import BadOneLoginModuleAttributeUseLinter
@@ -58,6 +59,7 @@
     BadExecUseLinter,
     BadHashlibUseLinter,
     BadInputUseLinter,
+    BadItsDangerousKwargUseLinter,
     BadMarshalUseLinter,
     BadOneLoginKwargUseLinter,
     BadOneLoginModuleAttributeUseLinter,

dlint/linters/bad_itsdangerous_kwarg_use.py

@@ -0,0 +1,85 @@
+#!/usr/bin/env python
+
+from __future__ import (
+    absolute_import,
+    division,
+    print_function,
+    unicode_literals,
+)
+
+import functools
+
+from .helpers import bad_kwarg_use
+
+from .. import tree
+
+
+class BadItsDangerousKwargUseLinter(bad_kwarg_use.BadKwargUseLinter):
+    """This linter looks for unsafe use of itsdangerous keyword arguments. These
+    keyword arguments may indicate insecure signing is being performed.
+    """
+    off_by_default = False
+
+    _code = 'DUO137'
+    _error_tmpl = 'DUO137 insecure "itsdangerous" use allowing empty signing'
+
+    @property
+    def kwargs(self):
+        def none_algorithm_predicate(call, kwarg_name):
+            return tree.kwarg_any([
+                functools.partial(
+                    tree.kwarg_module_path_call,
+                    call,
+                    kwarg_name,
+                    "itsdangerous.signer.NoneAlgorithm",
+                    self.namespace
+                ),
+                functools.partial(
+                    tree.kwarg_module_path_call,
+                    call,
+                    kwarg_name,
+                    "itsdangerous.NoneAlgorithm",
+                    self.namespace
+                ),
+            ])
+
+        def none_string_predicate(call, kwarg_name):
+            return functools.partial(
+                tree.kwarg_str,
+                call,
+                kwarg_name,
+                "none"
+            ),
+
+        return [
+            {
+                "module_path": "itsdangerous.signer.Signer",
+                "kwarg_name": "algorithm",
+                "predicate": none_algorithm_predicate,
+            },
+            {
+                "module_path": "itsdangerous.Signer",
+                "kwarg_name": "algorithm",
+                "predicate": none_algorithm_predicate,
+            },
+            {
+                "module_path": "itsdangerous.timed.TimestampSigner",
+                "kwarg_name": "algorithm",
+                "predicate": none_algorithm_predicate,
+            },
+            {
+                "module_path": "itsdangerous.TimestampSigner",
+                "kwarg_name": "algorithm",
+                "predicate": none_algorithm_predicate,
+            },
+            {
+                "module_path": "itsdangerous.jws.JSONWebSignatureSerializer",
+                "kwarg_name": "algorithm_name",
+                "predicate": none_string_predicate,
+            },
+            {
+                "module_path": "itsdangerous.JSONWebSignatureSerializer",
+                "kwarg_name": "algorithm_name",
+                "predicate": none_string_predicate,
+            },
+        ]

dlint/tree.py

@@ -165,6 +165,19 @@ def kwarg_module_path(call, kwarg_name, illegal_module_path, namespace):
     )
 
 
+def kwarg_module_path_call(call, kwarg_name, illegal_module_path, namespace):
+    return any(
+        keyword.arg == kwarg_name
+        and isinstance(keyword.value, ast.Call)
+        and isinstance(keyword.value.func, (ast.Attribute, ast.Name))
+        and namespace.illegal_module_imported(
+            module_path_str(keyword.value.func),
+            illegal_module_path
+        )
+        for keyword in call.keywords
+    )
+
+
 def kwarg_any(kwarg_functions):
     """Resolve kwarg predicates with short-circuit evaluation. This optimization
     technique means we do not have to evaluate every predicate if one is already

docs/README.md

@@ -40,6 +40,7 @@ Dlint uses a simple, folder-based hierarchy written in [Markdown](https://en.wik
 * [`DUO134` `BadCryptographyModuleAttributeUseLinter` insecure "cryptography" attribute use](https://github.com/duo-labs/dlint/blob/master/docs/linters/DUO134.md)
 * [`DUO135` `BadDefusedxmlUseLinter` enable all "forbid_*" defenses when using "defusedxml" parsing](https://github.com/duo-labs/dlint/blob/master/docs/linters/DUO135.md)
 * [`DUO136` `BadXmlsecModuleAttributeUseLinter` insecure "xmlsec" attribute use](https://github.com/duo-labs/dlint/blob/master/docs/linters/DUO136.md)
+* [`DUO137` `BadItsDangerousKwargUseLinter` insecure "itsdangerous" use allowing empty signing](https://github.com/duo-labs/dlint/blob/master/docs/linters/DUO137.md)
 
 # FAQs
 

docs/linters/DUO137.md

@@ -0,0 +1,49 @@
+# DUO137
+
+This linter searches for insecure keyword argument use in the `itsdangerous`
+library. Specifically, it looks for signing operations using the none algorithm
+which results in empty signatures.
+
+## Problematic code
+
+```python
+>>> import itsdangerous
+>>> s1 = itsdangerous.signer.Signer("key1", algorithm=itsdangerous.signer.NoneAlgorithm())
+>>> s2 = itsdangerous.signer.Signer("key2", algorithm=itsdangerous.signer.NoneAlgorithm())
+>>> signature = s1.sign("foo")
+>>> s2.unsign(signature)
+foo
+```
+
+The following usages of the none algorithm are insecure:
+
+```python
+itsdangerous.signer.Signer("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+itsdangerous.signer.Signer("key", algorithm=itsdangerous.NoneAlgorithm())
+itsdangerous.Signer("key", algorithm=itsdangerous.NoneAlgorithm())
+itsdangerous.Signer("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+
+itsdangerous.timed.TimestampSigner("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+itsdangerous.timed.TimestampSigner("key", algorithm=itsdangerous.NoneAlgorithm())
+itsdangerous.TimestampSigner("key", algorithm=itsdangerous.NoneAlgorithm())
+itsdangerous.TimestampSigner("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+
+itsdangerous.jws.JSONWebSignatureSerializer("key", algorithm_name="none")
+itsdangerous.JSONWebSignatureSerializer("key", algorithm_name="none")
+```
+
+## Correct code
+
+Simply not specifying `algorithm|algorithm_name` will default to secure
+behavior. Further, setting `HMACAlgorithm` will ensure verification is
+performed.
+
+## Rationale
+
+Setting the algorithm to none turns off signature verification. This breaks
+HMAC security. For more information see
+[Critical vulnerabilities in JSON Web Token libraries](https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/).
+
+## Exceptions
+
+None

tests/test_bad_itsdangerous_kwarg_use.py

@@ -0,0 +1,98 @@
+#!/usr/bin/env python
+
+from __future__ import (
+    absolute_import,
+    division,
+    print_function,
+    unicode_literals,
+)
+
+import unittest
+
+import dlint
+
+
+class TestBadItsDangerousKwargUse(dlint.test.base.BaseTest):
+
+    def test_bad_itsdangerous_kwarg_usage(self):
+        python_node = self.get_ast_node(
+            """
+            import itsdangerous
+
+            itsdangerous.signer.Signer("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+            itsdangerous.signer.Signer("key", algorithm=itsdangerous.NoneAlgorithm())
+            itsdangerous.Signer("key", algorithm=itsdangerous.NoneAlgorithm())
+            itsdangerous.Signer("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+
+            itsdangerous.timed.TimestampSigner("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+            itsdangerous.timed.TimestampSigner("key", algorithm=itsdangerous.NoneAlgorithm())
+            itsdangerous.TimestampSigner("key", algorithm=itsdangerous.NoneAlgorithm())
+            itsdangerous.TimestampSigner("key", algorithm=itsdangerous.signer.NoneAlgorithm())
+
+            itsdangerous.jws.JSONWebSignatureSerializer("key", algorithm_name="none")
+            itsdangerous.JSONWebSignatureSerializer("key", algorithm_name="none")
+            """
+        )
+
+        linter = dlint.linters.BadItsDangerousKwargUseLinter()
+        linter.visit(python_node)
+
+        result = linter.get_results()
+        expected = [
+            dlint.linters.base.Flake8Result(
+                lineno=4,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=5,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=6,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=7,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=9,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=10,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=11,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=12,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=14,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+            dlint.linters.base.Flake8Result(
+                lineno=15,
+                col_offset=0,
+                message=dlint.linters.BadItsDangerousKwargUseLinter._error_tmpl
+            ),
+        ]
+
+        assert result == expected
+
+
+if __name__ == "__main__":
+    unittest.main()