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?
1 Answer 1
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.
-
\$\begingroup\$ Isn't encoding two-way as well? An encoder without decoder also sounds weird. \$\endgroup\$dfhwze– dfhwze2019年06月22日 11:16:59 +00:00Commented Jun 22, 2019 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– KIKO Software2019年06月22日 11:48:50 +00:00Commented Jun 22, 2019 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– dfhwze2019年06月22日 11:52:27 +00:00Commented Jun 22, 2019 at 11:52
Explore related questions
See similar questions with these tags.
password_hash()
function. \$\endgroup\$authorise
is used to grant/deny permissions. Your method should be renamed toauthenticate
. \$\endgroup\$