Skip to content

Commit

Permalink
safe_eval()
Browse files Browse the repository at this point in the history
  • Loading branch information
rhaschke committed Sep 3, 2021
1 parent 5c12898 commit 9941961
Showing 1 changed file with 16 additions and 5 deletions.
21 changes: 16 additions & 5 deletions src/xacro/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,10 @@ def __getattr__(self, item):

def construct_angle_radians(loader, node):
"""utility function to construct radian values from yaml"""
value = loader.construct_scalar(node).strip()
value = loader.construct_scalar(node)
try:
return float(eval(value, global_symbols, global_symbols))
except SyntaxError as e:
return float(safe_eval(value, global_symbols))
except SyntaxError:
raise XacroException("invalid expression: %s" % value)


Expand Down Expand Up @@ -156,13 +156,24 @@ def load_yaml(filename):
# taking simple security measures to forbid access to __builtins__
# only the very few symbols explicitly listed are allowed
# for discussion, see: http://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html
global_symbols = {'__builtins__': {k: __builtins__[k] for k in ['list', 'dict', 'map', 'len', 'str', 'float', 'int', 'True', 'False', 'min', 'max', 'round']}}
global_symbols = {k: __builtins__[k] for k in
['list', 'dict', 'map', 'len', 'str', 'float', 'int',
'True', 'False', 'min', 'max', 'round']}
# also define all math symbols and functions
global_symbols.update(math.__dict__)
# expose load_yaml, abs_filename, and dotify
global_symbols.update(dict(load_yaml=load_yaml, abs_filename=abs_filename_spec, dotify=YamlDictWrapper))


def safe_eval(expr, globals, locals=None):
code = compile(expr.strip(), "<expression>", "eval")
invalid_names = [n for n in code.co_names if n.startswith("__")]
if invalid_names:
raise XacroException("Use of invalid name(s): ", ', '.join(invalid_names))
globals.update(__builtins__= {}) # disable default builtins
return eval(code, globals, locals)


class XacroException(Exception):
"""
XacroException allows to wrap another exception (exc) and to augment
Expand Down Expand Up @@ -683,7 +694,7 @@ def grab_properties(elt, table):
def eval_text(text, symbols):
def handle_expr(s):
try:
return eval(eval_text(s, symbols), global_symbols, symbols)
return safe_eval(eval_text(s, symbols), global_symbols, symbols)
except Exception as e:
# re-raise as XacroException to add more context
raise XacroException(exc=e,
Expand Down

7 comments on commit 9941961

@gavanderhoorn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhaschke: was this commit part of a PR? I'm having trouble finding it.

@rhaschke
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I pushed the security fix directly - without a PR. Do you face issues with this security feature?

@gavanderhoorn
Copy link
Contributor

@gavanderhoorn gavanderhoorn commented on 9941961 Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to have broken some things. UniversalRobots/Universal_Robots_ROS_Driver#463 fi.

Not to say double underscores are a good idea, but it took my some time to find what's causing it.

@gavanderhoorn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog is also not too specific about it (1.14.9 (2021-09-03)).

Would it be part of:

Fix eval security vulnerability - safe_eval() - unit tests validating the protection mechanism

?

@rhaschke
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to have broken some things. UniversalRobots/Universal_Robots_ROS_Driver#463 fi.

Sorry about breaking this, but it was unsafe to allow arbitrary names in eval().
I should have better announced this change, though. I now improved the CHANGELOG.

@gavanderhoorn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand.

Thanks for updating the changelog.

@fmauch
Copy link

@fmauch fmauch commented on 9941961 Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the xacro files accordingly. Thanks @gavanderhoorn for linking this so quickly!

Please sign in to comment.