8
\$\begingroup\$

I wrote a script that one way encrypts a user's password by generating a key, and multiplies by the ASCII value of the character and the ASCII value of the key character at (the position that the character has in the password string) and repeats this for the previous positions in the string meaning a key string with ASCII values of:

49, 50, 51, 52, 53, 54, 55, 56, 57, 48

and a password string of

112, 97, 115, 115, 119, 111, 114, 100, 46, 49

Would be multiplied like:

[(112 * 49)] + [(97 * 50) + (97 * 49)] + ...

and so on.

As an example, the string password.1 encrypted with the key 1234567890 (auto generated) would return 203456.

$pass = $_GET["pass"];
$key = $_GET['key'];
$newPass = go($pass, $key);
echo $newPass;
function go($pass, $key){
    $passArray = str_split($pass);
    $keyArray = str_split($key);

    $total = 0;
    $pos = 0;
    foreach ($passArray as $char){
        $posInitial = $pos;
        while($pos !== 0){
            $total = $total + (ord($keyArray[$pos]) * ord($char));
            $pos--;
        }
        $pos = $posInitial + 1;
     }
     return $total;
}
function authorise($pass, $passEncode, $key){
    $passEncodeTest = go($pass, $key);
    if ($passEncode == $passEncodeTest){
        return true;
    } else {
        return false;
    }
}
authorise('password.2', $newPass, $key); ==> returns false
authorise('password.1', $newPass, $key); ==> returns true

(I will add a password sanitiser in later, but for now, plaintext)

My question is, what can I do to improve this code?

\$\endgroup\$
  • 4
    \$\begingroup\$ I would argue that if you're not an expert at security to use an already existing and thoroughly tested password hashing library, or PHP's native password_hash() function. \$\endgroup\$ – Kid Diamond Apr 17 '15 at 9:10
  • \$\begingroup\$ In security terminology, authorise is used to grant/deny permissions. Your method should be renamed to authenticate. \$\endgroup\$ – dfhwze Jun 22 at 11:19
10
\$\begingroup\$

Reinventing the wheel

Please note that encryption should, in principle, always be two way. A 'one way encryption' just sounds weird. Normally something is encrypted with the intention to decrypt it at a later stage. This is not what you do. What you do is called hashing or perhaps 'encoding' as you indicated in your title.

You could improve the formatting of your code. I really miss the empty lines before and after the functions, and a space before { in a function. Your code is also not working because of the ==> in it at the end. This should be // ==>. Is is just nice if your code is working like published here.

The naming of your variables seems somewhat sloppy. In the beginning you call an encoded password $newPass. Is it a new password? I don't think so. Then in the authorise() function you rename it correctly to $passEncode, by which you probably mean $passEncoded.

So the big question is, why do you define your own hashing routine? Is it any better than the existing routines? PHP offers many of them!

See this link to tutsplus which addresses some issues that are encountered in hashing passwords. Is your routine resistent to all these issues? I don't think so.

It is far better to use a good existing password hashing library that is present in PHP. Have a look here: http://php.net/manual/en/faq.passwords.php

There's a lot of information about hashing and PHP out there. Find it and use it. I do really support new and inventive ways to solve problems but in this case it is definately not a good idea to reinvent the wheel.

\$\endgroup\$
  • \$\begingroup\$ Isn't encoding two-way as well? An encoder without decoder also sounds weird. \$\endgroup\$ – dfhwze Jun 22 at 11:16
  • \$\begingroup\$ @dfhwze That's why I said "perhaps". Some encoders are selecting information from the original data, and in those cases the original cannot be restore by simply decoding it. Take a photo camera as an example. There's a full spectrum 3D scene which is encoded as bitmap in the camera's memory. You cannot decode it back to the original 3D scene. This happens in pure computing as well. Take the lossy JPEG encoding format, for instance. Hashing is an extreme example of this type of encoding. On the other hand many hackers seem to succeed at decoding hashes. 🤔 \$\endgroup\$ – KIKO Software Jun 22 at 11:48
  • \$\begingroup\$ I understand encoding and decoding aren't necessarely bijective (no quality loss in decoding an encoded value), but still they are two-way. Anyway, +1 for mentioning existing APIs. \$\endgroup\$ – dfhwze Jun 22 at 11:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.