7
\$\begingroup\$

I'm working on a simple data storing program for my senior year (Secondary School / Grade 12), and there is a user sign-in functionality I've decided to implement. The way I've done this is by storing the usernames and passwords in a text file. They are stored as follows:

userName password
userName password
...

The usernames, I've decided to leave as plain text, as is decided by the user upon registration. The passwords however, I've decided to encrypt by inserting a special character ( '/' , '?' , ':', ...) between each of the characters entered by the user. This is simple to code as I've done:

// password is passed to the function as a string parameter
fstream writer;
writer.open("database.txt", ios::app);

int separators[16];

for (int i = 0; i < 16; i++)
{
    int randomi = rand() % (47 - 33 + 1) + 33;
    separators[i] = randomi;
}


for (int j = 0; j < password.length(); j++)
{
    int randomIndex = rand() % 6;
    writer << (int)password[j] << separators[randomIndex];
}

writer << "\n";

For the time being, I've decided to only use on of the special characters to figure out the decryption mechanism. So, using '/' as the special character, the following is my decryption code:

...
__int64 l=0;
...

for (int i = 0; i < a.length(); i = i + 2) //a is the password read from the file as a string.
{
    if (a[i] - 48 > 4)   //greater than 4 as the ASCII values of alphabets are above 50
    {
        l = l * 10;
        l = l + (a[i] - 48);  //yields the int value from the ASCII number.
        l = l * 10;
        l = l + (a[i + 1] - 48);
    }
}  

The above code, takes in the password from the file, and scans for the number 4 (explained in the comment above); if the current number is not 4, it stores its int value in a variable called l. (Pardon the insignificant naming please), and does the same for the next letter, before incrementing i by 2, as the special character's ASCII value is 2 numbers.

There are drawbacks of this, mentioned at the bottom.

The verification mechanism for this system is as:

...
__int64 dy=0;
...
for (int i = 0; i < c.length(); i++) //password passed to decrypt function as string
{
    dy = dy * 100;
    dy = dy + (toupper(c[i])); //toupper so that the alphabet's ASCII value is only 2 digits
}

if (dy == l) // `l` from the decryption mechanism
{
    cout << "VALID";
}
else
{
    cout << "INVALID";
}

This works on the basis that it takes every character's ASCII value and converts it into an int for the entire word.

Example:

User entered password : Mayur
Stored password (in file after encryption) : 77476547894785478247       //while using only the special character `/`
Password converted into `int` format : 7765898582 //each 2 digits corresponds to a character in the user entered password
Decrypted password : 7765898582
Passwords match! Valid!

Major drawbacks:

  1. At the moment, I have difficulties with decrypting for lower case alphabets due to triple digit ASCII values

  2. Password size small due to overflow conditions of the integer format the decryted password is stored to verify.

  3. At the moment, uses same special character to separate alphabets of actual password

I'd like you to pretty much to judge the system, and maybe if someone will post an estimate as to how hard this system would be to crack (given that the special characters will be random), and if there are more efficient ways to go about making this system more secure. (I might decided to encrypt usernames as well).

\$\endgroup\$
  • 1
    \$\begingroup\$ This is not in code review format, but you should not implement password based key derivation at all - consider following recommendations at owasp.org/index.php/Password_Storage_Cheat_Sheet Basically if this is for the goverment you should use PBKDF2 and scrypt if it's not as scrypt is more secure than PBKDF2 against GPU's attacks as I guess. \$\endgroup\$ – Boris Treukhov Oct 20 '14 at 17:03
  • 1
    \$\begingroup\$ Always store the password encrypted in some way (preferably by a one way encryption (so you can not unencrypted it)). Try and do an MD5 on the password; then store the MD5. When a user types the password get its MD5 and compare it to the stored MD5. If they are the same then you probably have the same password. Combine a user name and MD5 password then you will have a correct fit. Note Also look up what a salt is. Then store a per user salt that you generate randomly when they first create their username/password combination. PS. MD5 results in a 128 bit number (or 32 character hex string) \$\endgroup\$ – Martin York Oct 20 '14 at 19:25
  • \$\begingroup\$ @LokiAstari oclHashcat states that I can probe up to 93_000 _000_000 MD5 hashes PER SECOND on a single powerful specialized PC with several GPUs hashcat.net/oclhashcat - so MD5 + salt is not a panacea and MD5 is especially weak against brute force. \$\endgroup\$ – Boris Treukhov Oct 20 '14 at 20:53
  • 2
    \$\begingroup\$ @BorisTreukhov: No its not great. But a very easy example of a one way encryption function that most people will have heard of. For a school project it will be fine. For Gov/Corp work not even close. \$\endgroup\$ – Martin York Oct 20 '14 at 21:18
