Subject: | I did not launched this application... |
Summary: | Package rating comment |
Messages: | 7 |
Author: | Artur Graniszewski |
Date: | 2011-02-15 09:12:14 |
Update: | 2011-02-17 12:31:16 |
|
|
|
Artur Graniszewski rated this package as follows:
Utility: | Sufficient |
Consistency: | Good |
Documentation: | Good |
Examples: | Good |
|
Artur Graniszewski - 2011-02-15 09:12:16
I did not launched this application to test it more thoroughly, but from looking at the code I see various security problems:
-----------------------------------------------------------------
1) There is a security hole that allows simple user to change password to any account, even administrator, look at: process_profile_edit():
$sql="update ".$this->prefix."user_creds set
email='".mysql_real_escape_string($_POST['email'])."',
pass='".md5($_POST['password'])."'
where userID=".$_POST['userID']."";
there is no mysql_real_escape_string in $_POST['userID']
2) There are potential SQL injection vulnerabilities in:
a) user_info($userID),
b) is_logged_in() (in both methods the userId in $_SESSION variable can be an unescaped SQL query planted during the user registration process),
c) delete_account($userID)
d) show_profile_edit_form($post_to, $userID=0)
3) You really should use PHP5 exceptions rather than exiting after throwing the error. In your implementation this becomes a major problem when another webdeveloper tries to wrap your code in his own template, in case of an error you will break his HTML footer (it will not show up).
Bob Kennedy - 2011-02-16 15:58:29 - In reply to message 1 from Artur Graniszewski
Thanks for your concern. As with all code you get from other people, I'd suggest not using it until you understand it completely. So I appreciate you bringing it to my attention.
> 1) There is a security hole that allows simple user to change password to any account, even administrator, look at: process_profile_edit()
That is desired behavior, and exists so that an administrator may make changes to other user's accounts. That is why the function verifies that the supplied userID either matches the current user's ID or that the current user is an admin. If that is not what the developer wants, they would not make use of that function.
> 2) There are potential SQL injection vulnerabilities in:
none of the listed methods make use of the database, except delete_account($userID)
> (in both methods the userId in $_SESSION variable can be an unescaped SQL query planted during the user registration process)
Please explain by what method by which you would get SQL in there during the registration process so that I can protect against it. The userID is set by the script when returned by the query as a newly inserted ID. Can you fake that?
> 3) You really should use PHP5 exceptions rather than exiting after throwing the error.
Thank you, I'll be fixing that in the next round of updates. Also, I'll be making sure everything is escaped or verified as the correct data type (is_numeric() for IDs, etc) before making use of them
Bob Kennedy - 2011-02-16 16:28:28 - In reply to message 1 from Artur Graniszewski
On the point about "Exceptions"...
Instead I stored the errors in a variable and returned false when one occurred, and created a function to recall the list of errors and clear the variable. Now devs can use them if they want, but they're non-blocking.
private set_error()
private get_error()
documentation will be updated shortly.
Artur Graniszewski - 2011-02-16 19:22:21 - In reply to message 2 from Bob Kennedy
> 2) There are potential SQL injection vulnerabilities in:
none of the listed methods make use of the database, except delete_account($userID)
They dont have to use database directly to become a SQL injection vulnerability. Just try to enter some bogus e-mail addresses during the registration process, like so: "1 or userId like '%'" (ignore the double quotes).
Now let's see theoretically what will happen:
a) process_registration() will insert "1 or userId like '%'" into mysql table (no SQL injection because of mysql_real_escape_string() in that method)
b) user is logging in using malicious e-mail address "1 or userId like '%'" in process_login(). There is still no SQL injection, but it's becoming very funny: this e-mail address is saved in the $_SESSION array.
c) admin is trying to delete this e-mail account using delete_account(), now starts the SQL injection:
$sql="delete from ".$this->prefix."users where userID=".$userID;
becomes:
$sql = "delete from users where userID=1 or userID like '%'"
This one deletes all records from the user table.
This was just an example. I don't have time to check it more thoroughly, but as you see SQL injections can be executed on every method, even those which don't operate directly on the database.
Artur Graniszewski - 2011-02-17 08:37:26 - In reply to message 4 from Artur Graniszewski
What's more when the malicious e-mail address has been saved in the $_SESSION, it can be used in the rest of your methods where userId is not escaped at all, but taken 'as is' from the $_SESSION variable.
Artur Graniszewski - 2011-02-17 10:40:12 - In reply to message 5 from Artur Graniszewski
Sorry, my mistake. I thought that userId in database contains an e-mail address, but now I see its an INT data type. There are no SQL injections in this.
Bob Kennedy - 2011-02-17 12:31:16 - In reply to message 6 from Artur Graniszewski
Interesting. Well, I appreciate your making me look more fully into my code anyways. Like I said, I will be more thoroughly escaping the db interaction code soon. Thank you.
|