Un-sanitized eval may have security impact

[Travis McPeak]

On this line: https://github.com/openstack/ceilometer/blob/master/ceilometer/transformer/conversions.py#L62 eval is used for some transformation. The comments near by suggest that it is used for a 'multiplicative factor or else a string to be eval'd'.

If an attacker is able to provide an input like "__import__('os').system('rm -rf /etc')" the process will delete the etc directory with the privileges of the user that is running Ceilometer.

Eval input should always be sanitized. I was unable to find any places that this is actually used, but this should definitely be hardened.

[Thierry Carrez (ttx)]

The question is... is this a user-controlled string ? If not, we can fix it without issuing a CVE.

[Travis McPeak (travis-mcpeak)]

This is a utility library, so whether it is user-controlled input or not really depends on how it is being used. I was unable to find any actual usages of this during my review. Maybe somebody more familiar can help shed some light on this.

[Eoghan Glynn (eglynn)]

The string concerned is read from a ceilometer config file /etc/ceilometer/pipeline.yaml, that should be protected from an attacker by host-level file permissions.

For example, here it's used to provide the logic to scale the delta in cumulative CPU time in nanos for multiple vcpus to a single utilization percentage:

  https://github.com/openstack/ceilometer/blob/master/etc/ceilometer/pipeline.yaml#L46

The string to be eval'd is not submitted via the ceilometer API, so is not part of the "attack surface" as I would understand it.

[Travis McPeak (travis-mcpeak)]

There are other ways for a string to enter the system, it could be inferred from something that is user controlled, or it could come from config files, for example.

[Nejc Saje (nejc-saje)]

It does come from a config file, as Eoghan mentioned, and from config file only. Can you provide more specific examples of what you mean by inferred from something that is user controlled?

[Jeremy Stanley (fungi)]

Sounds like this is not a security vulnerability if the value is only taken from configuration files. As such the VMT would not issue an advisory for it.

[Travis McPeak (travis-mcpeak)]

An example of an inferred value would be the case where we have something like the following:

value_from_api = "40MB"
trans_val = value_from_api[-2:]

In this case, the trans_val is being inferred or calculated from the last two characters of user controlled input. I'm not saying this is the case, it's just an example.

[Brant Knudson (blk-u)]

The eval() call could be made safer by setting globals and locals to only those symbols that are useful in the context it's used. https://docs.python.org/2/library/functions.html#eval and http://lybniz2.sourceforge.net/safeeval.html

[Brant Knudson (blk-u)]

 >>> eval('abc + ghi * 100.0', {'__builtins__': None}, {'abc': 10, 'ghi': 20})
 2010.0
 >>> eval('__import__("os")', {'__builtins__': None}, {'abc': 10, 'ghi': 20})
 NameError: name '__import__' is not defined

[Brant Knudson (blk-u)]

The place that's mentioned here is

 return ((eval(scale, {}, ns) if isinstance(scale, six.string_types) ...

but since the globals doesn't mask __builtins__ you can do something like '__import__("os")' and then do whatever.

I'll propose a change to mask __builtins__ as a security hardening.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/123237

[Nejc Saje (nejc-saje)]

Even setting __builtins__ to None is possible to circumvent: http://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html .

I personally wouldn't mind including your change since it introduces more 'hygenic' use of eval, but as was said before, the input to the eval is protected by filesystem permissions anyway.

[OpenStack Infra (hudson-openstack)]

Change abandoned by Brant Knudson (<email address hidden>) on branch: master
Review: https://review.openstack.org/123237
Reason: This is completely useless.

[gordon chung (chungg)]

as mentioned by Nejc, this file is protected by filesystem permissions.