3
\$\begingroup\$

Let's do actual code review instead of attacking the cardinal mistake we've all made of trying to roll our own encryption (and learning how horrible naïve we were...)

writer.open("database.txt", ios::app);

This code is designed to work. That's bad, as it means there is no contingency for unexpected situations like when database.txt cannot be opened. If you don't plan for failure it has a way of hitting you when you don't expect it.

int separators[16];

This line is introducing a 'magic value', and a bug for later. Take these two lines...

for (int i = 0; i < 16; i++)

int randomIndex = rand() % 6;

One of those contains a bug. To guard against that you can define a 'constant' for the value you intend to use, and should you decide to change it then it's one change to affect all your code.

int randomi = rand() % (47 - 33 + 1) + 33;

There is no indication that before this code that rand() was ever 'seeded' this may actually wind up in the results being very predictable.

if (a[i] - 48 > 4)   //greater than 4 as the ASCII values of alphabets are above 50

At the risk of premature optimisation, is there a point to doing math on a[i] before comparing it? Well, no it's not premature optimisation in this case as it reveals the biggest weakness in the system... There is no point in even looking at the value contents since you know there will be a pattern of 2-digits meat followed by 2-digits salt. The random salt is just noise, it's mathematically and cryptographically irrelevant. Your 'encryption' boils down to some mild obfuscation at this point.

Think about the string 77476547894785478247 in your example. It's likely that someone hacking your system would be looking for text values in those strings and this 'encryption' presents a nice set of base-10 pairs to work with - it won't take a genius to deduce what the values mean.

To address some of the drawbacks, the ascii value issue could be resolved by converting the ascii values to Hexadecimal strings (base 16) instead of Decimal (base 10) for output. The limit of using an int is moot since you're not doing any math on it as an int - you can just work with bytes in your current system.

Moving forward you could just take standard well known and tested encryption practices using libraries which you can almost copy/paste from the examples. To learn a bit more about encryption you could stretch yourself instead and implement a classic encryption system (such as the WW-II Enigma which would be simple enough) then look at ways to expand it or adapt it for your project. Could you expand the key space? Make it a one-way function more suitable for password checking?

\$\endgroup\$
11
\$\begingroup\$

What I'd like from you guys, is pretty much to judge the system, and maybe if someone will, post an estimate as to how hard this system would be to crack

For a professional cryptographer (which I am not), it would be very easy to crack, due to the following factors:

  • the system relies on rand (rand is not really safe for cryptographic code; consider using a mersene twister, like mt19937).
  • the algorithm has low entropy, meaning it is easy to brute-force it, using a password dictionary.
  • considering your decryption algorithm is deducing the key from the data itself, a hacker could do the same.

There are probably other factors making the code easy to crack, but as I said, I am not a crptographer.

(given that the special characters will be random), and if there are more efficient ways to go about making this system more secure. (I might decided to encrypt usernames as well).

You are looking at a problem with a "yes/no" answer (i.e. you need to check that the data is the same, not use decrpyted data in more complex computations), you should consider a one-way hash.

The idea is to construct an ireversible function, (irreversible = "given x, f(x) is easy to compute, but given f(x) it is difficult/impossible to compute x").

