Add token to href to prevent CSRF

[HumanInternals]

This isn't critical at all, but there's a CSRF issue. One can force other users to vote for tickets by making them send request to the vote URL. For example, he can embed it as an image in a ticket - and than anyone viewing the ticket and requesting the image would vote-up without knowing.

This can be fixed by passing the token in the URL and making sure its there when processing the request.

[Ryan J Ollos]

I'm not sure I understand the issue well enough to fix it immediately, so a patch would be much appreciated!

[Steffen Hoffmann]

Replying to rjollos:

I'm not sure I understand the issue well enough to fix it immediately, so a patch would be much appreciated!

Hm, I think I do. The OP would like to have some unique request ID associated with the down-/up-vote links to check for it on submit, like Trac does for HTML forms in its templates. Although the assertion about not noticing it is likely bogus. He/she would see the vote in the ticket at least after getting redirected to it after the vote and could even correct it, right?

I've seen these trac_form_token cookies before, see trac.web.main.RequestDispatcher._get_form_token for implementation details.

[Ryan J Ollos]

Replying to hasienda:

Hm, I think I do.

Cool, feel free to commit if you come up with a patch. I look forward to learning from your solution :)

[Steffen Hoffmann]

Replying to rjollos:

Cool, feel free to commit if you come up with a patch. I look forward to learning from your solution :)

I've prepared a patch that adds working protection.

Just one question: Would you agree to raise an TracError on token verification failure? This seems a bit harsh, but

1. Trac does the same on form token verification failure
2. the request in question is done from within JavaScript, and I couldn't find a better way to get an error message from there to the web-UI.

[Ryan J Ollos]

I did some hunting around, and I think ​this code is the form token validation that is done by Trac that you referred to.

If I understand this correctly:

1. The token is added to the template by Trac (see ​here).
2. In your JavaScript code, you get the token and pass it back in the request data, like Trac does at various places, such as ​here.
3. Some token validation must be performed in VoteSystem.process_request, at which point you can choose to raise some sort of error if the validation fails.

Is that correct? I've studied the code in Trac a bit, but I don't know enough at this point to offer any guidance as to what is best. I certainly look forward to seeing the patch though, and will do some testing once you have it pushed.

There's a ​FormTokenInjector class, but I don't see it utilized anywhere in the Trac codebase.

[Steffen Hoffmann]

We'll probably agree then on something like the following patch.

I've been coming to similar conclusions, but would prefer to do most of the token handling in Python rather than JavaScript. Even if it is technically the same token I suggest going with 'link_token' (or even just 'token'?). There is no form involved at all, so it could be confusing to refer to it with identical descriptor as for Trac core's hidden form field. And because there is no form involved, we can't use the built-in CSFR detection code, but will do something similiar in VoteSystem.process_request, agreed.

I'll attach my current preliminary changes to demonstrate, what I have in mind for resolving the issue here (tested only in Trac 1.0 yet, works well so far).

[Steffen Hoffmann]

current patch version right from development stack

[Ryan J Ollos]

Replying to hasienda:

I've been coming to similar conclusions, but would prefer to do most of the token handling in Python rather than JavaScript. Even if it is technically the same token I suggest going with 'link_token' (or even just 'token'?). There is no form involved at all, so it could be confusing to refer to it with identical descriptor as for Trac core's hidden form field. And because there is no form involved, we can't use the built-in CSFR detection code, but will do something similiar in VoteSystem.process_request, agreed.

Make sense. The code looks good.

I did an experiment to invalidate the token by editing the HTML. I get a warning in the log,

01:17:32 AM Trac[main] WARNING: [127.0.0.1] HTTPInternalError: 500 Trac Error (Do you have cookies enabled?)

and the up/down arrows no longer have any action, but there is no direct error sent to the user. This behavior is probably good, and I think I understand now your previous comment about trying to get the error message to the end user. If it was desirable, is there any way you know of that we could pass back an error to the JavaScript code, perhaps using add_script_data (and some compatibility code for Trac < 0.12)?

I made a few minor suggested changes in fx-7744-2.patch​. Nothing more here than a minor refactoring and addition of a translation marker. Tested in Trac 1.0.2dev.

[Steffen Hoffmann]

