Protection against RAM attacks

[grzegorzkloc]

Good day,
first of all thank you for a great library, awesome job, much appreciated.

I am using your library in a project where cyber security is vital. In order to comply with some external requirements our software must ensure that the passwords are not stored in RAM longer than they need to. As we don't not have any control over when the GC collects the password string (and the string itself is immutable in memory) it would be a good idea to provide a mechanism to be able to overwrite the password in memory.

Do you think there is a chance for a feature which would include overloaded methods as below:

• HashPassword(byte[] text... and
• Verify(byte[] password...

Above changes would allow users to alter the contents of a password variable (because as you know we cannot do it with a string without using unsafe code) once a hash is generated or verified.

Should you be interested in including such a feature I have a sample code waiting and checked using WinDbg which would provide functionality described above.

Thank you for your time and an awesome piece of code which makes our lives easier.

[ChrisMcKee]

Feel free to throw it over and I'll have a look.
There's a fork a guy did a while back using secure string still around too

[AdamWhiteHat]

@ChrisMcKee It doesn't sound like @grzegorzkloc is actively working on such a thing (correct me if I'm wrong), but rather requesting the feature.

This sound like its in my wheelhouse and sounded like fun, so I've began taking stab at it. I'm also ensuring the framework methods that I call don't leave a copy of the bytes sitting around for the GC to collect, which makes it more challenging indeed.

@ChrisMcKee Question for you: Am I allowed to use the unsafe keyword? Will you accept such a solution? The constraints and requirements seem to be pushing in that direction, and I was wondering if unsafe code blocks were off the table or a deal breaker for you? I will obviously prefer a solution that avoids the use of the unsafe keyword, but it may be the most pragmatic solution. The trick is getting it from a string or SecureString to an array of bytes without the framework making a copy, which I wouldn't be able to zero out...

Yeah it probably doesn't make much sense to this without providing a SecureString overload, so expect that. I will look at that branch you mentioned to see what his approach was, and adopt that if its solves the problem to my satisfaction. Heck, it may be that that is already the correct and total solution, and I need not do anything. I don't know yet, I haven't looked.

[ChrisMcKee]

The secure string branch 'worked' afaik
#48

It's a niche requirement so I'm on the fence when it comes to adding this as an actual part of the library.

If you look at the .net dpa library you'll see it leads into the wonderful world of unsafe and the game of playing hide the key / removing from memory.

The use case is nearly always wpf apps as with web apps the password has already spent time as a string in the body / serialiser etc.

[AdamWhiteHat]

Re-reading what @grzegorzkloc said, I realized I totally misinterpreted what they were asking for; they just want an overload that takes an array of bytes, so they can zero it out after hashing and rest easy knowing that the BCrypt library didn't make a copy in memory.

Also, they already worked out some code that does this, contrary to what I thought before.

I, for one, think this is a good idea; it allows the library to be used in a more secure fashion, and it solves the non-trivial problem of trying to move a string into a byte array without making a copy in memory for the GC to clean up, which might not have made a lot of sense to do at that point anyways. So its a win-win.

Now, due to the way BCrypt versioning is handled, adding such an overload without a lot of duplication of code isn't so straight forward, so I would be interested in seeing what grzegorzkloc has. They have a fork but I cannot discern any changes made to it.

So @grzegorzkloc lets see what you have. Ignore my previous comments on this thead, I was confused.

[ChrisMcKee]

"a more secure fashion"... Really depends on the threat model but as stated prior, other than in wpf where you can buffer keystrokes to secure string from the outset, it's a lot of effort for inconsequential gain (imo).

I'd be interested in the use case though.

[AdamWhiteHat]

Well I suppose you're right. If you are at the point where you're considering the scenario where a threat actor is able to read your ram, you've got bigger problems...

[grzegorzkloc]

First of all, thank you for the answer and taking the time to think about my question / suggestion.
Secondly, I actually was working on a quick, dirty and simple solution which would not include the use of a SecureString. I remember there was a pull request which contained that. However, as it was rejected, I was under the impression that you did not want to include that in your project.

My suggestion is actually very simple, it would just be an overload of a few methods. Please find an example based on the HashPassword method below:

1. The original method could be changed as follows:

public static string HashPassword(
            string inputKey,
            string salt,
            bool enhancedEntropy,
            HashType hashType = DefaultEnhancedHashType) =>
            HashPassword(SafeUTF8.GetBytes(inputKey), salt, enhancedEntropy, hashType);

2. An overload could be introduced (basically the same implementation with minor alterations):

public static string HashPassword(byte[] inputKey, string salt, bool enhancedEntropy, HashType hashType = DefaultEnhancedHashType)
        {
            if (inputKey == null)
            {
                throw new ArgumentNullException(nameof(inputKey));
            }

            if (string.IsNullOrEmpty(salt))
            {
                throw new ArgumentException("Invalid salt: salt cannot be null or empty", nameof(salt));
            }

            if (enhancedEntropy && hashType == HashType.None)
            {
                throw new ArgumentException("Invalid HashType, You can't have an enhanced hash with type none. HashType.None is used for internal clarity only.", nameof(hashType));
            }

            // Determine the starting offset and validate the salt
            int startingOffset;
            char bcryptMinorRevision = (char)0;
            if (salt[0] != '$' || salt[1] != '2')
            {
                throw new SaltParseException("Invalid salt version");
            }
            else if (salt[2] == '$')
            {
                startingOffset = 3;
            }
            else
            {
                bcryptMinorRevision = salt[2];
                if (bcryptMinorRevision != 'a' && bcryptMinorRevision != 'b' && bcryptMinorRevision != 'x' && bcryptMinorRevision != 'y' || salt[3] != '$')
                {
                    throw new SaltParseException("Invalid salt revision");
                }
                startingOffset = 4;
            }

            // Extract number of rounds
            if (salt[startingOffset + 2] > '$')
            {
                throw new SaltParseException("Missing salt rounds");
            }

            // Extract details from salt
            int workFactor = Convert.ToInt16(salt.Substring(startingOffset, 2));

            // Throw if log rounds are out of range on hash, deals with custom salts
            if (workFactor < 1 || workFactor > 31)
            {
                throw new SaltParseException("Salt rounds out of range");
            }

            string extractedSalt = salt.Substring(startingOffset + 3, 22);

            byte[] inputBytes;

            //Only changes occur here
            if (enhancedEntropy)
            {
                inputBytes = EnhancedHash(inputKey, bcryptMinorRevision, hashType);
            }
            else
            {
                var additionalBytes = SafeUTF8.GetBytes(bcryptMinorRevision >= 'a' ? Nul : EmptyString);
                inputBytes = new byte[inputKey.Length + additionalBytes.Length];
                Array.Copy(inputKey, inputBytes, inputKey.Length);
                Array.Copy(additionalBytes, 0, inputBytes, inputKey.Length, additionalBytes.Length);
            }

            byte[] saltBytes = DecodeBase64(extractedSalt, BCryptSaltLen);

            BCrypt bCrypt = new BCrypt();

            byte[] hashed = bCrypt.CryptRaw(inputBytes, saltBytes, workFactor);

            // Generate result string
            var result = new StringBuilder(60);
            result.Append("$2").Append(bcryptMinorRevision).Append('$').Append(workFactor.ToString("D2")).Append('$');
            result.Append(EncodeBase64(saltBytes, saltBytes.Length));
            result.Append(EncodeBase64(hashed, (BfCryptCiphertext.Length * 4) - 1));
            
           //Clearing of arrays to ensure the contents are not left in memory and in order not to rely on GC
            //Clear Array could be any implementation which could zero out all elements.
            ClearArray(inputBytes);
            ClearArray(inputKey);

            return result.ToString();
        }
 

The advantages of above changes (at least from my point of view) are as follows:

1. The end user gets the possibility to use an array which (contrary to a string) can be modified in memory without creating a new instance.
2. It is the responsibility of the end user to get the byte[] from a SecureString and clear out the array once the hash is either calculated or verified.

I think the above could work well with a SecureString extension method such as this one (func could be a simple GenerateHash from your library):

public static unsafe string GetBCryptHashFromSecureString(this SecureString secureString, Func<byte[], string> func)
		{
			IntPtr bstr = Marshal.SecureStringToBSTR(secureString);
			int maxUtf8BytesCount = Encoding.UTF8.GetMaxByteCount(secureString.Length);
			IntPtr utf8Buffer = Marshal.AllocHGlobal(maxUtf8BytesCount);

			// Here's the magic:
			char* utf16CharsPtr = (char*)bstr.ToPointer();
			byte* utf8BytesPtr = (byte*)utf8Buffer.ToPointer();
			int utf8BytesCount = Encoding.UTF8.GetBytes(utf16CharsPtr, secureString.Length, utf8BytesPtr, maxUtf8BytesCount);

			Marshal.ZeroFreeBSTR(bstr);
			var utf8Bytes = new byte[utf8BytesCount];
			GCHandle utf8BytesPin = GCHandle.Alloc(utf8Bytes, GCHandleType.Pinned);
			Marshal.Copy(utf8Buffer, utf8Bytes, 0, utf8BytesCount);
			RtlZeroMemory(utf8Buffer, utf8BytesCount);
			Marshal.FreeHGlobal(utf8Buffer);
			try
			{
				return func(utf8Bytes);
			}
			finally
			{
				for (int i = 0; i < utf8Bytes.Length; i++)
				{
					utf8Bytes[i] = 0;
				}
				utf8BytesPin.Free();
			}
		}

Would you be interested in introducing such overloads?
Also, thank you in advance for any suggestions / comments.

[ChrisMcKee]

I can work on adding in byte array based methods similar to the frameworks handling of PBKDF2; these only limit exposure to GC though and in no way provide a guarantee of zero exposure.

Array clear is a built in method.

ref:

https://github.com/dotnet/aspnetcore/blob/702b937577122445498a2d800c7ca1782a1b3556/src/DataProtection/Cryptography.KeyDerivation/src/KeyDerivation.cs#L27

https://github.com/dotnet/aspnetcore/blob/702b937577122445498a2d800c7ca1782a1b3556/src/DataProtection/DataProtection/src/SP800_108/Win7SP800_108_CTR_HMACSHA512Provider.cs#L20

https://github.com/dotnet/aspnetcore/blob/702b937577122445498a2d800c7ca1782a1b3556/src/DataProtection/Cryptography.KeyDerivation/src/PBKDF2/ManagedPbkdf2Provider.cs#L69

[grzegorzkloc]

Thank you very much. I am sure that these changes will be beneficial not only to me but to many other users as well.
Much appreciated.

[penguinawesome]

+1

[jvandertil]

I'd replace byte[] with Span<byte> in the API for more flexibility.

Just my 2c.

[ChrisMcKee]

I'd replace byte[] with Span<byte> in the API for more flexibility.

For sure; it was certainly looking to be the choice. I'll undoubtedly have to tackle the documentation too as realistically this isn't something everyone needs to use and, realistically, I doubt most devs will even consider threat modelling before using something, in which case the standard routes the safest.

---

Just so I don't have to find them every few months and so I don't lose them again when I eventually un-gunk the docs. Secure String ...

• https://github.com/dotnet/platform-compat/blob/master/docs/DE0001.md
• Obsolete the SecureString type dotnet/runtime#30612
• GC Heap Compaction should clear the source data dotnet/runtime#10480
• SecureString obsoletions and shrouded buffer proposal dotnet/designs#147 - proposed replacement in .net6lts

[ChrisMcKee]

To be clear by "safest" (because god only knows this is public, and someone will appear with a tinfoil hat on to complain)
Jeremy Barton basically covered the argument for me so I'll resist repeating it.

dotnet/runtime#30612 (comment)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.