Bug 830245 (CVE-2012-2684)

Summary: CVE-2012-2684 cumin: SQL injection flaw
Product: [Other] Security Response Reporter: Vincent Danen <vdanen>
Component: vulnerabilityAssignee: Red Hat Product Security <security-response-team>
Status: NEW --- QA Contact:
Severity: medium Docs Contact:
Priority: medium    
Version: unspecifiedCC: fweimer, grid-maint-list, matt, security-response-team, sgraf, tmckay
Target Milestone: ---Keywords: Security
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard: impact=moderate,public=20120919,reported=20120515,source=redhat,cvss2=4.3/AV:N/AC:M/Au:N/C:N/I:P/A:N,mrg-1/cumin=wontfix,mrg-2/cumin=affected,fedora-all/cumin=affected
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:
Bug Depends On: 840112, 858868    
Bug Blocks: 808230, 828434, 828935    
Attachments:
Description Flags
Patch for the comparison filter
none
Patch for the comparison filter none

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.