Posted by Ben on 09/14/05 20:35

Gustav Wiberg wrote:

> All you guys, please comment if the code is well or bad written and
> why... :-)

Since you asked, a few things popped out from a security perspective,
though I didn't read through your code very thoroughly....


> <?php
>
> function chkIfPasswordTrue($un, $pw, $typeUser) {
>
> //Make username and password in-casesensitive
> //
> $un = strtolower($un);
>
> $pw = strtolower($pw);


Why limit your usernames/passwords to lower case? You've just made them
significantly easier to brute force.



>
> $sql = $sql . "SELECT IDAnvandare FROM tbanvandare WHERE";
>
> $sql = $sql . " Anvandarnamn=" . safeQuote($un) . " AND";
>
> $sql = $sql . " Losenord=" . safeQuote($pw) . " AND";


Where is your safeQuote() function coming from? From what I can see of
your code you aren't doing any testing against the username and password
before they are used as part of your SQL query. Sure would suck to have
an unauthenticated user drop or otherwise muck with your db!



> if (isset($_REQUEST["frmUsername"])) {
>
> $un = $_REQUEST["frmUsername"];

If you're going to use $_REQUEST you might as well just turn on register
globals (no, don't!).

If you're expecting a post look for a $_POST, if you're expecting a get
look for a $_GET. Ditto with cookies. You really need to know where
your variables are coming from if you want a measure of security.

- Ben

[Back to original message]