Edgewall Software
Modify

Opened 8 years ago

Closed 7 years ago

#11060 closed defect (fixed)

Logout link should be protected

Reported by: vlastimil.zima@… Owned by: Jun Omae
Priority: normal Milestone: 1.0.2
Component: web frontend Version:
Severity: normal Keywords: logout
Cc: Steffen Hoffmann, Jun Omae, Ryan J Ollos Branch:
Release Notes:

Logout requires POST request.

API Changes:
Internal Changes:

Description

Logout link is not protected against CSRF. It is very easy to include link to a fake image, even in Trac itself:

[[Image(http://trac.edgewall.org/logout)]]

It is a kind of security issue, because it is possible to block user from performing any action in private parts of any Trac system.

Used here for demonstration of the same problem: https://code.djangoproject.com/ticket/15619#comment:25

Attachments (0)

Change History (11)

comment:1 by cboos (or was, before being logged out when using the trick presented here...), 8 years ago

Component: generalweb frontend
Keywords: logout added
Milestone: 1.0.2

Could make for nice jokes ;-)

Right, we probably don't want that.

comment:2 by Ryan J Ollos, 7 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:3 by Ryan J Ollos, 7 years ago

Cc: Steffen Hoffmann added

Proposed changes can be found in log:rjollos.git:t11060. I would really appreciate some review on this one, since this is a change that I'm not too confident with. I patterned the change after similar changes for the th:VotePlugin that I worked with Steffen (th:#7744, which led to [th 13086]). I'll look into adding a functional test before committing final changes.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:4 by Peter Suter, 7 years ago

I'm no expert in this area. The common advice seems to suggest using POST requests. (GET requests might open up new attack vectors like logging, referrers, etc. Possibly these are rather harmless or can be mitigated using HTTPS, per-request-tokens etc. But simply using POST requests just seems less fragile.)

Is there a reason we can't use POST requests for logging out? (E.g. by sending the user to a confirmation page.)

In the discussion linked by the reporter a similar approach is proposed.

Trac already has a policy to use this approach for deleting wiki pages etc.


(OT: By the way, your git author name rjollos <rqTkzckbm0CUTEJqsQVX7kLmCHsur67zbkxfywbzVT7Rsbx0yHcqcMeGagsJoAE1> looks a bit weird recently. Is that intentional? It triggers a funny layout on the Trac timeline if the browser window is small and the prefs form gets in the way.)

Last edited 7 years ago by Peter Suter (previous) (diff)

in reply to:  4 comment:5 by Ryan J Ollos, 7 years ago

Replying to psuter:

(OT: By the way, your git author name rjollos <rqTkzckbm0CUTEJqsQVX7kLmCHsur67zbkxfywbzVT7Rsbx0yHcqcMeGagsJoAE1> looks a bit weird recently. Is that intentional? It triggers a funny layout on the Trac timeline if the browser window is small and the prefs form gets in the way.)

Those were changes that I pushed from a VM that I usually don't develop on any longer and I guess there must be something strange with my Git configuration on that system. I'll see if I can get that fixed before pushing any more changes from it.

Thanks for the other hints, I'll take a look tomorrow.

comment:6 by Ryan J Ollos, 7 years ago

Milestone: 1.0.2next-stable-1.0.x

comment:7 by Jun Omae, 7 years ago

Cc: Jun Omae added

Proposed changes in log:jomae.git:t11060.1 which creates an inline form to log out using POST method as metanav item.

comment:8 by Ryan J Ollos, 7 years ago

Owner: changed from Ryan J Ollos to Jun Omae

It looks good to me. Should we target it to 1.0.2?

Two questions:

  • What is the reason for wrapping the Logout text in a span?
  • Do you know the reason why the FunctionalTester.go_to_front() calls fix errors such as the following?
    ======================================================================
    FAIL: runTest (__main__.RegressionTestTicket6318)
    Regression test for non-ascii usernames (#6318)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/user/Workspace/t11060/teo-rjollos.git/trac/tests/functional/testcases.py", line 236, in runTest
        self._tester.logout()
      File "trac/tests/functional/tester.py", line 66, in logout
        tc.follow(r"\bLogout\b")
      File "/home/user/Workspace/twill-0.9/twill/commands.py", line 202, in follow
        raise TwillAssertionError("no links match to '%s'" % (what,))
    TwillAssertionError: no links match to '\bLogout\b'
    
    ----------------------------------------------------------------------
    Ran 11 tests in 12.474s
    
    FAILED (failures=1)
    

in reply to:  8 comment:9 by Jun Omae, 7 years ago

Milestone: next-stable-1.0.x1.0.2

I've verified log:jomae.git:t11060.1 with Firefox 26, Google Chrome 32 beta and IE 8 on Windows.

It looks good to me. Should we target it to 1.0.2?

Agreed. Changed to 1.0.2.

  • What is the reason for wrapping the Logout text in a span?

I couldn't fix invisible border-bottom in Google Chrome or Firefox yesterday…. Ah, I just fixed it. Removed the span in [f6a240c5/jomae.git].

  • Do you know the reason why the FunctionalTester.go_to_front() calls fix errors such as the following?

Once twill submits logout form immediately after login, it is redirected to login. This means it can't log out.

comment:10 by Ryan J Ollos, 7 years ago

Cc: Ryan J Ollos added

comment:11 by Jun Omae, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed in [12379-12380] and merged in [12381].

After the changes, when delete user in th:AccountManagerPlugin, the session probably doesn't log out. See http://trac-hacks.org/browser/accountmanagerplugin/trunk/acct_mgr/web_ui.py#L246.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae 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.