You could test much easier like I did: Just copy an URL and modify or remove the token entirely. And a TracError page should jump out immediately. Not sure, how you tested it, but from the error you've shown above, it is Trac's form validation, not my patched code, that has been triggered.

I've had the translation markers in place before, and removed them on purpose: There is no i18n support for this plugin right now, and since I customized messages, there is no use for marking them as translatable so far. But I'll re-add and extract that messages as soon as I get i18n in here, ok?

[Steffen Hoffmann]

Replying to rjollos:

Replying to hasienda:

I've been coming to similar conclusions, ...
And because there is no form involved, we can't use the built-in CSFR detection code, but will do something similiar in VoteSystem.process_request, agreed.

Make sense. The code looks good.

I made a few minor suggested changes in fx-7744-2.patch​. Nothing more here than a minor refactoring ...

A sure. Silly me. Of course below if old_vote == vote: it has to be like you did it, shorter and cleaner. Thanks for the hint, I'll alter my patch accordingly.

[Ryan J Ollos]

Replying to hasienda:

You could test much easier like I did: Just copy an URL and modify or remove the token entirely. And a TracError page should jump out immediately. Not sure, how you tested it, but from the error you've shown above, it is Trac's form validation, not my patched code, that has been triggered.

Oh, right. I didn't bother to look at the error carefully enough. I think I'm still not understanding this entirely though. If I look at the HTML for the page I see:

<span id="vote" title="Vote count (+1)">
  <a href="/tracdev/vote/up/ticket/1?link_token=6d4d48bcf3537892b4a7e899" id="upvote" title="Up-vote">
    <img src="/tracdev/chrome/vote/aupmod.png" alt="Up-vote">
  </a>
<span id="votes">+1</span>
  <a href="/tracdev/vote/down/ticket/1?link_token=6d4d48bcf3537892b4a7e899" id="downvote" title="Down-vote">
    <img src="/tracdev/chrome/vote/adowngray.png" alt="Down-vote">
  </a>
</span>

If I send a request to one of the hrefs, the vote increments or decrements. If I edit or remove the token and then send a request to the edited href, I get the TracError, like you describe.

However, if I edit the HTML to change the token and then click on the vote links, the vote is not recorded and I get the "Trac form handling" error:

04:35:31 PM Trac[main] WARNING: [127.0.0.1] HTTPInternalError: 500 Trac Error (Do you have cookies enabled?)

However, now I'd expect to get the same TracError as when sending a request after editing the token.

Anyway, my need to understand all the details shouldn't hold up pushing the change!

[Ryan J Ollos]

Here is more detail about what I did. In Chrome Developer Tools Inspector I edited the last character of the up-vote href from 9 -> 8:

Now, the down-vote still works as expected, but the up-vote is not recorded and I don't see the TracError page either. I see the HTTPInternalError when directing the debug output to the terminal.

[Steffen Hoffmann]

Ah I see now.

The token is not bound to a particular link, as well as the form token is not bound to a specific form. So it doesn't protect against altering link targets (or forms protected by form token), just against putting out false vote links or forms. You just don't know other-one’s token.

If you could alter content of a user's request or Trac's reply, you could mangle pretty much everything, and it doesn't hold as an attack scenario from my point of you. That Trac would just be compromised itself, right?

[Steffen Hoffmann]

Replying to rjollos:

Now, the down-vote still works as expected, but the up-vote is not recorded and I don't see the TracError page either. I see the HTTPInternalError when directing the debug output to the terminal.

Hm, according to my tests it should, but to the altered resource, if that one exists. But you'll see that when being re-directed (to the resource you voted on, ticket 8 in your case) right after the vote has been processed.

[Ryan J Ollos]

Oh, I didn't change the resource, I just changed one character in the token. I thought that by invalidating the token in the HTML and then attempting to submit a vote, the TracError should be raised, and if not visible on the page itself, then at least it would be found in the logs. I was attempting to simulate a vote with an incorrect token by altering the token in the HTML (and specifically, the query string in the href for the vote link).

Going back to the reporter's comment, I tried that experiment by embedding:

