fix: mitigate code injection related in #672 #681

juarezr · 2025-05-02T21:57:47Z

This PR has the objective of .

Changes

mitigate code injection related in Code Injection via eval() in util.base.expr #672

Checklist

Use this checklist to ensure the quality of pull requests that include new code and/or make changes to existing code.

coveralls · 2025-05-02T22:01:12Z

Pull Request Test Coverage Report for Build 16203809193

Details

97 of 136 (71.32%) changed or added relevant lines in 15 files are covered.
9 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-0.2%) to 90.924%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
petl/test/io/test_db_create.py	2	3	66.67%
petl/test/io/test_db_inference.py	14	16	87.5%
petl/test/util/test_base.py	26	41	63.41%
petl/util/base.py	28	49	57.14%

Files with Coverage Reduction	New Missed Lines	%
petl/io/remotes.py	1	84.95%
petl/test/io/test_db_create.py	1	93.23%
petl/test/io/test_numpy.py	1	98.43%
petl/test/io/test_pandas.py	1	93.33%
petl/test/io/test_pytables.py	1	98.15%
petl/test/io/test_whoosh.py	1	98.15%
petl/test/transform/test_intervals.py	1	97.73%
petl/test/io/test_remotes.py	2	88.89%

Totals
Change from base Build 15981322831:	-0.2%
Covered Lines:	13464
Relevant Lines:	14808

💛 - Coveralls

l3str4nge · 2025-05-20T12:07:43Z

 petl/util/__init__.py:4: in <module>
    from petl.util.base import Table, Record, values, header, data, \
E     File "/home/runner/work/petl/petl/petl/util/base.py", line 686
E       raise ValueError('Invalid expression: %s' % strexpr) from ve
E                                                               ^
E   SyntaxError: invalid syntax

Tests are failing 👀 however they are on the right way I guess 😄

l3str4nge · 2025-05-20T12:09:43Z

petl/util/base.py

-    return eval("lambda rec: " + prog.sub(repl, s))
+    strexpr = "lambda rec: " + prog.sub(repl, expression_text)
+    try:
+        fun = eval(strexpr, { '__builtins__': None }, { '__builtins__': None })


Imo it's still vulnerable, example:

"{__class__.__mro__[1].__subclasses__()[59]('/etc/passwd').read()}"

Maybe you can use https://lmfit.github.io/asteval/ ?

Imo it's still vulnerable, example:

"{__class__.__mro__[1].__subclasses__()[59]('/etc/passwd').read()}"

@l3str4nge,

You are correct.
I think this code wasn't meant to be used in unsafe environments.

Anyway, we have some possible approaches:

Add code like the above to mitigate somewhat the issue for current users.

Implement a second conditional version that uses asteval if available.

Deprecate the current mitigated code/approach and remove it in milestone v2.0 as it's planned for breaking changes.

Patches welcome. :)

petl/test/util/test_base.py

+        res = fu(2)
+        if res:
+            print(res)
+    assert exc_info is not None


petl/util/base.py

 from petl.comparison import comparable_itemgetter
+from petl.compat import (


petl/util/base.py

+        fun = eval(strexpr, restricted, restricted)
+        return fun
+    except ValueError as ve:
+        raise ValueError('Invalid expression: "%s" causes error: %s' % (strexpr, ve))


petl/util/base.py

+        fun = eval(strexpr, restricted, restricted)
+        return fun
+    except ValueError as ve:
+        raise ValueError('Invalid expression: "%s" causes error: %s' % (strexpr, ve))


petl/test/util/test_base.py

petl/util/base.py

petl/test/util/test_base.py

@@ -330,3 +330,18 @@
    table = []
    with pytest.raises(FieldSelectionError):
        rowgroupby(table, 'foo')
+
+
+def test_expr_ok():


petl/test/util/test_base.py

+    assert res == 2
+
+
+def test_expr_inject():


petl/test/util/test_base.py

petl/util/base.py

@@ -1,19 +1,38 @@
-from __future__ import absolute_import, print_function, division
+from __future__ import absolute_import, division, print_function


petl/util/base.py

+        }
+        fun = eval(strexpr, restricted, restricted)
+        return fun
+    except ValueError as ve:


petl/util/base.py

+
+    strexpr = "lambda rec: " + prog.sub(repl, expression_text)
+    try:
+        fun = eval(strexpr, _RESTRICTED, _RESTRICTED)


petl/util/base.py

+
+    strexpr = "lambda rec: " + prog.sub(repl, expression_text)
+    try:
+        fun = eval(strexpr, _RESTRICTED, _RESTRICTED)


petl/util/base.py

