eval used without validating input

[Mark McClain]

The db_base_plugin calls eval without validating the input.

Thanks for Maru Newby for reporting this.

[Mark McClain (markmcclain)]

The exploit can be triggered via the API. The following exploit will delete a file on the Quantum Server:

quantum port-list -- --fixed_ips "cfg.os.remove('/tmp/foo')"

[Maru Newby (maru)]

Here is a preliminary patch that cleans up the marshalling for get_ports(). Please let me know what the next steps are - I haven't worked on a security vulnerability before.

[Maru Newby (maru)]

Minor cleanup to 2 of the tests.

[dan wendlandt (danwent)]

I have not worked on a security vulnerability issue before either, so we are all learning. I can loop ttx in.

I think most of the issues around security vulnerabilities would apply if the software had been released and was upstream in a distribution already. In that case, we'd probably want to make sure we work with distros to make sure that the update to the stable branch gets rolled out quickly, and then announce it on the main mailing list in a very public way. I don't believe a milestone release (e.g., F-2, F-3) would count, but ttx is the expert here.

[Mark McClain (markmcclain)]

I'm getting test failures with the 002 patch.

test_list_ports_filtered_by_fixed_ip fails for NEC and Nicira plugins.

Also I'm not sure the logic is equivalent.

Previously, I could send multiple fixed_ip filters and have them OR'ed together. ((ip_address && subnet_id) || ip_address || subnet_id). The new patch seems to apply take the first one.

Open question to everyone....
Ideally the result set filtering should be done in the database, but changes this late concern me because there is an established behavior that we need maintain. I'm not sure the existing test suite has the breadth of tests to ensure this. Should we split the work into a narrow security fix and a make the filtering changes in Grizzly?

Thoughts?

[Salvatore Orlando (salvatore-orlando)]

Foreword: I have not thoroughly looked at Maru's patch. Just that much which is enough to grasp the nature of the problem and the solution.

I wonder if we can shorten the diff of the patch by using ast (abstract syntax tree). This module, which is standard Python library, has safe eval functions, and can easy convert a string into a dict:

>>> import ast
>>> test="{'x':'forza', 'y':'napoli'}"
>>> test_dict=ast.literal_eval(test)
>>> test_dict
{'y': 'napoli', 'x': 'forza'}

[Maru Newby (maru)]

Mark: The way I've written the tests I can add support for OR'ing pretty easily. I apologize for not testing more thoroughly.

Salvatore: I'm afraid the issue is not about direct conversion of a string to a dict - if it were json evaluation would be the obvious choice. Instead, the issue is about marshalling query parameters that are not yet in a form convenient for evaluation.

[Thierry Carrez (ttx)]

If my analysis is correct, this was introduced during the Folsom cycle in commit 681d096ef26247f6f8db8faca1abe9ae6a186562 which was never included in a formal release of Quantum, core or otherwise. It was present in development milestones starting in folsom-2... but those are not "released".

So I'd say it can be fixed directly and should not trigger a formal embargo process or OSSA. We could however mention it on the mailing-list as a heads-up.

Russell, Steve: thoughts ?

[Gary Kotton (garyk)]

On 09/18/2012 12:54 PM, Thierry Carrez wrote:
> If my analysis is correct, this was introduced during the Folsom cycle
> in commit 681d096ef26247f6f8db8faca1abe9ae6a186562 which was never
> included in a formal release of Quantum, core or otherwise. It was
> present in development milestones starting in folsom-2... but those are
> not "released".
>
> So I'd say it can be fixed directly and should not trigger a formal
> embargo process or OSSA. We could however mention it on the mailing-list
> as a heads-up.
>
> Russell, Steve: thoughts ?
>
> ** Tags added: folsom-rc-potential
>
Correct, the code was added in Folsom-2.
Thanks
Gary

[Russell Bryant (russellb)]

Yes, I agree that since this was never in an officially release, embargo and an OSSA is unnecessary. Nice job catching it before release! I also agree that a heads up to the mailing list would be a nice thing to do (but not strictly necessary).

[Thierry Carrez (ttx)]

@Maru: feel free to submit your patch for usual review

[Maru Newby (maru)]

@Thierry: Thanks!

@Mark: I've fixed the test failure - the test for filtering was incorrectly assuming that the locally created port was identical to what would be returned, but the NEC and Nicira plugins were returning slightly different data. The test only has to ensure that the correct port is returned, though, so I've relaxed the assertion accordingly.

As to the filtering functionality supporting arbitrarily complex AND/OR'ing behaviour, the consensus on irc seemed to be that this is not something we need to maintain at this time. The upcoming review will provide for logical OR'ing of multiple values for a key (via in_) and AND'ing across keys.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/13205
Committed: http://github.com/openstack/quantum/commit/a6fbe838954ebc9432da58322ddece3ae47178c0
Submitter: Jenkins
Branch: master

commit a6fbe838954ebc9432da58322ddece3ae47178c0
Author: Maru Newby <email address hidden>
Date: Mon Sep 17 18:46:38 2012 -0700

    Removed eval of unchecked strings.

     * eval() was previously used to marshall unchecked strings as
       filter parameters for QuantumDbPluginV2.get_ports() via
       the --fixed_ips flag.
     * This change removes the use of eval and cleans up the filtering
       implementation for get_ports().
     * The new filtering implementation does not support arbitrary
       OR'ing or AND'ing. Instead, multiple values for a given filter
       key are logically OR'ed, and filters across keys are AND'ed.
     * Example usage - filter for .2 or .3 in the given subnet:

         quantum port-list -- --fixed_ips ip_address=10.0.0.3 \
             ip_address=10.0.0.2 subnet_id=nOtaRealId

     * Addresses bug 1052179

    Change-Id: I451f33ae53e623f86015b3fc2e6a7ca2f51ee836

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: milestone-proposed
Review: https://review.openstack.org/13257

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/13257
Committed: http://github.com/openstack/quantum/commit/eb7f6b7fbe97d4929093894968d39835cfff4254
Submitter: Jenkins
Branch: milestone-proposed

commit eb7f6b7fbe97d4929093894968d39835cfff4254
Author: Salvatore Orlando <email address hidden>
Date: Tue Sep 18 18:27:00 2012 -0700

    Removed eval of unchecked strings.

     * eval() was previously used to marshall unchecked strings as
       filter parameters for QuantumDbPluginV2.get_ports() via
       the --fixed_ips flag.
     * This change removes the use of eval and cleans up the filtering
       implementation for get_ports().
     * The new filtering implementation does not support arbitrary
       OR'ing or AND'ing. Instead, multiple values for a given filter
       key are logically OR'ed, and filters across keys are AND'ed.
     * Example usage - filter for .2 or .3 in the given subnet:

         quantum port-list -- --fixed_ips ip_address=10.0.0.3 \
             ip_address=10.0.0.2 subnet_id=nOtaRealId

     * Addresses bug 1052179

    Change-Id: I451f33ae53e623f86015b3fc2e6a7ca2f51ee836