Modify

Opened 5 years ago

Closed 4 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 5 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 5 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 4 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

Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.