51 comments so far

  1. MERLiiN on

    I suspect that you have no formal training in database design, your id0 auto_increment field does not belong in your table design as you use login name as you primary key. The id0 field it entirely obsolete. I guess you could call it obscurity, but it’s hardly worthy of the name.
    Furthermore your script is vulnerable to SQL injection leading to authentication bypass or worse, database enumeration and theft. You seem to store passwords in plaintext, yet care enought to check an md5 hash of the username as opposed to the password, although the username is also stored in plaintext as well. It also seems that you forgot to mention that you would need to set some authenticated token and check for it’s presence in all the protected documents, otherwise knowing the path to private.php would completely bypass the login in the first place.

    Please don’t take this as a personal attack, it is not. I just wish to point out things you appear to be aware of, but have neglected in your article. I often find that developers with no real understanding of certain problems read blog entries like this looking for their “silver bullet” approach that will make their problems go away. Giving them incomplete or faulty information will hurt them, not help them. I am sure your article made perfect sense to you and most experienced developers will read between the lines and complete the missing items themselves. However you state that this is to help “rookie” developers, the very same ones that are unlikely to realize that they are implementing a flawed desgin due to your incomplete instructions. Please take more time when writing these kind of articles, preferably ask someone else to read over your article and provide critiscism. I know it can be hard to hear, but your articles will be better for it and your readers will love you more for it in the end.

    Best regards,
    MERLiiN

  2. dblackshell on

    MERLiiN apparently you misunderstood the code.

    1. i use id0 not for the login process but for the session junkie, because where I wrote
    //mark as valid user
    I use a session starting/storing of the id0 through another query… and for other uses like deleting a user, modifying data, etc.

    2. it is not vulnerable to sql injection, check it on a webserver and you’ll, that is if you don’t realize the md5($username) md5($password)

    3. I don’t store the password in clear text, if you notice char(32) the length of a md5 hash. and also the comparison md5(l0gn4m3)

    4. i did forgot to mention about the authentification token which as I already mentioned is stored in a session variable…

    and no I did not find you comment as a personal attack… cheers ;)

    and be attentive to the code better next time…

  3. Paulius on

    To whoever wrote this script… Yeah, I’m sorry, it’s pretty bad. I was googling to find a bunch of slightly more secure PHP login scripts (the ones shown in tutorials all around the web are absolutely and completely pathetic).

    I agree with what Merlin said, and you still don’t understand what he tried to imply.

    It seems as you don’t even have the slightest clue about what a MySQL injection is. Using an MD5 hash as a password doesn’t do anything to make it immune from an injection. An MD5 is useful if your database has been stolen by a malicious party and to protect them from seeing your user’s passwords.

    And no, this is not a personal attack but I would really appreciate if you wouldn’t advertise this as the “script that will solve all your troubles once and for all”.

    Have a good day.

  4. dblackshell on

    Paulius and all those who don’t understand the script and quote stuff like “script that will solve all your troubles once and for all”, which I didn’t say… do me a favor…

    shoot yourself… this is starting to get really old, really fast…

    for fucking once try to check your opinion… it can’t be sql injected…

    i hashed the username and password Paulius, how the fuck do you put a sql injection in a hash value?

  5. MERLiiN on

    It’s not quite beating a horse with a dead fish yet, but we’ll get there;

    It CAN be sql injected, you are inserting variables directly in the sql string with no filtering. The injection does not occur in the hash. I fail to understand why you are reluctant to learn what sql injection is or how it works, but I will make an attempt at explaining it to you. As usual the example will be simplified and intended to only highlight one of the many aspects of SQL injection, so please keep the trolling to a minimum….

    Your code states:
    $username = md5($_POST[”username”]);
    …snip…
    $query = “SELECT r34ln4m3 FROM 1nside0ut WHERE
    md5(l0gn4m3)=$username AND entryw41=$passwd”;

    Assume I fill in the following username:
    ” or 1 = 1 #–
    I could fill in whatever I wished for password, it does not even matter…

    Your code comes along and “inserts” the value of the vairables into your query.. this makes $query = “SELECT r34ln4m3 FROM 1nside0ut WHERE
    md5(l0gn4m3)=” or 1 = 1 #– AND entryw41=$passwd”

    Your SQL will match any line where the md5 of logname is ” or any line where 1=1 (which is always true in case you didn’t realize). it then hits # and — which are SQL comment indicators making the SQL server ignore the rest of your statement.

    A few ways to help you understand why you are wrong
    - you are calling md5 on a table row, not any user data
    - you are using SQL md5 operations leaving you exposed to SQL injections within your md5 statement anyway
    - You don’t filter the input

    Now you might have tested this yourself and found that you cannot insert the apostrophe’s in your tests. If so it is likely due to your web hosts php configuration, by settings such as magic_quotes_gpc helping to protect against bad practises. This does however not prevent SQL injection, or your host or others, but rather limits the vectors that can be used to achieve sql injection.

    Again, it is not a personal attack, but you seem to have taken it that way, responding with a teenage tantrum. I don’t have an issue with you being wrong, we’re all entitled to our mistakes and I myself have made my share of them. The thing here is that you can learn from your mistakes and correct your script. That would be the real benefit here. Both to you and future visitors to your blog. Yet you seem insistent upon remaining ignorant and leading other people down the same path. We already have one Steve Gibson, please don’t turn yourself into another one.

    Best regards,
    MERLiiN

  6. dblackshell on

    You just don’t want to understand… do you MERliiN…. ok so some pseudo-php-code to explain it.

    $username = md5($_POST[”username”]);

    //so $username = 34bf72…[32 chars in total aka md5 hash of the submited username]

    //with password the same whe get a md5 hash

    //the query
    //replacing $username
    $query = “SELECT * FROM whatever WHERE md5(l0gn4m3)=34bf72..”

    if you still don’t understand… copy-paste the fucking script and test it… you are really starting to piss me off… if you don’t understand the code…. enough said peace out… eof!

  7. dblackshell on

    and almost forgot… the user data is md5-ed already and I md5 the table column because it will have to match the username… just as a p.s. if somebody is wrong and you are right (but he insists I’m wrong…) what would you expect me to be ……. oh well..

  8. Tapan Bhanot on

    Hi,

    I don’t understand why people failed to understand such simple code ?! The author has already done filtering by md5ing the inputs.

    So as Merlin says: If he enters ” or 1 = 1 #–
    then it can do bad things, but it can’t coz author is md5ing the input in the start:

    So Merlin’s input (” or 1 = 1 #–) will become a 32 char string which won’t be ” or 1 = 1 #– so your injection will fail.

    Correct me if I am wrong ?!

    Thanks.

  9. chrisbabar on

    Hey,
    Like Merlin, at first I thought ‘hello sql injection’, but decided to RE-READ the code and realised that this script cannot be sql injected.

    Read these lines carefully:
    $username = md5($_POST[”username”]);
    $passwd = md5($_POST[”passwd”]);

    I actually think this is quite a clever script, although I would add an extra check on the number of rows returned – should only return 1. At the moment it just makes sure it’s not zero.
    Regards

  10. Alex Tokar on

    Nice script, I don’t think SQL injection is possible in there.

  11. dblackshell on

    yes chrisbabar it checks that the return of the sql querry isn’t 0 which I think it’s enough… could check also as you mentioned it it’s 1 but no need for that couz no mysql query error could intervene…

    actually there could be such a posibility… but than the whole script wouldn’t work… if the mysql server where down…

  12. ruffcrib2000 on

    This is a real good script. Appreciate you put it on the net.

  13. MiST on

    And what if the hacker just enters the address http://www.somedomain.somextension/somefolders/private.php?

    This is not safe at all!

  14. dblackshell on

    MiST… it’s just a login script… you’re a bit off the ideea… if you watch closely there is a code comment somewhere in there

    //mark as valid user

    where you “mark” the user as valid, via session/cookie who cares… which later you can validate in private.php for example…

  15. newtoid on

    works for me man.

    one of the sites i look after was recently attacked with mysql injection and i have been looking for a decent script for a new login area, cheers dude.

  16. weazzle on

    As far as I can tell, this is a pretty good setup. I suggest using a password hashing algorithm based in sha1 rather than just an md5, otherwise brute force could be used to retrieve the password from a compromised database.

  17. dblackshell on

    may be weazzle, but I’m more of md5 fan… my first hashing love… :D

  18. weazzle on

    The reason I suggest sha1 is that it is much less likely to result in hash collisions. Or more accurately, it is statistically less likely to run into them than md5.

  19. James on

    dblackshell, would you be willing to post an example of this script? From your own standpoint, I’d like to try injecting as mentioned above. That should prove either point fairly easily.

  20. dblackshell on

    the script is right in the post… and I’m very curios what injection you would pull off ;)

  21. [...] login script, validation | If some of you remember I wrote a long time ago an article about secure login script, and entered a comment in the php code => mark as valid… well this is a tiny article on [...]

  22. Ticonetster on

    It seems like this should work pretty good. I was wondering if using a key for encrypting the username and password is less secured than that md5 that u talk about…
    like

    $encryptme = $_POST["username"];
    $useme = “auniquekey”;
    $result = encrypt($encryptme, $useme);

    Don’t hate we are all trying to learn :)

  23. dblackshell on

    is as secure as md5 until nobody knows the key… that’s why I use hashing algorithms, so no one could reverse it (theoretically)… of course brute forcing would help… etc…

    but as said before using encrypt would be as safe as md5… just don’t know why you would want to use encrypt… :-?

  24. zplits on

    hello there dblackshell, good day.
    I’m curious when you wrote, “but this is less useful for a server that has MySQL 5 or above, see why MySQL Information Schema table.” May i know why?

    And one more thing, what about if the admin user wants to create a new user? what code will it be to store it into the database?

  25. dblackshell on

    because in mysql 5 and above there is the information_schema database in which are stored table names with column names and everything… so obscurity won’t add any extra help in there…

    adding a new user (sql command)

    INSERT INTO 1nside0ut(l0gn4m3, entryw41, r34ln4m3) VALUES($userlogin, $password, $username)

    where

    $userlogin – secret login name
    $password – dah
    $username – name to display

    also many would find useful to add an email column for reminders, password recovery, etc…

  26. Shane Houstein on

    To Merlin and all you other wankers who bagged out this script, you were wrong and you made yourselves look like fuckwits!

    Saying shit like “you have no formal database training”… fucking get over yourselves!

    IF you have had formal training, Merlin, it obviously hasn’t helped you… not one bit!

    As for the script: very well done! :)

  27. Matt on

    so much arguing wow :) I think some people need to start learning again :)

  28. Graham on

    Sho, thanks for the script – glad no one else has ragged you about sql injections :) cheers

  29. John on

    Just want to say I think this is very clever, but add a quick note. Although it is statistically not probable, it is possible for two users to end up with colliding md5 pairs. I’d like to propose that those who use this supplement it by still checking the regular username (real_escape’d of course). i’d struggle to find a reason why this added “necessity” doesn’t negate the need for the hashed username in the first place

  30. John on

    and thanks for posting this, its provided me with quite a bit to think about ;)

    cheers
    –john

  31. dblackshell on

    the collision should be checked upon registration…

  32. bastitch on

    hey dblackshell, when I try to use this code I get this error in mysql:
    #1075 – Incorrect table definition; there can be only one auto column and it must be defined as a key

    what am i doing wrong?

  33. Jack Read on

    Hey,

    Great little script! Really proving to be useful to me.

    I have a question to do with this bit of code:

    $result = mysql_query($query, $handle);
    $list = mysql_result($result, 0);

    if (mysql_num_rows($list)!=0) {

    I keep getting an error thrown at me saying “mysql_num_rows(): supplied argument is not a valid MySQL result resource”. Any ideas why that would be?

  34. dblackshell on

    @bastitch
    I repaired the problem in the script (by accident I set l0gn4m3 as primary key but) id0 should have been set primary key… now you should be able to create the table…

    @Jack Read
    edited the script and now should not give any errors.. the problem occurred because in a prior version I used mysql_result to extract the value… thus list would have contained the actual value (row) not the query resource as it should have… just copy the new variant of the script and should work just fine

  35. John on

    @dblackshell

    it might seem weird to tell someone they can’t have a specific username/password, not cause its taken, but because the md5 of it conflicts with what someone else chose

    just a thought..

    • weizenspreu on

      @John: You could just… uhm… lie and pretend that the chosen username is already taken. How should the user find out that it just colides with the MD5 of another user?

  36. dblackshell on

    @John

    ^^ you are right, but I never said this is a script to implement in apps, was just a small idea that I liked… funny enough I never though about that :D

  37. Todd on

    Terrible example

  38. wewatchyourwebsite on

    Nice strategy using the MD5. I must admit my first thought was the SQL injection as well, but rather than blasting you, I looked at the code again and realized you’re right.

    Congratulations! (not on being right, but on providing many of us with some useful ideas)

  39. Kevin on

    Great script.
    On the human side of things.

    Merlin was definitely too confident he read the code right.
    That is fine, people do that sometimes. But…

    Its fine if people love pointing out mistakes of others. What really annoys me are people who do that, and at the same time try too hard to portray themselves to be high & mighty, overly-matured, articulate, trying to imply that dblackshells’ article is all not as good as it actually is and even treads on issues such as lack of general knowledge, but tries to cover their obnoxious attitude (with much failure of course) with an overdone dose of being “understanding” by giving a long lecture on learning from ones mistakes and telling them how they should post.

    Bloody nag. And I’m bloody sure he took several hours tweaking his comments to sound articulate and wise before posting it, way more time than what was spent reading the code.

    “…I suspect that you have no formal training in database design…”

    “…Yet you seem insistent upon remaining ignorant and leading other people down the same path. We already have one Steve Gibson, please don’t turn yourself into another one….”

    “…It’s not quite beating a horse with a dead fish yet, but we’ll get there;…”

    “…I fail to understand why you are reluctant to learn what sql injection is or how it works, but I will make an attempt at explaining it to you…”

  40. Kevin on

    I seriously think all that Merlin said was downright insulting.

    “…Again, it is not a personal attack, but you seem to have taken it that way, responding with a teenage tantrum. …”

    lol. Seriously my merlin friend. You seriously have a problem with the way your write, and I find yourself to be self contradictory. Just get straight to the fking point and stop the long lecture, did you know its a fact that people are less likely simple because over-nagging causes people to turn away from you? I’d be pissed off at you, if you were first to comment on my post acting all self-righteous.

    Praises to you dblackshell for taking such crap without a sweat. I can tell by the way you write that you didn’t flinch.

    What a stressful day at work. I feel better now :)

  41. DRide on

    Nice idea, I currently just hash the password, but will start to hash both from now on. I can not see how this can be injected i may be wrong.
    Even if it could be injected Merlins’ method would not do it. give Merlin his dues it would be hard to read the script with his head so far up his own back side, Good job

  42. Alexwebmaster on

    Hello webmaster
    I would like to share with you a link to your site
    write me here preonrelt@mail.ru

  43. Kai Sellgren on

    I hope everyone understands how insecure code this page gives you.

  44. rimau on

    Hi All,

    Im wondering, when creted a user and password, should I md5 the _POST the user & passwd?

  45. Justin on

    Hey all,

    Im still a PHP/MySQL noob and only just recently found out my original script that I based my website on was infact the worse written code ever known to man! So now I need to bloody re-code the whole entire lot and basically re-learn how to code the proper way. Im becoming more and more security minded and so this is interesting. Thanks for the blog.

    With regards to the comments made about the fact that someone may not be able to check if a username is taken or not as the MD5 might clash ….

    Would you be able to set up 2 indentical username fields in the database (username1, username2) and then only md5 one of them and use the second to check the usernames? These would both have to be indentical in any changes or checks made to them but surely it would be a way round the checking?

    I know this would take away the fact that hackers may be able to find the actual username itself with ease but would still add that md5 security feature to the script for when they try and hack????

  46. HackWizard on

    I just hacked it quite easily

  47. Kevin on

    RE: HackWizard

    If you hacked it that easily…why not tell us all how?

  48. gad on

    the code is pretty lame, where you add the security?

  49. Sarah on

    To the ones complaining about SQL injection:
    It is *obvious* that SQL injection is impossible in this example, and therefore it is not *required* to escape the username and password before executing the query.
    Let’s assume he username is “usr” and the password is “pwd”.
    The code would then be:

    $username = md5(”usr”); // = 0a744893951e0d1706ff74a7afccf561
    $passwd = md5(”pwd”); // = 9003d1df22eb4d3820015070385194c8

    $query = “SELECT r34ln4m3 FROM 1nside0ut WHERE
    md5(l0gn4m3)=’0a744893951e0d1706ff74a7afccf561′ AND entryw41=’9003d1df22eb4d3820015070385194c8′”;

    ——
    Now, let’s try to inject some SQL code here:

    $username = md5(”usr”); // = 0a744893951e0d1706ff74a7afccf561
    $passwd = md5(”‘ OR 1 = 1 #”); // = ee87d6a9c00b5f1a06d8024724465299

    $query = “SELECT r34ln4m3 FROM 1nside0ut WHERE
    md5(l0gn4m3)=’0a744893951e0d1706ff74a7afccf561′ AND entryw41=’ee87d6a9c00b5f1a06d8024724465299′”;
    ——

    How is that vulnerable to SQL injection?
    If you (general “you”) can’t understand that, I seriously doubt your capability to think logically.

  50. stan on

    @Sarah

    I believe that point was already pointed out a year ago and by the 10 posts prior to yours


Leave a reply