{{{
#!html
<a href="/tracdev/vote/up/ticket/1?link_token=6d4d48bcf3537892b4a7e898" id="upvote" title="Up-vote">
  <img src="http://www.nasa.gov/images/content/711375main_grail20121205_4x3_946-710.jpg" width="500">
</a>
}}}

If the href for the a has a correct token, then the image will change to a giant up arrow when clicked on and the vote will be recorded. Good. If you embed an incorrect token, then there will be no effect when clicking on the image, and the following is seen in the logs:

11:31:45 PM Trac[main] WARNING: [127.0.0.1] HTTPInternalError: 500 Trac Error (Do you have cookies enabled [TracVote])

You see, I changed the error message to append [TracVote], so I can be sure that error is actually coming from TracVote.process_request and not from the Trac code that handles the form token. I was surprised that the error was an HTTPInternalError and not a TracError.

Okay, I think I understand now. My conclusion is, if a user attempts to vote but the token is incorrect somehow, then they won't see any effect. If the token in the href for the vote image was somehow incorrect, the user will see that there is no vote being recorded, but they won't be redirected to a page displaying the TracError.

[Steffen Hoffmann]

Replying to rjollos:

You see, I changed the error message to append [TracVote], so I can be sure that error is actually coming from TracVote.process_request and not from the Trac code that handles the form token. I was surprised that the error was an HTTPInternalError and not a TracError.

Smart move to mark the error message that way. I'm surprised too. Obviously the TracError doesn't bubble up as it works in my setup, but it raises the HTTPInternalError on its way up instead.

So I wonder how our setups are different. Until now I only know, that you use a different browser than me, but I see different Trac behavior, so I suspect the root cause rather there. For what its worth I've tested exclusively with Trac 1.0.1 here by now.

... If the token in the href for the vote image was somehow incorrect, the user will see that there is no vote being recorded, but they won't be redirected to a page displaying the TracError.

Protection is working ok, but I'd really like to fix that too. The user should be notified, that there was something going wrong, so he/she could watch out for the reason.

[Ryan J Ollos]

Replying to hasienda:

Smart move to mark the error message that way. I'm surprised too. Obviously the TracError doesn't bubble up as it works in my setup, but it raises the HTTPInternalError on its way up instead.

I've been using Trac 1.0.2dev from the ​t:browser:/branches/1.0-stable branch. My dev environment is setup following the steps described at ​t:TracDev/DevelopmentEnvironmentSetup (tracd and SQLite). I'll try testing with Trac 1.0.1, and some other browsers as well.

[Steffen Hoffmann]

Looking forward to your results. In theory the difference regarding Trac revisions shouldn’t matter. I guess a plugin issue would be more likely. OTOH theory may be a step behind, when it comes to practice. ;-)

[Ryan J Ollos]

I tried with Trac 1.0.1 in Chromium 25 and Firefox 20 and I get the same result as before.

I'm running with just fx-7744.patch​ on top of r13012. I just realized that your patch appears to apply on top of your patches from #10942. I am guessing that is what accounts for the difference here, but I will test again to confirm once you have everything pushed to the t-h.o repository.

[Steffen Hoffmann]

Replying to rjollos:

I just realized that your patch appears to apply on top of your patches from #10942. I am guessing that is what accounts for the difference here, but I will test again to confirm once you have everything pushed to the t-h.o repository.

Yes, this is true. I'll review your reply to #10942 and do next iteration or to push results at least by the end of this week, provided agreement in all major changes. Thanks for the follow-up.

[Steffen Hoffmann]

(In [13081]) VotePlugin: Allow vote requests only from resource view pages, refs #7744.

Because the vote request always redirects to the standard resource view, it will be undesired to vote from pages like i.e. wiki page editor.

[Steffen Hoffmann]

(In [13086]) VotePlugin: Insert and evaluate tokens to prevent CSFR attacks, refs #7744.

Code has been re-used from Trac core, and I've been lucky to get pre-commit review of these changes by Ryan J Ollos, who provided the simpler set_vote.

[Steffen Hoffmann]

All pending issues are addressed as far as I can see.

Therefor I propose to actually tag next version of this plugin as votes-0.2 after some more testing.

[Ryan J Ollos]

0.2.0 tagged in [14760]. 0.3.0 tagged in [14762].