Modify

Opened 6 years ago

Closed 5 years ago

#10023 closed defect (fixed)

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

Reported by: Steffen Hoffmann Owned by: Steffen Hoffmann
Priority: high Component: AccountManagerPlugin
Severity: minor Keywords: sql injection security
Cc: Ryan J Ollos, Michael Renzmann Trac Release: 0.11

Description

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

     
    277277             WHERE authenticated=1
    278278            """
    279279        if user:
    280             sql = "%s AND sid='%s'" % (sql, user)
    281         cursor.execute(sql)
     280            sql += " AND sid=?"
     281            cursor.execute(sql, (user,))
     282        else:
     283            cursor.execute(sql)
    282284        # Don't pass over the cursor (outside of scope), only it's content.
    283285        res = []
    284286        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.

Attachments (0)

Change History (6)

comment:1 Changed 6 years ago by 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.

comment:2 Changed 6 years ago by Steffen Hoffmann

Status: newassigned

This should do for the moment.

comment:3 Changed 5 years ago by 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"

comment:4 in reply to:  3 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by 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 ;)

comment:6 Changed 5 years ago by Steffen Hoffmann

Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.