#7893 CSRF checks don't work on login

v1.3.0
closed
Dave Brondsema
security (36) sf-2 (994)
General
Igor Bondarenko
2015-08-20
2015-06-08
Dave Brondsema
No

CSRFMiddleware deletes all cookies (including login session) if CSRF checks fail. However that doesn't stop a forged login since there isn't a session cookie yet anyway. The login continues and you are logged in.

Also we have no tests for CSRF functionality.

Related

Tickets: #7906

Discussion

  • Dave Brondsema

    Dave Brondsema - 2015-06-08
    • status: in-progress --> review
     

  • Dave Brondsema

    Dave Brondsema - 2015-06-08

    Branch db/7893

    • make sure regular logins still work, particularly in a new browser session and going directly to /auth/ as your first page
    • make sure an off-site login form can't submit successfully (e.g. <form action="http://localhost/auth/do_login" method="POST"> <input type="hidden" name="username" value="test-user" /> <input type="hidden" name="password" value="foo" /> </form>)
    • make sure other operations on /auth/ still work like forgot password, changing settings, etc.
     

  • Igor Bondarenko - 2015-06-09
    • Reviewer: Igor Bondarenko
     

  • Igor Bondarenko - 2015-06-09
    • status: review --> in-progress
     

  • Igor Bondarenko - 2015-06-09

    Overall looks and works good!

    One concern of mine is that /auth/oauth urls now require csrf token. I'm not sure if clients that use oauth will work properly after this change. It seems to me that now clients would have to explicitly send some request to generate csrf token and then send it with all other params. Though, I'm not very familiar with how OAuth work, so can misunderstand something.

    Also, you have a couple of print statements in tests, which probably should be removed.

    +        print 'email working with is', email
+        print 'later', M.EmailAddress.find(dict(email=email_address, claimed_by_user_id=user.
_id)).first()
     

  • Dave Brondsema

    Dave Brondsema - 2015-06-09
    • status: in-progress --> review
     

  • Dave Brondsema

    Dave Brondsema - 2015-06-09
    • new commit cleans up the print statements
    • I'm fairly confident that /auth/oauth is just for the HTML interface that people use to manually manage their oauth apps and tokens. Using oauth tokens in an automated fashion goes through /rest/oauth
     

    • Igor Bondarenko - 2015-06-09

      Right, I meant /rest/oauth there, but typed /auth/oauth. I saw some changes for tests involving /rest/oauth, which have introduced _session_id.

      I see now that one of them is commented out (test_request_token_valid), so maybe these test changes are not required there after all? Since new logic affects only paths that start from /auth/, as far as I can tell from code. If that's the case these tests should also be cleaned up to avoid confusion.

       

      • Dave Brondsema

        Dave Brondsema - 2015-06-09

        I agree. I've removed all the _session_id changes to the /rest/oauth tests. They were unnecessary.

         

  • Igor Bondarenko - 2015-06-09
    • status: review --> closed
     

  • Igor Bondarenko - 2015-06-09

    Looks good, merged.

     

  • Dave Brondsema

    Dave Brondsema - 2015-06-09
    • private: Yes --> No
     

  • Dave Brondsema

    Dave Brondsema - 2015-06-15
    • labels: security, sf-current, sf-2 --> security, sf-2
     

  • Igor Bondarenko - 2015-06-18
    • Milestone: unreleased --> asf_release_1.3.0
     

Log in to post a comment.