@@ -678,7 +700,32 @@
    def repl(matchobj):
        return "rec['%s']" % matchobj.group(1)

-    return eval("lambda rec: " + prog.sub(repl, s))
+    global _RESTRICTED


petl/util/base.py

+
+    strexpr = "lambda rec: " + prog.sub(repl, expression_text)
+    try:
+        fun = eval(strexpr, _RESTRICTED, _RESTRICTED)


petl/test/util/test_base.py

+    fu = expr("{foo2} * {bar2}", trusted=True)
+    with pytest.raises(KeyError) as exc_info:
+        fu({'foo': 3, 'bar': 2})
+        assert exc_info is not None


petl/test/util/test_base.py

+        # trick: using trusted=None will allow to skip using asteval
+        fu = expr("__import__('os').system('ls')", trusted=None)
+        fu({'foo': 3, 'bar': 2})
+        assert exc_info is not None


petl/test/util/test_base.py

+        fu = expr("{foo2} * {bar2}", trusted=True)
+        with pytest.raises(KeyError) as exc_info:
+            fu({'foo': 3, 'bar': 2})
+            assert exc_info is not None


petl/test/util/test_base.py

+def _has_asteval():
+    if PY3:
+        try:
+            import asteval


petl/util/base.py

github-advanced-security

Pylint (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Pylintpython3 (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

petl/test/util/test_base.py

@@ -4,9 +4,9 @@

 from petl.errors import FieldSelectionError
 from petl.test.helpers import ieq, eq_
-from petl.compat import next
+from petl.compat import PY3, next


petl/test/util/test_base.py

+def _has_asteval():
+    if PY3:
+        try:
+            import asteval


petl/test/util/test_base.py

+def _has_asteval():
+    if PY3:
+        try:
+            import asteval


petl/transform/maps.py



-def iterfieldmap(source, mappings, failonerror, errorvalue):
+def iterfieldmap(source, mappings, failonerror, errorvalue, trusted):


petl/transform/maps.py



-def iterfieldmap(source, mappings, failonerror, errorvalue):
+def iterfieldmap(source, mappings, failonerror, errorvalue, trusted):


petl/util/base.py

+            self.aeval.symtable['rec'] = rec
+            evaluated = self.aeval("expr(rec)")
+            if len(self.aeval.error) > 0:
+                err = [ e.get_error()[-1] for e in self.aeval.error ]


petl/util/base.py

+            evaluated = self.aeval("expr(rec)")
+            if len(self.aeval.error) > 0:
+                err = [ e.get_error()[-1] for e in self.aeval.error ]
+                msg = "\n".join(err)


petl/util/base.py

+            if len(self.aeval.error) > 0:
+                err = [ e.get_error()[-1] for e in self.aeval.error ]
+                msg = "\n".join(err)
+                raise ValueError("Failed to evaluate expression due to: %s" % msg)


petl/util/base.py

+            if len(self.aeval.error) > 0:
+                err = [ e.get_error()[-1] for e in self.aeval.error ]
+                msg = "\n".join(err)
+                raise ValueError("Failed to evaluate expression due to: %s" % msg)


petl/util/base.py

+                err = [ e.get_error()[-1] for e in self.aeval.error ]
+                msg = "\n".join(err)
+                raise ValueError("Failed to evaluate expression due to: %s" % msg)
+            return evaluated


…ih asteval

… convert

petl/test/io/test_db_inference.py

+def test_json_inference():
+    data = [{'a': 1}, {'b': 2}, None]
+    col = make_sqlalchemy_column(data, 'payload')
+    assert col.name == 'payload'


petl/test/io/test_db_inference.py

+    data = [{'a': 1}, {'b': 2}, None]
+    col = make_sqlalchemy_column(data, 'payload')
+    assert col.name == 'payload'
+    assert isinstance(col.type, JSON)


petl/test/io/test_db_inference.py

+
+def test_int_inference():
+    col = make_sqlalchemy_column([1, 2, 3], 'n')
+    assert col.name == 'n'


petl/test/io/test_db_inference.py

+def test_int_inference():
+    col = make_sqlalchemy_column([1, 2, 3], 'n')
+    assert col.name == 'n'
+    assert isinstance(col.type, Integer)


juarezr added the Bug It must work in all situations, but this failed label May 2, 2025

juarezr self-assigned this May 2, 2025

juarezr linked an issue May 2, 2025 that may be closed by this pull request

Code Injection via eval() in util.base.expr #672

Closed

1 task

juarezr mentioned this pull request May 2, 2025

Code Injection via eval() in util.base.expr #672

Closed

1 task

l3str4nge suggested changes May 20, 2025

View reviewed changes