MySQLdb escaping example incorrect

Bug #1745555 reported by Joshua Padman on 2018-01-26
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Security Advisory
Undecided
Unassigned
OpenStack Security Notes
Low
Joshua Padman

Bug Description

The following web page provides information for securing sql queries using parameterized queries.

https://security.openstack.org/guidelines/dg_parameterize-database-queries.html

The first "correct" example under MySQL is incorrect. Firstly, it escapes the entire query which would lead to it being invalid SQL. Secondly, it isn't a real parameterised query.

import MySQLdb

query = "select username from users where username = '%s'" % name
con = MySQLdb.connect('localhost', 'testuser', 'test623', 'testdb');

with con:
    cur = con.cursor()
    cur.execute(MySQLdb.escape_string(query)) <-- escaping the whole query will lead to the ' around the name being escaped.

There are three ways I can see to fix this, my preference is 2:
1. Modify the example as follows - escaping the user input only:
import MySQLdb

query = "select username from users where username = '%s'" % MySQLdb.escape_string(name)
con = MySQLdb.connect('localhost', 'testuser', 'test623', 'testdb');

with con:
    cur = con.cursor()
    cur.execute(query)

2. Do step option 1, plus note that this is still not ideal and that the second example is the actual "correct" parameterised query example.

3. Remove this example from the documentation.

This was bought to my attention by a friend sneakerhax.

Thanks for the report. I've assigned it to the OSSN project, which seems like the group owning the security guidelines.

Changed in ossa:
status: New → Won't Fix
Joshua Padman (jpadman) wrote :

Thank you Tristan for reassigning it. I only opened it in OSSA because the file is in that repository.

http://git.openstack.org/cgit/openstack/ossa/tree/doc/source/guidelines/dg_parameterize-database-queries.rst

Joshua Padman (jpadman) wrote :

Just realised Tristan created one (suggestion 3).
I have no strong feelings either way, Tristan's is probably easier to get the wording/intent correct.
https://review.openstack.org/#/c/539420/

Joshua Padman (jpadman) on 2018-07-10
Changed in ossn:
assignee: nobody → Joshua Padman (jpadman)
importance: Undecided → Low

Change abandoned by Tristan Cacqueray (<email address hidden>) on branch: master
Review: https://review.openstack.org/539420
Reason: Fixed by another change

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers