4 ms·

Thank you for looking over the code and providing your honest criticism. * the textfile doesn't seem like a problem to me, just another version of a DB, what w

by nanch 14y ago

Thank you for looking over the code and providing your honest criticism. * the textfile doesn't seem like a problem to me, just another version of a DB, what would be better? * I updated the password to accept any password, not just alphanumerics * I updated submit.php to use a random 16-character salt rather than the static salt I'll look over the site to see if I can find those typos ;)

Cyranix 14y ago
It doesn't seem to me that you're storing the per-user salt anywhere. I'm glad you're working on a project that is fun for you. It will be an opportunity for you to learn quite a lot, but I would urge you to question your design choices and keep asking "How could this be done better?". Read about how other people have approached similar problems. Get a small group of friends, family, and other technical acquaintances to pick this apart. I wouldn't endorse this for the general public.
- nanch 14y ago
  The salt gets stored with the password hash in 'userstocreate.txt' so a line in userstocreate.txt looks something like: username:$6$salt$passwordhash$:email Then the password is sent to the /etc/passwd (/etc/shadow) file by /usr/sbin/useradd in createusers.pl.
  - Cyranix 14y ago
    Now that I'm waking up some more, I realize 1) you're correct, the salt is stored, but 2) you're using SHA-512 for password hashing, which has been repeatedly discussed as a bad idea for quite a while on HN. This is the last I'll say on it: I am genuinely glad that you're trying out a project of this complexity, but you have a responsibility not to give others a false sense of security. If you search HN, you will find plenty of advice and reading material -- restrict this app until you have absorbed more of it. It's not enough that it works; strive to make it work well. [Also, a nitpick just because it's killing me: please replace the entirety of isStringAllowed with a regular expression. \w{5,} would give you alphanumerics plus underscore and check for minimum length. Regexes don't solve every string-oriented problem, but they're a fantastic tool to have in your toolbelt.]
    - rgbrgb 14y ago
      I disagree, congrats on releasing something! I think it is entirely appropriate to make it public and solicit feedback here. He has already gotten some useful stuff. Perhaps you could point him to "advice and reading material" that would be fruitful for him to "absorb" before he makes another gaff like getting on the front page of HN?
  - jemfinch 14y ago
    We don't need yet another scheme for managing passwords. If you can, use something like PHP's new password API: https://wiki.php.net/rfc/password_hash https://wiki.php.net/rfc/password_hash
viraptor 14y ago
> the textfile doesn't seem like a problem to me, just another version of a DB, what would be better? It's not atomic at all. You could at least use bdb, or sqlite.
- nanch 14y ago
  great point! I'll fix this; probably by adding some file-based locking for simplicity's sake.

by nanch 14y ago

Cyranix 14y ago

It doesn't seem to me that you're storing the per-user salt anywhere. I'm glad you're working on a project that is fun for you. It will be an opportunity for you to learn quite a lot, but I would urge you to question your design choices and keep asking "How could this be done better?". Read about how other people have approached similar problems. Get a small group of friends, family, and other technical acquaintances to pick this apart. I wouldn't endorse this for the general public.

nanch 14y ago
The salt gets stored with the password hash in 'userstocreate.txt' so a line in userstocreate.txt looks something like: username:$6$salt$passwordhash$:email Then the password is sent to the /etc/passwd (/etc/shadow) file by /usr/sbin/useradd in createusers.pl.
- Cyranix 14y ago
  Now that I'm waking up some more, I realize 1) you're correct, the salt is stored, but 2) you're using SHA-512 for password hashing, which has been repeatedly discussed as a bad idea for quite a while on HN. This is the last I'll say on it: I am genuinely glad that you're trying out a project of this complexity, but you have a responsibility not to give others a false sense of security. If you search HN, you will find plenty of advice and reading material -- restrict this app until you have absorbed more of it. It's not enough that it works; strive to make it work well. [Also, a nitpick just because it's killing me: please replace the entirety of isStringAllowed with a regular expression. \w{5,} would give you alphanumerics plus underscore and check for minimum length. Regexes don't solve every string-oriented problem, but they're a fantastic tool to have in your toolbelt.]
  - rgbrgb 14y ago
    I disagree, congrats on releasing something! I think it is entirely appropriate to make it public and solicit feedback here. He has already gotten some useful stuff. Perhaps you could point him to "advice and reading material" that would be fruitful for him to "absorb" before he makes another gaff like getting on the front page of HN?
- jemfinch 14y ago
  We don't need yet another scheme for managing passwords. If you can, use something like PHP's new password API: https://wiki.php.net/rfc/password_hash https://wiki.php.net/rfc/password_hash

viraptor 14y ago

> the textfile doesn't seem like a problem to me, just another version of a DB, what would be better? It's not atomic at all. You could at least use bdb, or sqlite.

nanch 14y ago
great point! I'll fix this; probably by adding some file-based locking for simplicity's sake.