5
\$\begingroup\$

The point of the program will be to have an file that will hold encrypted passwords which will only be unlocked by the password that encrypted them. I don't plan to use it in any web application/login system! I just want a safe way to save my passwords on a file that will be as difficult as possible to crack!

using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Cryptography;
using System.Text;
using System.Threading.Tasks;
using System.Numerics;
using System.Globalization;

namespace PassSaver.Model
{
    class Encryptor
    {
        static Random rng;
        public Encryptor() {
            rng = new Random();

        }
        public string Encrypt(string input,string password,int salt_length) {
            string hex_input = StringtoHex(GetRandomString(salt_length) + input + GetRandomString(salt_length));

            string hashed_password = GetHashString(password);

            BigInteger resullt = BigInteger.Multiply(BigInteger.Parse("0" + hex_input, NumberStyles.AllowHexSpecifier) , BigInteger.Parse("0" + hashed_password, NumberStyles.AllowHexSpecifier));
            return resullt.ToString("X2");
        }


        public string Decrypt(string input, string password,int salt_length)
        {
            BigInteger dec_input = BigInteger.Parse("0" + input, NumberStyles.AllowHexSpecifier);

            string hashed_password = GetHashString(password);

            BigInteger resullt = BigInteger.Divide(dec_input, BigInteger.Parse("0" + hashed_password, NumberStyles.AllowHexSpecifier));

            string toRe = HextoString(resullt.ToString("X2"));
            return toRe.Substring(salt_length, toRe.Length - salt_length*2);
        }

        private static byte[] GetHash(string inputString)
        {
            HashAlgorithm algorithm = SHA512.Create();  
            return algorithm.ComputeHash(Encoding.UTF8.GetBytes(inputString));
        }
        private static string GetRandomString(int length)
        {
            string chars = "$%#@!*abcdefghijklmnopqrstuvwxyz1234567890?;:ABCDEFGHIJKLMNOPQRSTUVWXYZ^&";
            int index = rng.Next(chars.Length);
            string toRe = "";

            for (int i = 0; i < length; i++)
            {
                toRe += chars[rng.Next(chars.Length)];
            }
            return toRe;
        }
        private static string GetHashString(string inputString)
        {
            StringBuilder sb = new StringBuilder();
            foreach (byte b in GetHash(inputString))
            {
                sb.Append(b.ToString("X2"));
            }
            return sb.ToString();
        }
        private static string StringtoHex(string input) {
            StringBuilder sb = new StringBuilder();
            foreach (byte b in Encoding.UTF8.GetBytes(input))
            {
                sb.Append(b.ToString("X2"));
            }
            return sb.ToString();
        }
        private static string HextoString(string input)
        {
            return Encoding.UTF8.GetString(HexToByteArray(input));
        }
        private static byte[] HexToByteArray(String hex)
        {
            int NumberChars = hex.Length;
            byte[] bytes = new byte[NumberChars / 2];
            for (int i = 0; i < NumberChars; i += 2)
            {
                bytes[i / 2] = Convert.ToByte(hex.Substring(i, 2), 16);
            }
            return bytes;
        }

    }
}

What I want to know is:

What can I do to make it better? Keep in mind, This is a learning project only. I don't plan to use it on a webapp or anything like that! I want to make it as good as possible but understand everything along the way because if i use lets say MD5, there would be a block of code in my program that i had no idea how it worked!

(This is only the encryption class, the final app will have a GUI and stuff like that.)

An example:

new Encryptor().Encrypt("Password","MyKey",5);

which returns:

2052430551065E0DD715BF4837F6A042542E918B1C49D2EB59CD700CF6B557E6E7ECC8BF91A5C1542DC8FD7AB50A9B90763336D3A71B442E55D5A3C833CD4C77CB27B4A359774B3BCF72FA7C14F96EDDD080

So:

  • Having the returned value, how difficult would it be to get the: "Password" and "MyKey"?
  • Having the returned value and the "Password" how difficult would it be to
    get the: "MyKey"?

EDIT:Two encrypted 3 digit long passwords, encrypted using the same key!

1A7E48889C97265AB09AD90550210BE0153F7B305B1DE5499E4159A082B72D65B2AA88104552F06B7E0EE7AB10C02504BE3FD713CB16D485776504A6A89DC34F8C3EFA 

