Security issues

[gregmolnar]

hi Fernando,

I really like the idea behind this app but it has huge security holes. I will send you the admin username and password in email to prove my point. I am also happy to let you know where to fix the issues I found or I can even send a PR.

[deleteman]

Hey @gregmolnar , if you could send a PR with the fixes, it would be awesome, otherwise I can take care of it. I just don't know when I'll be able to tackle it, I'm having very little free time lately.

Either way, thanks for pointing this out!

[mpscholten]

Currently some design-aspects of the application just cry for security holes.
For example: The model helpers provides methods like load_developer_where which doesn't escape by default, therefore we need to escape the parameters passed to this method by hand. But sometimes we forget to escape things and then our app is on fire :) We should fix this by enabling escaping by default in the helpers itself.

[deleteman]

Wait, I'm not escaping inside the helper functions, because I'm escaping all request parameters on the main controller class: https://github.com/deleteman/lfpr/blob/master/core/MakiaveloControllerClass.php#L11-L13
You don't think that's enough @mpscholten ? I know @gregmolnar told me that I should not be using mysql_real_escape_string on everything, but maybe we just need to fix that? Or is the security hole bigger than that?

[gregmolnar]

@mpscholten is right, the way you handle the queries are not ideal. It would be better to handle the escaping at a central place and just use a find_by_id find_by_params method. In the first one you handle escaping for integers, in the second one for any string. But the best would be to use prepared statements and PDO.
Another problem is the way you store the passwords. They are in plain text so if a malicious user gets to the database somehow he got all the passwords right away. If you would hash them, it would be more difficult to reverse them.

[mpscholten]

Yes it's not enough to escape all the request params because sometimes we query the database with data which is not based on the request, e.g. https://github.com/deleteman/lfpr/blob/master/app/controllers/ProjectController.php#L124 <- here we can inject into the query by creating a project with a bad name.

@gregmolnar is right with using PDO and prepared statements. That would be the right way :)

[gregmolnar]

Another issue is the way you output the project description. It is coming from the user and isn't htmlescaped, which makes it possible to use a stored XSS and steal someone else's session for instance.

[mpscholten]

I suggest using a template engine which escapes by default, like twig. This would protect against any kind off xss in the project. What's your opinion on this @deleteman @gregmolnar? :)

[gregmolnar]

I don't know twig but the idea sounds good.

On 21 June 2014 20:55, Marc Scholten notifications@github.com wrote:

I suggest using a template engine which escapes by default, like twig.
This would protect against any kind off xss in the project. What's your
opinion on this @deleteman https://github.com/deleteman @gregmolnar
https://github.com/gregmolnar? :)

—
Reply to this email directly or view it on GitHub
#47 (comment).

[deleteman]

@gregmolnar , @mpscholten guys! These are both greats suggestions, using PDO instead of the current storage layers has been part of my to-do list since the begining, but I never did get around to it.
As for the template engine, that's an awesome idea. I think the only one I ever used for PHP was smarty like 5+ years ago, so I'm a bit outdated in that aspect, and Makiavelo (the webframework I'm using for LFPR) might not make things easy for us... we might need to do some tests.

What do you guys think? Does either one of you have enough time to grab any of these tasks?

[mpscholten]

Count me in. Let's fix the sql stuff first, by either using plain pdo or using a modern orm like propel, doctrine or eloquent. I'd suggest to don't reinvent the wheel and to stick with one of the already existing orm's. So plain pdo or an orm, what do you think?

[deleteman]

Glad to have you on-board @mpscholten ! I would go with plain pdo, that way we can just edit the sql helper functions and keep the rest working pretty much the same (as long as that's possible). what do you think?

[gregmolnar]

I am not working with PHP anymore and trying to stay away from it to be honest. But I am happy to review the changes from a security point of view when they are done.

[mpscholten]

just edit the sql helper functions

I wouldn't do it this way because with classes we can declare much cleaner interfaces. For example all our save_{model} model require implicit a global state, the database connection. With object we can make these dependencies explicit and therefore gain much easier code.

Just hacked together this small gist: https://gist.github.com/mpscholten/b394669a8ced094404cd My plan would be to move all the methods (in this case: https://github.com/deleteman/lfpr/blob/master/app/sql/helpers/Developer.php) into Repository classes. It's basically an implementation of the data mapper pattern :) With some generic methods we can keep the amount of code at the minimum. Do you think we can do this or do you want to keep the current way of querying the database?

---

I am not working with PHP anymore and trying to stay away from it to be honest.

Can understand this, php is just ... ugly. ruby is, in my opinion, a much nicer language.

[deleteman]

@mpscholten I like your approach, although I would change the methods that write the queries, to dinamically pull the attributes from the entity being saved, this way when a migration add or removes attributes, you don't have to go into the repository class and update the queries...(which is something you have to do now with the current sql helpers [I was lazy at the moment and didn't add this feature :P]).
What do you think about that?

  Can understand this, php is just ... ugly. ruby is, in my opinion, a much nicer language.

So can I, to be honest, I used PHP because I knew back then that finding a hosting that would let me run Rails 3.x was going to be difficult..