Edgewall Software
Modify

Opened 17 years ago

Closed 17 years ago

Last modified 4 years ago

#4049 closed defect (fixed)

CSRF vulnerabilities in trac

Reported by: dkg-debian.org@… Owned by: Jonas Borgström
Priority: high Milestone: 0.10.2
Component: general Version: 0.10
Severity: critical Keywords: CSRF security
Cc: dkg-debian.org@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

Despite [1701], trac appears to still be vulnerable to a fairly widespread class of Cross Site Request Forgery attacks.

These attacks require minimal action by the user: All the user needs to do is:

  1. be logged into a targeted trac installation, and
  2. visit a malicious remote web site (or click a malicious remote link) with the same browser

Because these attacks make use of the browsers' cached credentials, and they are launched from the browser (not from the malicious remote site), firewalls and other perimeter restrictions are not useful against them.

I've put a demonstration of this attack up here, along with details of how trac might be modified to protect against such an attack.

Please let me know if i can be of any help fixing this.

Attachments (6)

trac-csrf.patch (3.0 KB ) - added by Jonas Borgström 17 years ago.
A proof of concept patch for trunk
trac-csrf.2.patch (2.9 KB ) - added by Jonas Borgström 17 years ago.
An updated version of the patch
trac-csrf.3.patch (2.9 KB ) - added by Jonas Borgström 17 years ago.
A safer and cleaner version of the patch.
trac-0.10-csrf.patch (4.9 KB ) - added by Jonas Borgström 17 years ago.
A version for 0.10-stable, by far not as elegant.
trac-0.10-csrf.2.patch (5.1 KB ) - added by Jonas Borgström 17 years ago.
An updated version of the 0.10-stable patch.
trac-0.10-csrf.3.patch (5.1 KB ) - added by Jonas Borgström 17 years ago.
An updated version of the 0.10-stable patch.

Download all attachments as: .zip

Change History (33)

comment:1 by dkg-debian.org@…, 17 years ago

Assuming the conditions above are met, the remote malicious site can perform any actions on the targeted trac installation that the browsing user is authorized to do. I believe this is a serious security concern.

comment:2 by dkg-debian.org@…, 17 years ago

I should add that i've tried this and tested it successfully using a fairly standard Firefox 1.5 installation, and konqueror 3.5.5, on a different trac installation. The exploit was tuned to the other installation.

I just made a couple changes to the exploit to make it function with this trac installation so that it is more apparent. If you tried it already but it failed and you want to see it in action, refresh the linked page (perhaps with your cache cleared).

sigh. even my bug reports have bugs!

comment:3 by anonymous, 17 years ago

Keywords: security added
Priority: normalhigh
Severity: normalcritical
Summary: CSRF vulnerabilities in tracTest Ticket for CSRF on Trac

I got burned by this exploit.

comment:4 by Christopher Lenz, 17 years ago

I got burned by this exploit.

comment:5 by Christian Boos, 17 years ago

Summary: Test Ticket for CSRF on TracCSRF vulnerabilities in trac

I got burned by this exploit.

comment:6 by dkg-debian.org@…, 17 years ago

OK, that's two more: Firefox/2.0 and Safari/419.3

Let me know if you want me to take down the exploit itself.

comment:7 by anonymous, 17 years ago

I got burned by this exploit.

comment:8 by anonymous, 17 years ago

I got burned by this exploit.

comment:9 by Christian Boos, 17 years ago

Milestone: 0.10.1

Well, I've got the php source, so yes you can take it down ;)

As you said, there's nothing special to do in it… so this is indeed a bit scary.

comment:10 by anonymous, 17 years ago

I got burned by this exploit.

comment:11 by dkg-debian.org@…, 17 years ago

i've taken down the exploit

by Jonas Borgström, 17 years ago

Attachment: trac-csrf.patch added

A proof of concept patch for trunk

comment:12 by Jonas Borgström, 17 years ago

Status: newassigned

The patch above automatically adds a form token to every method="post" form. The token generation might not be strong enough. It's probably better to use the session id instead of the username to build the token.

by Jonas Borgström, 17 years ago

Attachment: trac-csrf.2.patch added

An updated version of the patch

by Jonas Borgström, 17 years ago

Attachment: trac-csrf.3.patch added

A safer and cleaner version of the patch.

by Jonas Borgström, 17 years ago

