search

davarravad / UserApplePie

UserApplePie website portal based on UserCake. UserApplePie is a fully open source user management system. UAP v.2 Build Started - Check that Repo.
http://www.userapplepie.com
GNU General Public License v2.0
1 stars 1 forks source link

SQL Injection #31

Open ddmler opened 8 years ago

ddmler commented 8 years ago

In: UserApplePie/pages/message/msend.php You missed to check "mto" and "mfrom" If you thought that it is not possible to extract data in an insert into statement, that's wrong. Also a client-side validation of the usernames is not enough. (we can change that so easily with chrome dev tools)

An example for more clarity:

  1. Go to the "send new message" page and open chrome dev tools
  2. search for the hidden input field containing the own username mfrom (it works with the mto field too) Change the value of the hidden input field for example "ddmler" into something like this: ddmler', msubject = (select password from uap_users where user_name='ThisUserWillBeSad' LIMIT 1)# And you have in your Outbox a nice new message which subject string is the password of any user you'd like (hash of course). This takes like 10 seconds with copy and paste

The golden Rule is: if you have anything in your query, that was on the users pc before (get, post, cookie, ...) use (non emulated) prepared statements. Addslashes is not secure and even real_escape_string is not 100% secure (in some strange character encodings, that probably no sane person would use, there are sql injections possible)

davarravad commented 8 years ago

Agreed. I have a few other places where I need to update this style of protection. I have decided to go ahead and start using pdo prepared statements. The messages system needs to be overhauled completely. I am sure there is a much better way to handle them.

This is on my to do list. Should have some time to make a bunch of updates over the coming holiday break. :-)

davarravad commented 8 years ago

The messages system has been updated to PDO Prepared Statements! woo!

Thanks again!

ddmler commented 8 years ago

Great the bug is gone, however it should still be possible to send messages in another name. https://github.com/davarravad/UserApplePie/blob/master/pages/message/msend.php#L25 If i know the userid of a valid user and send it via post this line will accept it and write it to the database. You should just ditch the hidden from field and since you check in line 14 for "isUserLoggedIn" you should be able to get the user id of the currently logged in user from somewhere and should use this. So something like: $mfrom = getCurrentUserId();

davarravad commented 8 years ago

Good catch. Fixed! Also updated how the email gets the from username. :+1: