SQL Injection in acct_mgr.api.AccountManager.lastseen()

[Steffen Hoffmann]

In the dawn of 2012-04-25 this claim was brought privately to my attention by Timo "bluec0re" Schmid. The following is a rough translation of the German original email message:

The AccountManagerPlugin for Trac includes an SQL injection vulnerability in the user admin page, more specifically in ap.py:last_seen. There the username is directly included into the SQL statement.

Example: ​http://localhost/admin/accounts/users?user=foobar%27

This vulnerability is hard to exploit, because

1. ) one doesn't get feedback about the query result
2. ) one needs access to the useradmin section as a prerequisite
3. ) one is unable to execute multiple statements at a time. (something like ';INSERT INTO permissions values ('bluec0re', 'TRAC_ADMIN')--` is impossible)

Nevertheless at that place parameter binding should be used as well:

acct_mgr/api.py

@@ -277 +277 @@
              WHERE authenticated=1
             """
         if user:
-            sql = "%s AND sid='%s'" % (sql, user)
-        cursor.execute(sql)
+            sql += " AND sid=?"
+            cursor.execute(sql, (user,))
+        else:
+            cursor.execute(sql)
         # Don't pass over the cursor (outside of scope), only it's content.
         res = []
         for row in cursor:

---

I replicate this information for reference, because I adhere to a strict don't-hide-security-problems policy. IMHO this is the only responsible way to go for a component like AccountManager.

[Steffen Hoffmann]

(In [11545]) AccountManagerPlugin: Fix SQL injection vulnerability, refs #10023.

An instant push to the 0.11 ensures, that this is available for users of the stable branch right now too - without the need to wait for the next release.

Thanks to Timo "bluec0re" Schmid for both, report and suggested correction.

[Steffen Hoffmann]

This should do for the moment.

[Ryan J Ollos]

I know next to nothing about the database API, but reviewing your changeset reminded me of the Trac documentation I'd read here: ​t:TracDev/DatabaseApi#Parameterpassing, which indicates that

sql += " AND sid=?"

is "not okay", and should be replaced by:

sql += " AND sid=%s"

[Steffen Hoffmann]

Replying to rjollos:

I know next to nothing about the database API, but reviewing your changeset reminded me of the Trac documentation I'd read here: ...

Right, the suggested fix was not perfect in this respect. Anyway, if you take a second look at the actual changeset you'll recognize, that I didn't copy it verbatim, but already did it as the docs suggest.

[Ryan J Ollos]

Ah, I wasn't looking closely enough. I had doubted in the back of my mind that you would have made an elementary mistake anyway ;)

[Steffen Hoffmann]

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.