This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 830245 - (CVE-2012-2684) CVE-2012-2684 cumin: SQL injection flaw
CVE-2012-2684 cumin: SQL injection flaw
Status: NEW
Product: Security Response
Classification: Other
Component: vulnerability (Show other bugs)
unspecified
All Linux
medium Severity medium
: ---
: ---
Assigned To: Red Hat Product Security
impact=moderate,public=20120919,repor...
: Security
Depends On: 840112 858868
Blocks: 808230 828434 828935
  Show dependency treegraph
 
Reported: 2012-06-08 11:56 EDT by Vincent Danen
Modified: 2013-03-13 22:55 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 840112 (view as bug list)
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Patch for the comparison filter (4.97 KB, patch)
2012-06-11 22:20 EDT, Trevor McKay
no flags Details | Diff
Patch for the comparison filter (5.49 KB, patch)
2012-07-30 12:57 EDT, Trevor McKay
no flags Details | Diff

  None (edit)
Description Vincent Danen 2012-06-08 11:56:08 EDT
Florian Weimer reported an SQL injection flaw in Cumin, where the agent variable is not properly encoded.  This could allow a remote user to manipulate the contents of the backend database using a specially-crafted URL.
Comment 2 Trevor McKay 2012-06-08 18:08:06 EDT
Started investigating this.

Bad values for agent or object ids are constructed (indirectly) in the get_sample_filters_by_signature() routine.  This routine is used to pull certain values out of the URL to use as a lookup for an object in question.

The root of the issue is the way that SQL filter text is generated using Python string concatenation.  According to docs associated with psycopg2 (our db adapter), using string concatenation directly leads to SQL injection vulnerabilities due to bad quoting.  psycopg2 provides a mechanism that uses a template and values to create properly formatted/escaped SQL strings.  The authors strongly recommend that Py string concatention never, ever be used.

Developing.  We use the template mechanism in other places, but not everywhere.  This may resolve to scrubbing the rosemary code for any place where concat is used and replacing with the psycopg mechanism.  Will continue investigating with test programs....
Comment 4 Florian Weimer 2012-06-11 04:16:16 EDT
(In reply to comment #2)
> Bad values for agent or object ids are constructed (indirectly) in the
> get_sample_filters_by_signature() routine.  This routine is used to pull
> certain values out of the URL to use as a lookup for an object in question.

Good catch.  So this has:

        col = self.sql_samples_table._qmf_agent_id
        filters.append(SqlComparisonFilter(col, "'%s'" % agent_id))

        col = self.sql_samples_table._qmf_object_id
        filters.append(SqlComparisonFilter(col, "'%s'" % obj_id))

Ideally, this should be

        col = self.sql_samples_table._qmf_agent_id
        filters.append(SqlComparisonFilter(col, agent_id))

        col = self.sql_samples_table._qmf_object_id
        filters.append(SqlComparisonFilter(col, obj_id))

and SqlComparisonFilter would take care of the escaping (or prepare things for use in a parameterized query).  This should fix all users of SqlComparisonFilter (I think), except for ObjectSqlAdapter.add_value_filter(), which would break.

(The code in r5192 is different.)

> The root of the issue is the way that SQL filter text is generated using Python
> string concatenation.  According to docs associated with psycopg2 (our db
> adapter), using string concatenation directly leads to SQL injection
> vulnerabilities due to bad quoting.

Yes, it's hard to avoid SQL injection issues if you rely on plain string concatenation.

> psycopg2 provides a mechanism that uses a
> template and values to create properly formatted/escaped SQL strings.  The
> authors strongly recommend that Py string concatention never, ever be used.

In order to use this mechanism, you'd have to tracker parameter values separately from the SQL string (which would have to be constructed using parameters).  In other words, instead of constructing a string, you construct a pair (sql, params), where params is a tuple containing the parameter values.

For a short term solution, I think it is preferable to add the missing quoting in the correct place.
Comment 5 Trevor McKay 2012-06-11 22:04:26 EDT
Attaching a patch for this issue, tested in a scratch build on el6.

The pyscopg2.cursor.mogrify(text, values) method uses the same mechanism as pyscopg2.cursor.execute(text, values) to do substitution, with proper quoting provided by psycopg2.  The difference is that mogrify just returns the query string (whereas execute builds the string with substitution and then executes it).

For the filter cases, the string concatentation can be replaced with use of cursor.mogrify().

Anything in Cumin that builds query strings with an embedded pattern like "%(Name)s" is already being handled ultimately by cursor.execute(text, values), so those are fine.  This applies to most of the instances of SqlComparisonFilter, as well as it's derived types SqlValueFilter and SqlLikeFilter.

To keep everything compatible, SqlComparisonFilter has been extended with extra arguments that allow it to obtain a cursor object and apply psycopg2 quoting selectively.  This is used in the get_sample_filters_by_signature() call and in one other place, admin.get_roles_for_user(), although in the second it is not strictly necessary (but safer to do anyway).

The only case I'm not certain about is the SqlExistenceFilter.  Not sure what this is trying to accomplish, but it is only used in a single place under the Messaging UI and the "subquery" is an instance of SqlComparisonFilter with a "%(name)s" type pattern, so I'm pretty confident it's fine.  Will continue to look at this (something seems to have broken with newer versions of the broker which is preventing me from testing it).
Comment 6 Trevor McKay 2012-06-11 22:20:40 EDT
Created attachment 591059 [details]
Patch for the comparison filter
Comment 7 Trevor McKay 2012-07-30 12:57:48 EDT
Created attachment 601316 [details]
Patch for the comparison filter
Comment 8 Vincent Danen 2012-09-19 11:58:19 EDT
Acknowledgements:

This issue was discovered by Florian Weimer of the Red Hat Product Security Team.
Comment 9 errata-xmlrpc 2012-09-19 13:43:46 EDT
This issue has been addressed in following products:

  MRG for RHEL-5 v. 2

Via RHSA-2012:1278 https://rhn.redhat.com/errata/RHSA-2012-1278.html
Comment 10 errata-xmlrpc 2012-09-19 13:52:53 EDT
This issue has been addressed in following products:

  MRG for RHEL-6 v.2

Via RHSA-2012:1281 https://rhn.redhat.com/errata/RHSA-2012-1281.html
Comment 11 Vincent Danen 2012-09-19 17:10:22 EDT
Created cumin tracking bugs for this issue

Affects: fedora-all [bug 858868]
Comment 12 Fedora Update System 2012-11-19 22:02:11 EST
cumin-0.1.5522-4.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 13 Fedora Update System 2012-11-19 22:15:38 EST
cumin-0.1.5522-4.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 14 Fedora Update System 2013-03-13 22:55:13 EDT
cumin-0.1.5522-4.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.