Modify

Opened 3 years ago

Closed 2 years ago

#10023 closed defect (fixed)

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

Reported by: hasienda Owned by: hasienda
Priority: high Component: AccountManagerPlugin
Severity: minor Keywords: sql injection security
Cc: rjollos, otaku42 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 3 years ago by hasienda

(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 3 years ago by hasienda

  • Status changed from new to assigned

This should do for the moment.

comment:3 follow-up: Changed 3 years ago by rjollos

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 3 years ago by hasienda

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 3 years ago by rjollos

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 2 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Add Comment

Modify Ticket

Action
as closed The owner will remain hasienda.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.