Modify

Opened 2 years ago

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

  • Status changed from new to assigned

This should do for the moment.

comment:3 follow-up: Changed 23 months 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 23 months 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 23 months 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 17 months 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 .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from hasienda. Next status will be 'closed'.
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.