Attachment: trac-0.10-csrf.patch added

A version for 0.10-stable, by far not as elegant.

by Jonas Borgström, 17 years ago

Attachment: trac-0.10-csrf.2.patch added

An updated version of the 0.10-stable patch.

comment:13 by Jonas Borgström, 17 years ago

I've now committed trac-csrf.3.patch to trunk r4133.

The 0.10-stable version still needs more testing before we can commit anything to the stable branch.

by Jonas Borgström, 17 years ago

Attachment: trac-0.10-csrf.3.patch added

An updated version of the 0.10-stable patch.

comment:14 by dkg-debian.org@…, 17 years ago

trac-0.10-csrf.3.patch seems to work when applied to 0.10 for me. my original exploit fails against it. Nice work, jonas.

comment:15 by Jonas Borgström, 17 years ago

I've applied trac-0.10-csrf.3.patch to the 0.10-stable branch and upgraded t.e.o to get some good testing.

comment:16 by ThurnerRupert, 17 years ago

we get

 WARNING: 500 Internal Server Error (500 Internal Server Error
 (Missing or invalid form token Do you have cookies enabled?))

either caused by r4147, or r4145.

usually we have http://trac-hacks.org/wiki/AccountManagerPlugin enabled as well, and i thought it might be incompatible with that. trying to go back to

 trac.web.auth.LoginModule = enabled
 # acct_mgr.web_ui.LoginModule = enabled

had no effect. finally we did a roll back of the two changesets.

comment:17 by Jonas Borgström, 17 years ago

Ok, I've committed an updated version of the CSRF patch uses a new cookie (trac_form_token) to keep trac of the token. The trac_session/auth cookie is no longer reused. I think this will solve the problem.

comment:18 by ThurnerRupert, 17 years ago

thanks a lot, now it works.

comment:19 by ThurnerRupert <rupert.thurner@…>, 17 years ago

Milestone: 0.10.20.10.1
Resolution: fixed
Status: assignedclosed

shouldn't be this be closed, by #4153?

comment:20 by ThurnerRupert <rupert.thurner@…>, 17 years ago

i ment r4153

comment:21 by Christopher Lenz, 17 years ago

Yeah, thanks.

comment:22 by Shun-ichi Goto <shunichi.goto@…>, 17 years ago

As side effect, #4122 is raised.

comment:23 by dkg-debian.org@…, 17 years ago

For those who upgraded to trac 0.10.1 to work around this bug and you are using an authz file, you probably need to apply [4201] and [4207] to your trac install to avoid #3996.

comment:24 by Christian Boos, 17 years ago

Resolution: fixed
Status: closedreopened

Well, using the HTMLParser has its limits… see e.g. http://dog4.dyndns.org:8080/trac/wiki/AxisAngleRotation (picked from a conversation on IRC).

There, simply viewing a wiki page fails because the LaTeX formula plugin probably returns some invalid HTML.

What about adding explicitely the <input type="hidden" name="__FORM_TOKEN" value=<?cs var:__FORM_TOKEN ?>"> element in all the forms, instead of using the injector?

That would be a one time fix, as existing ClearSilver templates won't evolve anymore. Same goes for plugins, the few requiring POSTs could be easily adapted.

in reply to:  24 comment:25 by Emmanuel Blot, 17 years ago

Milestone: 0.10.10.10.2

Replying to cboos:

There, simply viewing a wiki page fails because the LaTeX formula plugin probably returns some invalid HTML.

Actually, 0.10.1 has triggered one HTML syntax issue in XmlRpcPlugin, another one in GraphvizPlugin.

comment:26 by Jonas Borgström, 17 years ago

Resolution: fixed
Status: reopenedclosed

As far as I know the XmlRpcPlugin issue was about FORM_TOKEN validation and not HTML syntax. This should be fixed in 0.10-stable already.

The GraphvizPlugin issue is both invalid HTML and invalid XHTML so I guess they will need to find some other way to do this. IMHO it's not important enough to block the 0.10.2 release.

in reply to:  26 comment:27 by Emmanuel Blot, 17 years ago

Replying to jonas:

The GraphvizPlugin issue is both invalid HTML and invalid XHTML so I guess they will need to find some other way to do this. IMHO it's not important enough to block the 0.10.2 release.

+1.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jonas Borgström.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jonas Borgström to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.