1B50094415F42CFB13E4841E3555E09995DF86DF912F20EE4FD9E9B1CA541C160484F55E0CB8B7C0F42B01E2768B85C58AC6E8C0A919932734EE5DC0F65B603ED504B4 
\$\endgroup\$
  • 4
    \$\begingroup\$ "is it secure?" what is your anticipated attack vector? \$\endgroup\$ – Jesse C. Slicer May 1 '17 at 19:50
  • \$\begingroup\$ @JesseC.Slicer Pretty much anything, if you have access to anything (even source code, and some of my locked passwords decrypted) other than my password used for locking them, how difficult would it be to find the rest or get my password? \$\endgroup\$ – Feconiz May 1 '17 at 19:57
  • 1
    \$\begingroup\$ @πάνταῥεῖ it is supposed to just hold my passwords.Nothing amazing, but I want to make it as secure as I can, just for the fun of it pretty much! I could have used on of the existing programms if i wanted ssomething sure,but the point is i want to learn from the experience, plus have something i made that will be secure! \$\endgroup\$ – Feconiz May 1 '17 at 20:03
  • 1
    \$\begingroup\$ This is not actually an answer, but "encryption" and "hashing" are two very different concepts. You should never encrypt passwords and always hash them. \$\endgroup\$ – SO used to be good May 1 '17 at 20:15
  • 4
    \$\begingroup\$ As an exercise in writing code, have at it. On the other hand, if you are thinking of using this in an actual system then STOP. First read this: crackstation.net/hashing-security.htm (especially the part about how easy it is to mess up and you just shouldn't write your own crypto). \$\endgroup\$ – Donald.McLean May 1 '17 at 20:15
1
\$\begingroup\$
        public string Encrypt(string input,string password,int salt_length) {
            string hex_input = StringtoHex(GetRandomString(salt_length) + input + GetRandomString(salt_length));

            string hashed_password = GetHashString(password);

            BigInteger resullt = BigInteger.Multiply(BigInteger.Parse("0" + hex_input, NumberStyles.AllowHexSpecifier) , BigInteger.Parse("0" + hashed_password, NumberStyles.AllowHexSpecifier));
            return resullt.ToString("X2");
        }

I see BigInteger.Parse("0" + hashed_password, NumberStyles.AllowHexSpecifier) cropping up a lot, and it makes for some unreadable lines of code. That should definitely be pulled into a method as a starting point for possible refactoring to merge conversion methods which are always used together.

BigInteger provides an operator overload for *.


        public string Decrypt(string input, string password,int salt_length)
        {
            BigInteger dec_input = BigInteger.Parse("0" + input, NumberStyles.AllowHexSpecifier);

            string hashed_password = GetHashString(password);

            BigInteger resullt = BigInteger.Divide(dec_input, BigInteger.Parse("0" + hashed_password, NumberStyles.AllowHexSpecifier));

            string toRe = HextoString(resullt.ToString("X2"));
            return toRe.Substring(salt_length, toRe.Length - salt_length*2);
        }

So password is the key? Given that the description of the program said that it was for storing passwords, that was not at all obvious to me. After reading just the encryption method I wondered how you could get the password out again.


        private static byte[] GetHash(string inputString)
        {
            HashAlgorithm algorithm = SHA512.Create();  
            return algorithm.ComputeHash(Encoding.UTF8.GetBytes(inputString));
        }

HashAlgorithm implements IDisposable, so there should be a using expression here.


        private static string GetRandomString(int length)
        {
            string chars = "$%#@!*abcdefghijklmnopqrstuvwxyz1234567890?;:ABCDEFGHIJKLMNOPQRSTUVWXYZ^&";
            int index = rng.Next(chars.Length);
            string toRe = "";

            for (int i = 0; i < length; i++)
            {
                toRe += chars[rng.Next(chars.Length)];
            }
            return toRe;
        }

Looks like index is left over from a refactor and is now completely unused.

Why append to a string? Other methods in the same class use StringBuilder, which is a better way to do it. In this case, with a known fixed length, there's a good argument for char[] too.


One high-level design question: why so much use of hex? It's a terribly inefficient encoding. If you want to be able to copy-paste passwords, at least use base 64 if not base 85.


Finally, to address the security: give me two passwords encrypted with the same key and I will give you both passwords in very short order by finding the greatest common divisor, which will either be the hashed key or a small multiple of it. I will be able to tell when I've found the right multiple because the salt padding will all be from the reduced set of printable characters.

\$\endgroup\$
  • \$\begingroup\$ First of all thanks for your answer, I will implement most of the changes you proposed. I will not mark your answer as correct until the end of the day, so I give the chance to others, but really you helped me a lot! \$\endgroup\$ – Feconiz May 3 '17 at 9:00
  • \$\begingroup\$ I will change the encoding to base 64, to be fair I thought of doing it when writing it, but I thought it would be an overkill, now tho that you mention it after seeing the final results, you are right! \$\endgroup\$ – Feconiz May 3 '17 at 9:02
  • \$\begingroup\$ Now for the last part about the security, how would you recommend I make this not a problem? What can I change to make it stronger? \$\endgroup\$ – Feconiz May 3 '17 at 9:04
  • \$\begingroup\$ I don't really understand how the security problem would work... :/ can you axplain it with an example? I will edit 2 encrypted passwords in my question, both encrypted with the same key! \$\endgroup\$ – Feconiz May 3 '17 at 9:27
  • \$\begingroup\$ @Feconiz, abc and def with no salt. pastebin.com/raw/9aJpTASc The golden rules of crypto are something like: 1. Don't do this at home. 2. Use standard primitives which have been extensively reviewed, not homebrew. 3. Don't do this at home. If you're doing it for education purposes, use AES and a key whitening algorithm. But you'll probably still have side-channel attacks on the implementation. \$\endgroup\$ – Peter Taylor May 3 '17 at 10:23

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.