Bug 830245 (CVE-2012-2684)

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....

(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.

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).

Created attachment 591059 [details]
Patch for the comparison filter

Created attachment 601316 [details]
Patch for the comparison filter

Acknowledgements:

This issue was discovered by Florian Weimer of the Red Hat Product Security Team.

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

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

Created cumin tracking bugs for this issue

Affects: fedora-all [bug 858868]

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.

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.

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.