From 07a70af3448c76e1ae31a886ac8ec13eafa26e6a Mon Sep 17 00:00:00 2001 From: Taus Date: Tue, 11 May 2021 19:41:39 +0000 Subject: [PATCH 1/9] Python: Limit set of globals that may be built-ins I am very tempted to leave out the constants, or at the very least `False`, `True`, and `None`, as these have _many_ occurrences in the average codebase, and are not terribly useful at the API-graph level. If we really do want to capture "nodes that refer to such and such constant", then I think a better solution would be to create classes extending `DataFlow::Node` to facilitate this. --- python/ql/src/semmle/python/ApiGraphs.qll | 70 ++++++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/python/ql/src/semmle/python/ApiGraphs.qll b/python/ql/src/semmle/python/ApiGraphs.qll index a530e44e85da..86ad81e7454a 100644 --- a/python/ql/src/semmle/python/ApiGraphs.qll +++ b/python/ql/src/semmle/python/ApiGraphs.qll @@ -351,20 +351,74 @@ module API { private import semmle.python.types.Builtins as Builtins + /** Returns the names of known built-ins. */ + private string builtin_name() { + // Built-in functions shared between Python 2.7.6 and 3.9.5 + result in [ + "abs", "all", "any", "bin", "bool", "bytearray", "callable", "chr", "classmethod", + "compile", "complex", "delattr", "dict", "dir", "divmod", "enumerate", "eval", "filter", + "float", "format", "frozenset", "getattr", "globals", "hasattr", "hash", "help", "hex", + "id", "input", "int", "isinstance", "issubclass", "iter", "len", "list", "locals", "map", + "max", "memoryview", "min", "next", "object", "oct", "open", "ord", "pow", "print", + "property", "range", "repr", "reversed", "round", "set", "setattr", "slice", "sorted", + "staticmethod", "str", "sum", "super", "tuple", "type", "vars", "zip", "__import__" + ] + or + // Built-in constants shared between Python 2.7.6 and 3.9.5 + result in ["False", "True", "None", "NotImplemented", "Ellipsis", "__debug__"] + or + // Python 3.9.5 only + major_version() = 3 and + result in ["ascii", "breakpoint", "bytes", "exec"] + or + // Python 2.7.6 only + major_version() = 2 and + result in [ + "basestring", "cmp", "execfile", "file", "long", "raw_input", "reduce", "reload", + "unichr", "unicode", "xrange" + ] + } + /** * Gets a data flow node that is likely to refer to a built-in with the name `name`. * - * Currently this is an over-approximation, and does not account for things like overwriting a + * Currently this is an over-approximation, and may not account for things like overwriting a * built-in with a different value. */ private DataFlow::Node likely_builtin(string name) { - result.asCfgNode() = - any(NameNode n | - n.isGlobal() and - n.isLoad() and - name = n.getId() and - name in [any(Builtins::Builtin b).getName(), "None", "True", "False"] - ) + exists(Module m | + result.asCfgNode() = + any(NameNode n | + possible_builtin_accessed_in_module(n, name, m) and + not possible_builtin_defined_in_module(name, m) + ) + ) + } + + /** + * Holds if a global variable called `name` (which is also the name of a built-in) is assigned + * a value in the module `m`. + */ + private predicate possible_builtin_defined_in_module(string name, Module m) { + exists(NameNode n | + n.isGlobal() and + n.isStore() and + name = n.getId() and + name = builtin_name() and + m = n.getEnclosingModule() + ) + } + + /** + * Holds if `n` is an access of a global variable called `name` (which is also the name of a + * built-in) inside the module `m`. + */ + private predicate possible_builtin_accessed_in_module(NameNode n, string name, Module m) { + n.isGlobal() and + n.isLoad() and + name = n.getId() and + name = builtin_name() and + m = n.getEnclosingModule() } /** From 5c7e73d4854ac817a10d32fb44e6b7272b5f11ef Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 12 May 2021 09:51:55 +0000 Subject: [PATCH 2/9] Python: Add exception types --- python/ql/src/semmle/python/ApiGraphs.qll | 31 ++++++++++++++++++----- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/python/ql/src/semmle/python/ApiGraphs.qll b/python/ql/src/semmle/python/ApiGraphs.qll index 86ad81e7454a..177d0f3998e5 100644 --- a/python/ql/src/semmle/python/ApiGraphs.qll +++ b/python/ql/src/semmle/python/ApiGraphs.qll @@ -353,7 +353,7 @@ module API { /** Returns the names of known built-ins. */ private string builtin_name() { - // Built-in functions shared between Python 2.7.6 and 3.9.5 + // Built-in functions and exceptions shared between Python 2 and 3 result in [ "abs", "all", "any", "bin", "bool", "bytearray", "callable", "chr", "classmethod", "compile", "complex", "delattr", "dict", "dir", "divmod", "enumerate", "eval", "filter", @@ -361,17 +361,36 @@ module API { "id", "input", "int", "isinstance", "issubclass", "iter", "len", "list", "locals", "map", "max", "memoryview", "min", "next", "object", "oct", "open", "ord", "pow", "print", "property", "range", "repr", "reversed", "round", "set", "setattr", "slice", "sorted", - "staticmethod", "str", "sum", "super", "tuple", "type", "vars", "zip", "__import__" + "staticmethod", "str", "sum", "super", "tuple", "type", "vars", "zip", "__import__", + // Exceptions + "ArithmeticError", "AssertionError", "AttributeError", "BaseException", "BufferError", + "BytesWarning", "DeprecationWarning", "EOFError", "EnvironmentError", "Exception", + "FloatingPointError", "FutureWarning", "GeneratorExit", "IOError", "ImportError", + "ImportWarning", "IndentationError", "IndexError", "KeyError", "KeyboardInterrupt", + "LookupError", "MemoryError", "NameError", "NotImplemented", "NotImplementedError", + "OSError", "OverflowError", "PendingDeprecationWarning", "ReferenceError", "RuntimeError", + "RuntimeWarning", "StandardError", "StopIteration", "SyntaxError", "SyntaxWarning", + "SystemError", "SystemExit", "TabError", "TypeError", "UnboundLocalError", + "UnicodeDecodeError", "UnicodeEncodeError", "UnicodeError", "UnicodeTranslateError", + "UnicodeWarning", "UserWarning", "ValueError", "Warning", "ZeroDivisionError" ] or - // Built-in constants shared between Python 2.7.6 and 3.9.5 + // Built-in constants shared between Python 2 and 3 result in ["False", "True", "None", "NotImplemented", "Ellipsis", "__debug__"] or - // Python 3.9.5 only + // Python 3 only major_version() = 3 and - result in ["ascii", "breakpoint", "bytes", "exec"] + result in [ + "ascii", "breakpoint", "bytes", "exec", + // Exceptions + "BlockingIOError", "BrokenPipeError", "ChildProcessError", "ConnectionAbortedError", + "ConnectionError", "ConnectionRefusedError", "ConnectionResetError", "FileExistsError", + "FileNotFoundError", "InterruptedError", "IsADirectoryError", "ModuleNotFoundError", + "NotADirectoryError", "PermissionError", "ProcessLookupError", "RecursionError", + "ResourceWarning", "StopAsyncIteration", "TimeoutError" + ] or - // Python 2.7.6 only + // Python 2 only major_version() = 2 and result in [ "basestring", "cmp", "execfile", "file", "long", "raw_input", "reduce", "reload", From 3d30efed112473c87e213f0e7feed3b0196fc8de Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 12 May 2021 11:07:16 +0000 Subject: [PATCH 3/9] Python: Add `exec` as a shared built-in This is _slightly_ wrong, since `exec` isn't a built-in function in Python 2. It should be harmless, however, since `exec` is a keyword, and so cannot be redefined anyway. --- python/ql/src/semmle/python/ApiGraphs.qll | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/ql/src/semmle/python/ApiGraphs.qll b/python/ql/src/semmle/python/ApiGraphs.qll index 177d0f3998e5..9ad125f86bc6 100644 --- a/python/ql/src/semmle/python/ApiGraphs.qll +++ b/python/ql/src/semmle/python/ApiGraphs.qll @@ -372,7 +372,9 @@ module API { "RuntimeWarning", "StandardError", "StopIteration", "SyntaxError", "SyntaxWarning", "SystemError", "SystemExit", "TabError", "TypeError", "UnboundLocalError", "UnicodeDecodeError", "UnicodeEncodeError", "UnicodeError", "UnicodeTranslateError", - "UnicodeWarning", "UserWarning", "ValueError", "Warning", "ZeroDivisionError" + "UnicodeWarning", "UserWarning", "ValueError", "Warning", "ZeroDivisionError", + // Added for compatibility + "exec" ] or // Built-in constants shared between Python 2 and 3 From ff2b6b9737b608c1d99752ba4a2df521863eb5ef Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 12 May 2021 18:07:18 +0000 Subject: [PATCH 4/9] Python: Correctly locate stores to built-ins --- python/ql/src/semmle/python/ApiGraphs.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/semmle/python/ApiGraphs.qll b/python/ql/src/semmle/python/ApiGraphs.qll index 9ad125f86bc6..080c967ed2d1 100644 --- a/python/ql/src/semmle/python/ApiGraphs.qll +++ b/python/ql/src/semmle/python/ApiGraphs.qll @@ -422,7 +422,7 @@ module API { */ private predicate possible_builtin_defined_in_module(string name, Module m) { exists(NameNode n | - n.isGlobal() and + not exists(LocalVariable v | n.defines(v)) and n.isStore() and name = n.getId() and name = builtin_name() and From fe12e620dde570aba2fea1746b460e61707c040d Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 12 May 2021 18:37:42 +0000 Subject: [PATCH 5/9] Python: Avoid clobbering `range` in test This was an unwanted interaction between two unrelated tests, so I switched to a different built-in in the second test. I also added a test case that shows an unfortunate side effect of this more restricted handling of built-ins. --- .../experimental/dataflow/ApiGraphs/test.py | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/python/ql/test/experimental/dataflow/ApiGraphs/test.py b/python/ql/test/experimental/dataflow/ApiGraphs/test.py index 5a382f3d6cc9..f4988e41a0f9 100644 --- a/python/ql/test/experimental/dataflow/ApiGraphs/test.py +++ b/python/ql/test/experimental/dataflow/ApiGraphs/test.py @@ -122,14 +122,18 @@ def my_print(x): print = my_print print("these words") -def local_redefine_range(): - range = 5 - return range - -def global_redefine_range(): - global range - range = 6 - return range #$ SPURIOUS: use=moduleImport("builtins").getMember("range") +def local_redefine_chr(): + chr = 5 + return chr + +def global_redefine_chr(): + global chr + chr = 6 + return chr + +def what_is_chr_now(): + # If global_redefine_chr has been run, then the following is _not_ a reference to the built-in chr + return chr(123) #$ MISSING: use=moduleImport("builtins").getMember("chr").getReturn() def obscured_print(): p = print #$ use=moduleImport("builtins").getMember("print") From fad55b3635994ab25925548b62939e355d6eb474 Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 12 May 2021 21:09:51 +0000 Subject: [PATCH 6/9] Python: Reimplement `py/use-of-input` --- python/ql/src/Expressions/UseofInput.ql | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/python/ql/src/Expressions/UseofInput.ql b/python/ql/src/Expressions/UseofInput.ql index 2b11eecfa2b1..db905818276f 100644 --- a/python/ql/src/Expressions/UseofInput.ql +++ b/python/ql/src/Expressions/UseofInput.ql @@ -11,11 +11,11 @@ */ import python +import semmle.python.dataflow.new.DataFlow +import semmle.python.ApiGraphs -from CallNode call, Context context, ControlFlowNode func +from DataFlow::CallCfgNode call where - context.getAVersion().includes(2, _) and - call.getFunction() = func and - func.pointsTo(context, Value::named("input"), _) and - not func.pointsTo(context, Value::named("raw_input"), _) + call = API::builtin("input").getACall() and + call != API::builtin("raw_input").getACall() select call, "The unsafe built-in function 'input' is used in Python 2." From 79cfe5aca2b19193a1adce28e0cead35fad996a0 Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 12 May 2021 21:23:16 +0000 Subject: [PATCH 7/9] Python: Limit `py/use-of-input` to Python 2 --- python/ql/src/Expressions/UseofInput.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/python/ql/src/Expressions/UseofInput.ql b/python/ql/src/Expressions/UseofInput.ql index db905818276f..bf2a71de22e6 100644 --- a/python/ql/src/Expressions/UseofInput.ql +++ b/python/ql/src/Expressions/UseofInput.ql @@ -16,6 +16,7 @@ import semmle.python.ApiGraphs from DataFlow::CallCfgNode call where + major_version() = 2 and call = API::builtin("input").getACall() and call != API::builtin("raw_input").getACall() select call, "The unsafe built-in function 'input' is used in Python 2." From 75a43e76e8873e6719c293a44007f20fcc214c69 Mon Sep 17 00:00:00 2001 From: Taus Date: Wed, 19 May 2021 11:54:47 +0000 Subject: [PATCH 8/9] Python: Address review comments. - Removes the version check on the set of built-in names. - Renames the predicate used to represent said set. - Documents how these lists of names were obtained. - Gets rid of a superfluous import. --- python/ql/src/semmle/python/ApiGraphs.qll | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/python/ql/src/semmle/python/ApiGraphs.qll b/python/ql/src/semmle/python/ApiGraphs.qll index 080c967ed2d1..ad10f9950ffe 100644 --- a/python/ql/src/semmle/python/ApiGraphs.qll +++ b/python/ql/src/semmle/python/ApiGraphs.qll @@ -349,10 +349,10 @@ module API { ) } - private import semmle.python.types.Builtins as Builtins - - /** Returns the names of known built-ins. */ - private string builtin_name() { + /** Gets the name of a known built-in. */ + private string getBuiltInName() { + // These lists were created by inspecting the `builtins` and `__builtin__` modules in + // Python 2 and 3 respectively, using the `dir` built-in. // Built-in functions and exceptions shared between Python 2 and 3 result in [ "abs", "all", "any", "bin", "bool", "bytearray", "callable", "chr", "classmethod", @@ -381,7 +381,6 @@ module API { result in ["False", "True", "None", "NotImplemented", "Ellipsis", "__debug__"] or // Python 3 only - major_version() = 3 and result in [ "ascii", "breakpoint", "bytes", "exec", // Exceptions @@ -393,7 +392,6 @@ module API { ] or // Python 2 only - major_version() = 2 and result in [ "basestring", "cmp", "execfile", "file", "long", "raw_input", "reduce", "reload", "unichr", "unicode", "xrange" @@ -425,7 +423,7 @@ module API { not exists(LocalVariable v | n.defines(v)) and n.isStore() and name = n.getId() and - name = builtin_name() and + name = getBuiltInName() and m = n.getEnclosingModule() ) } @@ -438,7 +436,7 @@ module API { n.isGlobal() and n.isLoad() and name = n.getId() and - name = builtin_name() and + name = getBuiltInName() and m = n.getEnclosingModule() } From c4bb3c27e0f7c729a397ebab7daa2a1a4451acea Mon Sep 17 00:00:00 2001 From: Taus Date: Thu, 20 May 2021 13:14:09 +0200 Subject: [PATCH 9/9] Python: Update python/ql/src/semmle/python/ApiGraphs.qll Co-authored-by: Rasmus Wriedt Larsen --- python/ql/src/semmle/python/ApiGraphs.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/semmle/python/ApiGraphs.qll b/python/ql/src/semmle/python/ApiGraphs.qll index ad10f9950ffe..61c9f4c1c7f7 100644 --- a/python/ql/src/semmle/python/ApiGraphs.qll +++ b/python/ql/src/semmle/python/ApiGraphs.qll @@ -352,7 +352,7 @@ module API { /** Gets the name of a known built-in. */ private string getBuiltInName() { // These lists were created by inspecting the `builtins` and `__builtin__` modules in - // Python 2 and 3 respectively, using the `dir` built-in. + // Python 3 and 2 respectively, using the `dir` built-in. // Built-in functions and exceptions shared between Python 2 and 3 result in [ "abs", "all", "any", "bin", "bool", "bytearray", "callable", "chr", "classmethod",