eval being used in session.py

Bug #1414529 reported by gvb on 2015-01-26
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Low
Unassigned
OpenStack Security Advisory
Undecided
Unassigned

Bug Description

There's a FIXME comment saying eval is evil (which it is obviously) in
the file below:

/opt/stack/nova/nova/virt/xenapi/client/session.py

    def _unwrap_plugin_exceptions(self, func, *args, **kwargs):
        """Parse exception details."""
        try:
            return func(*args, **kwargs)
        except self.XenAPI.Failure as exc:
            LOG.debug("Got exception: %s", exc)
            if (len(exc.details) == 4 and
                exc.details[0] == 'XENAPI_PLUGIN_EXCEPTION' and
                    exc.details[2] == 'Failure'):
                params = None
                try:
                    # FIXME(comstud): eval is evil.
                    params = eval(exc.details[3]) <--- here
                except Exception:
                    raise exc
                raise self.XenAPI.Failure(params)
            else:
                raise
        except xmlrpclib.ProtocolError as exc:
            LOG.debug("Got exception: %s", exc)
            raise

This should indeed be fixed as it looks that an arbitrary plugin can now
potentially achieve arbitrary code execution by throwing specifically
prepared XenAPI Failure Exceptions.

This code has been here from the beginning according to the git logs at
http://git.openstack.org/cgit/openstack/nova/log/nova/virt/xenapi/client/session.py
so the FIXME coomment has existed since at least 2013-11-22.

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

Changed in ossa:
status: New → Incomplete

From a security point of view, this bug report does not seems to describe a vulnerability: an arbitrary plugin goal is to achieve arbitrary code execution... and in order to reach this eval, the plugin must be installed first.

However this code might deserve some strengthening... I suggest to remove the OSSA task and open this report.

Let me know if that works for you.

gvb (gvb-2) wrote :

Works for me.

information type: Private Security → Public
Changed in ossa:
status: Incomplete → Won't Fix
Joe Gordon (jogo) on 2015-01-28
Changed in nova:
status: New → Confirmed
importance: Undecided → Low
Changed in nova:
assignee: nobody → Abhilash Goyal (abhilash-goyal)

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

Changed in nova:
status: Confirmed → In Progress

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

Change abandoned by Abhilash Goyal (<email address hidden>) on branch: master
Review: https://review.openstack.org/276048
Reason: other patch available.

Changed in nova:
assignee: Abhilash Goyal (abhilash-goyal) → nobody
status: In Progress → Confirmed
Changed in nova:
assignee: nobody → Tanvir Talukder (tanvirt16)
Changed in nova:
assignee: Tanvir Talukder (tanvirt16) → nobody
Michael Glaser (mikeg451) wrote :

Abhilash,
Your solution looks good and is still an active commit, so why did you set the assignee to nobody?

Change https://review.openstack.org/#/c/275513/ is fixing this and looks good.

Changed in nova:
status: Confirmed → In Progress
assignee: nobody → Abhilash Goyal (abhilash-goyal)

Reviewed: https://review.openstack.org/275513
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=9955d1685d3a5a4357f405b442abaa43f60cd6d0
Submitter: Jenkins
Branch: master

commit 9955d1685d3a5a4357f405b442abaa43f60cd6d0
Author: abhilash-goyal <email address hidden>
Date: Tue Feb 9 16:25:13 2016 +0530

    Replace use of eval with ast.literal_eval

    literal_eval supports a limited subset of Python, and is therefore
    safer than eval.

    Close-bug: 1414529
    Change-Id: Ib6145408360fa57cccc8d77c590a203e5088b193

Sean Dague (sdague) wrote :

There are no currently open reviews on this bug, changing
the status back to the previous state and unassigning. If
there are active reviews related to this bug, please include
links in comments.

Changed in nova:
status: In Progress → Confirmed
assignee: Abhilash Goyal (abhilash-goyal) → nobody
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers