SQL injection through limit parameter

Bug #918608 reported by Thierry Carrez on 2012-01-19
262
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Critical
Ziad Sawalha

Bug Description

Received as encrypted email from Nikita Savin (GridDynamics):

* Background
1) in <=sqlalchemy-0.7.0b limit() function not checked parameter is integer (actually in 0.6.7 in changelog this marked as fixed, but this issue still exists in 0.6.8). http://www.sqlalchemy.org/changelog/CHANGES_0_6_7, http://www.sqlalchemy.org/changelog/CHANGES_0_7_0
2) keystone api not check limit parameter is integer and pass it to sqlaclhemy as is, resulting in sql injection

* Workaround
Upgrade sqlalchemy to >= 0.7.0b

* Patch
straightforward patch for keystone-2012.1 attached
probably it will be good idea to re-write work with API parameters like it done in Glance (with SUPPORT_PARAMS and strict checking)

CVE References

Thierry Carrez (ttx) wrote :
Thierry Carrez (ttx) wrote :

Note that this issue raises a new vulnerability process question: should we release advisories for a project that was never officially released ? (My take on it is that it's sufficiently serious for us to do an advisory even if no "official" version exists)

Thierry Carrez (ttx) wrote :

Adding Ziad as keystone PTL -- please look at the patch and pre-approve it. Please do not make this issue public until the end of the embargoed period (still undetermined).

Changed in keystone:
importance: Undecided → High
status: New → Confirmed
importance: High → Critical
Thierry Carrez (ttx) wrote :

I wouldn't be surprised if the CVE allocation folks would consider this all to be an SQLAlchemy vulnerability. After all, it should be the responsibility of the ORM to protect you against this kind of stuff. We'll see how it goes.

Thierry Carrez (ttx) wrote :

Proposed description for stakeholders:

Title: SQLAlchemy/Keystone SQL injection through limit handling
Impact: Critical
Reporter: Nikita Savin
Products: Keystone
Affects: All versions

Description:
Nikita Savin from GridDynamics reported a vulnerability in Keystone's usage of SQLAlchemy. Before version 0.7.0, SQLAlchemy did not sanitize the contents of the limit clause, potentially allowing SQL injection. Keystone directly passes user-provided values to the SQLAlchemy call, triggering SQL injection on vulnerable SQLAlchemy versions, potentially resulting in complete compromise of the authentication database. Only setups using SQLAlchemy < 0.7.0 are affected.

Patch: The attached patch works around the issue in Keystone itself, by coercing limit values to integer. Alternatively, this can be fixed by upgrading to SQLAlchemy >= 0.7.0.

Note: This could all be considered a vulnerability in SQLAlchemy, and a CVE assigned against that (silently-fixed) issue.

Russell Bryant (russellb) wrote :

I do agree that this is a SQLAlchemy vulnerability and not so much a keystone vulnerability. I can work this issue from that angle. I'll look into getting a CVE allocated for it. I think it's still worth posting an OpenStack security advisory about this to help make users aware of the problem. The advisory can just reference the SQLAlchemy CVE.

As to the general process question, I think that if a project is a core Openstack project, it's a good idea to release a security advisory, even if there hasn't been an official release yet.

Russell Bryant (russellb) wrote :

I have contacted the security response team at Red Hat about the SQLAlchemy issue. They will handle allocating a CVE for it and coordinating with linux-distros as necessary.

    https://access.redhat.com/security/team/
    https://access.redhat.com/security/team/contact/

Once I hear something back, I'll note it here so that it can be referenced in the OpenStack advisory.

Ziad Sawalha (ziad-sawalha) wrote :

I'll incorporate Nikita's patch into the code for E3 (patch already submitted)

Changed in keystone:
assignee: nobody → Ziad Sawalha (ziad-sawalha)
milestone: none → essex-3
Changed in keystone:
status: Confirmed → In Progress
Thierry Carrez (ttx) wrote :

Russell: +1
We just need to rewrite the description so that's it's clear the security advisory is about an SQLAlchemy vulnerability that affects Keystone negatively, and not a Keystone issue.

Maybe something like:

Title: SQLAlchemy SQL injection through limit hangling
Impact: Critical
Reporter: Nikita Savin
Products: Keystone
Affects: Versions prior to Keystone Essex-3 milestone

Description:
Nikita Savin from GridDynamics reported a vulnerability in SQLAlchemy. Before version 0.7.0, SQLAlchemy did not sanitize the contents of the limit clause, potentially allowing SQL injection. Since Keystone passes user-provided values to the SQLAlchemy limit calls, it may be abused to trigger that SQL injection on vulnerable SQLAlchemy versions, potentially resulting in complete compromise of the authentication database. Only setups using SQLAlchemy < 0.7.0 are affected.

Solution:
Users are advised to upgrade to unaffected versions of SQLAlchemy (>= 0.7.0). Keystone Essex-3 milestone also contains a workaround that solves the issue for affected versions of SQLAlchemy.

Nikita Savin (nsavin) wrote :

For sure this is issue in SQLAlchemy and it has to sanitize limit().
But this is partially issue in Keystone too - this is usually very good idea to sanitize all user's input. Especially in public API.
For example, glance. It supports limit in API. And it works with the same buggy SQLAlchemy. But glance API control user input, so no sqlinjection like in keystone, but rather meaningful error message:
> curl 'localhost:9191/images?limit=1"'
<html>
 <head>
  <title>400 Bad Request</title>
 </head>
 <body>
  <h1>400 Bad Request</h1>
  The server could not comply with the request since it is either malformed or otherwise incorrect.<br /><br />
limit param must be an integer

 </body>
</html>

Thierry Carrez (ttx) wrote :

@Nikita: I certainly agree. The description must walk a thin line between blaming keystone for making reasonable assumptions about SQLAlchemy and ignoring Keystone vulnerability completely. In this case I'd say the vulnerability is in SQLAlchemy, and the lack of a parameter sanitization layer in Keystone made it worse.

Changed in keystone:
status: In Progress → Fix Committed
Thierry Carrez (ttx) wrote :

@Russell: the fix is public now... this bug should be opened. How is the linux-distros disclosure going ? If we do an advisory, it would need to go out ASAP.

Nikita Savin (nsavin) on 2012-01-24
Changed in keystone:
status: Fix Committed → Fix Released
Nikita Savin (nsavin) wrote :

oops, changed status to "Fix Released" by mistake and can't return it back. Can anybody fix ?

Thierry Carrez (ttx) on 2012-01-24
Changed in keystone:
status: Fix Released → Fix Committed
Russell Bryant (russellb) wrote :

I've asked for an update from the Red Hat security team. It looks like they did verify the vulnerability in SQLAlchemy, assigned a CVE (CVE-2012-0805), and are now working on coordinating a release. I'll report back when I get a date. I know the fix is already out, but ideally I think this bug should stay private until the coordinated release for the CVE.

After thinking about it some more, I don't feel strongly that we need to an OpenStack advisory for this. Do we want to release an advisory every time a project that OS uses has a vulnerability? I wouldn't think so, except perhaps for extreme circumstances.

Thierry Carrez (ttx) wrote :

If they are coordinating a release, better keep that one below the radar until that's done. I also agree that we should not issue an OpenStack advisory on this... but that doesn't prevent us from sending an information e-mail.

Thierry Carrez (ttx) on 2012-01-25
Changed in keystone:
status: Fix Committed → Fix Released
Russell Bryant (russellb) wrote :

The coordinated release date is Tuesday, January 31st. We can open this bug and post a note to the list then.

Thierry Carrez (ttx) wrote :

@Russell: we are way past Jan 31st now. Could you open this and post a "security notice" to the list ?

visibility: private → public
Russell Bryant (russellb) wrote :

opened up and message sent to to the list, sorry for the delay

Thierry Carrez (ttx) on 2012-04-05
Changed in keystone:
milestone: essex-3 → 2012.1
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers