Raw SQL used in swift/swift/common/db.py could be escaped

[Thierry Carrez]

Grant Murphy (<email address hidden>) conducted an audit of OpenStack and reported the following potential SQL injection vulnerabilities in Swift and Nova. These may well not be exploitable, we need to doublecheck them.

swift/swift/common/db.py:376: UPDATE %s_stat SET id=?
swift/swift/common/db.py:379: SELECT ROWID FROM %s ORDER BY ROWID DESC LIMIT 1
swift/swift/common/db.py:403: UPDATE %s_stat SET created_at=MIN(?, created_at),
swift/swift/common/db.py:424: SELECT * FROM %s WHERE ROWID > ? ...
swift/swift/common/db.py:440: "SELECT sync_point FROM %s_sync WHERE remote_id=?"
swift/swift/common/db.py:456: SELECT remote_id, sync_point FROM %s_sync
swift/swift/common/db.py:512: INSERT INTO %s_sync (sync_point, remote_id)
swift/swift/common/db.py:518: UPDATE %s_sync SET sync_point=max(?, sync_point)
swift/swift/common/db.py:561: metadata = conn.execute('SELECT metadata FROM %s_stat' %
swift/swift/common/db.py:592: md = conn.execute('SELECT metadata FROM %s_stat' %
swift/swift/common/db.py:607: conn.execute('UPDATE %s_stat SET metadata = ?' %
swift/swift/common/db.py:633: md = conn.execute('SELECT metadata FROM %s_stat' %
swift/swift/common/db.py:644: conn.execute('UPDATE %s_stat SET metadata = ?' %

nova/nova/virt/hyperv/volumeutils.py:78: "WHERE TargetName='%s'" % target_iqn)
nova/nova/virt/hyperv/hostutils.py:66: "WHERE DeviceID='%s'"
nova/nova/virt/hyperv/basevolumeutils.py:123: "Class WHERE TargetName='%s'"
nova/nova/db/sqlalchemy/utils.py:64: return "INSERT INTO %s %s" % (
nova/nova/db/sqlalchemy/migrate_repo/versions/152_change_type_of_deleted_column.py:40:
   return "INSERT INTO %s %s" % (

[Chuck Thier (cthier)]

For Swift:

The calls listed are used specifically to make code reusable, and the variables are only set within the code, and are not derived from user input, and thus are not at risk for injection.

[Chuck Thier (cthier)]

That said, I'm also not against reporting it as a bug, and having them properly escaped in swift, but it shouldn't be reported as a vulnerability for swift.

[Thierry Carrez (ttx)]

Awesome, thanks Chuck for the analysis.

I'll mark Swift task as Invalid as far as the "vulnerability" is concerned... If they all turn out to be code improvements I'll just reopen the task and use this bug to track further fixes... otherwise if this bug turns out covering a real vulnerability I'll create a separate (wishlist) bug report for Swift to track this.

[Thierry Carrez (ttx)]

Adding Alessandro and Boris so that they can look into Nova hyper-v and sqlalchemy cases.
Are those raw SQL statements operating on user-controllable input ?

[Alessandro Pilotti (alexpilotti)]

On the Nova Hyper-V side, those are not SQL instructions. It's WQL, a pseudo-SQL language used by WMI, very limited in comparison with SQL.

Although those instructions are not exploitable, sanitizing the input for WQL is generally a good idea.

Adding also Pedro for the Volumeutils bits.

[Boris Pavlovic (boris-42)]

I saw this code in nova/virt/hyperv/hostutils.py:

        logical_disk = self._conn_cimv2.query("SELECT Size, FreeSpace "
                                              "FROM win32_logicaldisk "
                                              "WHERE DeviceID='%s'"
                                              % drive)[0]

It doesn't look like safe=)

[Alessandro Pilotti (alexpilotti)]

1) That's not SQL, It's WQL, a WMI specific language. There'n no database involved and there's no DML or DDL.

2) That method is used only by hostops.py:

drive = os.path.splitdrive(self._pathutils.get_instances_dir())[0]
        (size, free_space) = self._hostutils.get_volume_info(drive)

https://github.com/openstack/nova/blob/master/nova/virt/hyperv/hostops.py#L80

Now, please tell me how you want to exploit it :-)

[Boris Pavlovic (boris-42)]

Ok in this current situation it is imposible to exploit it,

but without such context, this method is unsafe. I agree that with WQL you are not able to execute any Alter or Drop sql request. But you are able to run something like delete * from win32_logicaldisk =)

[Alessandro Pilotti (alexpilotti)]

No. As I wrote in a previous comment, there's no DML or DDL. The only SQL analog verb supported by WQL is "SELECT".

For your convenience here's a list of the WQL keywords:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa394606(v=vs.85).aspx

Generally speaking, as I wrote before, I agree in putting input sanitization checks as a rule of thumb when we use WQL, although there are no real exploit concerns. Its SQL-like syntax, unfamiliar to those which are not involved in Windows development, can understandably generate discussions such as this one. :-)

Note: WQL does not support parameters, so we have create an ad hoc method to manually escape the WQL string quotes.

[Boris Pavlovic (boris-42)]

ok =)

[Thierry Carrez (ttx)]

@Boris: could you comment on the exploitability of:

nova/nova/db/sqlalchemy/utils.py:64: return "INSERT INTO %s %s" % (
nova/nova/db/sqlalchemy/migrate_repo/versions/152_change_type_of_deleted_column.py:40: return "INSERT INTO %s %s"

[Boris Pavlovic (boris-42)]

Ok I wrote that part of code, it is really imposible to make anything bad with this part of code.

sqlalchemy/utils.py are utils that are used only in db migrations.
DB migrations are runed only by host administrators with root access to host + root access to db.

So if you are able to run this code, you are able to make something like:
ssh root@my_control_host
mysql -u...
> drop database nova

=)

Also there is no user/administrator input at all (input parameters for this functions are hardcoded in db migrations)

[Thierry Carrez (ttx)]

That's what I thought... but better safe than sorry.

@ Kurt:

Given that none of those occurences are actually exploitable, I think it makes sense to open this bug report and treat it as a wishlist strengthening improvement to get rid of the SQL calls where they could be replaced by library calls, if any.

Let me know if you object to opening this bug report.

[Boris Pavlovic (boris-42)]

@ Kurt, @Thierry

Just to be clear,
I would prefer to avoid any changes in db migrations scripts and db migration utils

[Alessandro Pilotti (alexpilotti)]

@Thierry

On the WMI side (Hyper-V) the pseudo SQL syntax is simply the way in which the APIs are designed to be used.

[Thierry Carrez (ttx)]

Turning into Wishlist bug as suggested by Chuck on comment 2