Opened 14 years ago
Closed 9 years ago
#7744 closed defect (fixed)
Add token to href to prevent CSRF
Reported by: | HumanInternals | Owned by: | Steffen Hoffmann |
---|---|---|---|
Priority: | normal | Component: | VotePlugin |
Severity: | normal | Keywords: | CSRF token |
Cc: | Trac Release: | 0.11 |
Description
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.
Attachments (3)
Change History (27)
comment:1 follow-up: 2 Changed 14 years ago by
comment:2 follow-up: 3 Changed 12 years ago by
Keywords: | CSRF token added |
---|
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.
comment:3 follow-up: 5 Changed 12 years ago by
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 :)
comment:4 Changed 12 years ago by
Owner: | changed from Ryan J Ollos to Steffen Hoffmann |
---|
comment:5 Changed 12 years ago by
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
- Trac does the same on form token verification failure
- 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.
comment:6 Changed 12 years ago by
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:
- The token is added to the template by Trac (see here).
- 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.
- 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.
comment:7 Changed 12 years ago by
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).
Changed 12 years ago by
Attachment: | fx-7744.patch added |
---|
current patch version right from development stack
Changed 12 years ago by
Attachment: | fx-7744-2.patch added |
---|
comment:8 follow-up: 10 Changed 12 years ago by
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
.
comment:9 follow-up: 11 Changed 12 years ago by
Priority: | low → normal |
---|
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?
comment:10 Changed 12 years ago by
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 inVoteSystem.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.
comment:11 Changed 12 years ago by
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 href
s, 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!
Changed 12 years ago by
Attachment: | ChromeInspector.png added |
---|
comment:12 follow-up: 14 Changed 12 years ago by
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.
comment:13 Changed 12 years ago by
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?
comment:14 Changed 12 years ago by
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 theHTTPInternalError
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.
comment:15 follow-up: 16 Changed 12 years ago by
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
.
comment:16 follow-up: 17 Changed 12 years ago by
Replying to rjollos:
You see, I changed the error message to append
[TracVote]
, so I can be sure that error is actually coming fromTracVote.process_request
and not from the Trac code that handles the form token. I was surprised that the error was anHTTPInternalError
and not aTracError
.
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 theTracError
.
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.
comment:17 Changed 12 years ago by
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 theHTTPInternalError
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.
comment:18 Changed 12 years ago by
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. ;-)
comment:19 follow-up: 20 Changed 12 years ago by
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.
comment:20 Changed 12 years ago by
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.
comment:21 Changed 11 years ago by
(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.
comment:22 Changed 11 years ago by
(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
.
comment:23 Changed 11 years ago by
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.
comment:24 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
I'm not sure I understand the issue well enough to fix it immediately, so a patch would be much appreciated!