Using a one-way function in your case:

To encode the password, you apply the function.

To check the password, you ask for user input, you apply the function on the input, then compare the result with the already computed value (the one you need to verify).

This will (theoretically) allow you to make an algorithm that is not required to be reversible for checking the data.

As a rule of thumb though, security code is notoriously hard to get right. Consider:

  • using third party solutions (like CryptoPP or OpenSSL)
  • using known solutions (OpenSSL, PGP/GPG)
  • implementing known cryptographic algorithms (look up symetric encryption, asymetric encryption, commercial hash functions and algorithms)
\$\endgroup\$
  • 2
    \$\begingroup\$ additionally to hashing passwords it is considered good practice to store a user-specific salt alongside the password, to make rainbow-table attacks even more difficult. \$\endgroup\$ – Vogel612 Oct 20 '14 at 14:53
  • 3
    \$\begingroup\$ Essentially, spend a few hours reading security.se, clicking through links from blog posts like Jeff Atwood on hashing, and when you're suitably terrified and depressed, KUDOS, you are ready to do security. \$\endgroup\$ – Yann Oct 20 '14 at 15:13
  • \$\begingroup\$ mt19937 is also not suitable for cryptographic purposes. \$\endgroup\$ – Yuushi Oct 21 '14 at 10:50
6
\$\begingroup\$

Cryptography is frighteningly difficult to get right, but of course very important when looking after people’s passwords.

As @utnapistim says, the greatest problem is that “considering your decryption algorithm is deducing the key from the data itself, a hacker could do the same” (it’s actually a cipher, and once cracked, all passwords become available).

A better solution for managing passwords is to use a one-way hash function, which cannot be decrypted. In this usage, when you validate a password, you hash the supplied password, and compare it with your stored hash. If it matches, the user is authenticated (no decryption is needed).

Standard examples of such hashes are MD5 and SHA1.

SHA1 might be suitable for your needs, but the problem with this approach is that if your users use weak passwords, a hacker can use tables of hashes of common words to make a brute-force attack.

To address this, hashes are ‘salted’ with a pseudo-random salt before being hashed (the salt is stored with the hash). However, with specialised techniques relying on modern GPUs, even this is not adequately safe.

Hence algorithms were devised which are specifically designed to be computationally efficient for ‘normal’ use, but computationally expensive to attack (caveat: I’m not a cryptographic expert!). They could also be parameterised to be more expensive to attack as hardware becomes more powerful.

Good examples of these algorithms are bcrypt, PBKDF2, scrypt.

The best thing you can do is find an implementation of say either PBKDF2 or scrypt, and adapt that to your needs.

I don’t see any C++ implementations in the Wikipedia references for those two, so if you do a good job of it, you could offer it back to the community by publishing it on github!

\$\endgroup\$
  • 1
    \$\begingroup\$ This answer showed up in the low quality queue. This is good advice, but doesn't quite fit into the format of a Code Review. I do think it however. Answers solely suggesting alternatives are ok. You might want to read Tips for writing better answers. \$\endgroup\$ – RubberDuck Oct 20 '14 at 17:45
  • \$\begingroup\$ The advice in this "answer" is spot on. However, in order to make it a valid answer for Code Review, you have to at least point out one thing that is wrong with the code if you're going to recommend an alternative approach. \$\endgroup\$ – 200_success Oct 20 '14 at 17:47
  • \$\begingroup\$ Crypto++ (cryptopp.com) provides PBKDF. \$\endgroup\$ – Yuushi Oct 21 '14 at 10:53
  • \$\begingroup\$ I guess OpenSSL also does stackoverflow.com/questions/22795471/… \$\endgroup\$ – Boris Treukhov Oct 21 '14 at 15:08
  • \$\begingroup\$ I’ve expanded my answer to give more background/rationale. \$\endgroup\$ – ChrisV Oct 21 '14 at 23:05

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.