C# - compare two SecureStrings for equality

Question

I have a WPF application with two PasswordBoxes, one for the password and another for the password to be entered a second time for confirmation purposes. I was wanting to use PasswordBox.SecurePassword to get the SecureString of the password, but I need to be able to compare the contents of the two PasswordBoxes to ensure equality before I accept the password. However, two identical SecureStrings are not considered equal:

var secString1 = new SecureString();
var secString2 = new SecureString();
foreach (char c in "testing")
{
    secString1.AppendChar(c);
    secString2.AppendChar(c);
}
Assert.AreEqual(secString1, secString2); // This fails

I was thinking comparing the Password property of the PasswordBoxes would defeat the point of accessing only SecurePassword because I'd be reading the plain-text password. What should I do to compare the two passwords without sacrificing security?

Edit: based on this question, I'm checking out this blog post about "using the Marshal class to convert the SecureString to ANSI or Unicode or a BSTR", then maybe I can compare those.

Nikola Novak · Answer 1 · 2014-05-16T14:29:53.327

40

This doesn't have unsafe blocks and won't display the password in plaintext:

public static bool IsEqualTo(this SecureString ss1, SecureString ss2)
{
 IntPtr bstr1 = IntPtr.Zero;
 IntPtr bstr2 = IntPtr.Zero;
 try
 {
  bstr1 = Marshal.SecureStringToBSTR(ss1);
  bstr2 = Marshal.SecureStringToBSTR(ss2);
  int length1 = Marshal.ReadInt32(bstr1, -4);
  int length2 = Marshal.ReadInt32(bstr2, -4);
  if (length1 == length2)
  {
   for (int x = 0; x < length1; ++x)
   {
    byte b1 = Marshal.ReadByte(bstr1, x);
    byte b2 = Marshal.ReadByte(bstr2, x);
    if (b1 != b2) return false;
   }
  }
  else return false;
  return true;
 }
 finally
 {
  if (bstr2 != IntPtr.Zero) Marshal.ZeroFreeBSTR(bstr2);
  if (bstr1 != IntPtr.Zero) Marshal.ZeroFreeBSTR(bstr1);
 }
}

Edit: Fixed the leak as recommended by Alex J.

edited May 16 '14 at 14:29

answered Apr 20 '14 at 13:38

Nikola Novak

4,091
5
24
33

1

You might leak the IntPtrs if an exception occurs after the Marshal.SecureStringToBSTR() calls, but before the try block. You should put them inside the try block instead. – Alex J May 15 '14 at 16:22
@AlexJ, thanks, I've modified the code to fix the leak. – Nikola Novak May 16 '14 at 14:36
1

something like this or more 'bro-bust' should be built into the class like `SecureString.Equals(secstringone, secstringtwo);` usage, nice work – stackuser83 Dec 17 '14 at 22:42
@NikolaNovak +1 This is definitely a better answer! I was going to post something like this after reading the accepted answer until I saw this. Using *pointers in C# just shouldn't be done when you can just compare the byte values from memory using Marshal.ReadByte()! ;) – CptRobby Mar 10 '15 at 19:55
@stackuser83 I agree with you that this should be built in. But if you'll note the `this` keyword used in the first line, Nikola actually created an extension method. So using your example, you could just write `secstringone.IsEqualTo(secstringtwo);` Pretty easy to read code, no? ;) – CptRobby Mar 10 '15 at 20:00
1

This code is vulnerable to timing attacks. For the original question this doesn't matter, but if you are doing this to verify a password (which you almost certainly should not be because you should be using hashes instead) this can be mitigated by declaring `int result = 0;` ahead of the loop, changing the loop body to `result |= Marshal.ReadByte(bstr1, x) ^ Marshal.ReadByte(bstr2, x);` and returning `result == 0`. This will make the comparison constant time as it always visits every character in the string - it will still leak the length but this is unavoidable. – DaveRandom Nov 25 '17 at 01:48
uh, I hate to burst your C# bubble, but `Marshal.SecureStringToBSTR` converts it to a byte array; so just because you don't cast as a (char *) it's still loaded that way in memory even if your debugger is confused on how to display it for you. – Gregor y Jan 13 '18 at 00:15
@DaveRandom, `if(length1 == length2)` would avoid leaking the length; although for speed and security it'd probably be better to instead add `if(ss1.length != ss2.length)return false;` at the outset, then neither SecureStrings would be needlessly converted to 'plain-text'. – Gregor y Jan 13 '18 at 00:49
@Gregory The bytes are always written somewhere in memory whether you `Marshal.SecureStringToBSTR` or not. The best you can ever do in terms of hiding the password is to consume each character as it is typed into a password field by adding it to a hash, but that still leaves some time when each character is in memory. And even if you do all this, then your password is actually the resulting hash (since you can't simply convert it back to the actual password), which is no problem for a persistent attacker. – Nikola Novak Apr 13 '18 at 06:28
2

@NikolaNovak, ok I guess I just don't know what you mean in your answer when you say "and won't display the password in plaintext", because `Marshal.SecureStringToBSTR` does decrypt the contents of `SecureString` and loads the deciphered copy at the returned memory address, which MS calls a "clear-text string in unmanaged memory" https://msdn.microsoft.com/en-us/library/system.security.securestring(v=vs.110).aspx#interop – Gregor y Apr 20 '18 at 22:48
1

@Gregory I mean exactly that. It won't display it in plaintext. Instead of 'A' you will see 65. Not much of a security, but good enough if your potential attacker happens to be looking over your shoulder when you're debugging. – Nikola Novak Apr 23 '18 at 00:07

SwDevMan81 · Accepted Answer · 2012-07-16T18:26:39.783

19

It looks like you could use this to compare the two SecureStrings.

It uses unsafe code to iterate through the strings:

bool SecureStringEqual(SecureString s1, SecureString s2)  
{  
    if (s1 == null)  
    {  
        throw new ArgumentNullException("s1");  
    }  
    if (s2 == null)  
    {  
        throw new ArgumentNullException("s2");  
    }  

    if (s1.Length != s2.Length)  
    {  
        return false;  
    }  

    IntPtr bstr1 = IntPtr.Zero;  
    IntPtr bstr2 = IntPtr.Zero;  

    RuntimeHelpers.PrepareConstrainedRegions();  

    try 
    {  
        bstr1 = Marshal.SecureStringToBSTR(s1);  
        bstr2 = Marshal.SecureStringToBSTR(s2);  

        unsafe 
        {  
            for (Char* ptr1 = (Char*)bstr1.ToPointer(), ptr2 = (Char*)bstr2.ToPointer();  
                *ptr1 != 0 && *ptr2 != 0;  
                 ++ptr1, ++ptr2)  
            {  
                if (*ptr1 != *ptr2)  
                {  
                    return false;  
                }  
            }  
        }  

        return true;  
    }  
    finally 
    {  
        if (bstr1 != IntPtr.Zero)  
        {  
            Marshal.ZeroFreeBSTR(bstr1);  
        }  

        if (bstr2 != IntPtr.Zero)  
        {  
            Marshal.ZeroFreeBSTR(bstr2);  
        }  
    }  
}

I have modified it below to work without unsafe code (note however you are able to see the string in plain text when debugging):

  Boolean SecureStringEqual(SecureString secureString1, SecureString secureString2)
  {
     if (secureString1 == null)
     {
        throw new ArgumentNullException("s1");
     }
     if (secureString2 == null)
     {
        throw new ArgumentNullException("s2");
     }

     if (secureString1.Length != secureString2.Length)
     {
        return false;
     }

     IntPtr ss_bstr1_ptr = IntPtr.Zero;
     IntPtr ss_bstr2_ptr = IntPtr.Zero;

     try
     {
        ss_bstr1_ptr = Marshal.SecureStringToBSTR(secureString1);
        ss_bstr2_ptr = Marshal.SecureStringToBSTR(secureString2);

        String str1 = Marshal.PtrToStringBSTR(ss_bstr1_ptr);
        String str2 = Marshal.PtrToStringBSTR(ss_bstr2_ptr);

        return str1.Equals(str2);
     }
     finally
     {
        if (ss_bstr1_ptr != IntPtr.Zero)
        {
           Marshal.ZeroFreeBSTR(ss_bstr1_ptr);
        }

        if (ss_bstr2_ptr != IntPtr.Zero)
        {
           Marshal.ZeroFreeBSTR(ss_bstr2_ptr);
        }
     }
  }

edited Jul 16 '12 at 18:26

answered Dec 21 '10 at 18:41

SwDevMan81

48,814
22
151
184

2

`SecureString` doesn't override `Equals`, though, so this also just checks for reference equality. – Will Vousden Dec 21 '10 at 18:44
1

SwDevMan81: that social.msdn link's suggestion worked for me. I used http://www.csharpfriends.com/Articles/getArticle.aspx?articleID=351 to allow unsafe code in my project. You should provide that MSDN article you linked as an Answer and I'll select it. If someone has a suggestion for fixing the code to not use `unsafe`, that'd be helpful, too! – Sarah Vessels Dec 21 '10 at 19:01
@SwDevMan81: wrong link! ;) The code at http://social.msdn.microsoft.com/Forums/en-US/clr/thread/555a5cb6-790d-415d-b079-00d62b3a9632/ is what I used successfully for comparing two `SecureString`s. – Sarah Vessels Dec 21 '10 at 21:52
6

@SwDevMan: yikes, this one definitely works and doesn't have `unsafe` blocks, but stepping through it with the debugger shows those two `str1` and `str2` strings end up with the sensitive data in plain text. I prefer the `unsafe` version because it just compares pointers; stepping through it with the debugger, I don't see intelligible data in plain text. – Sarah Vessels Dec 21 '10 at 22:11
Downvote: This code exposes the plain text values. The purpose of using SecureString is to prevent exposing plain text values. You must use unsafe code, to compare the de-referenced values. – Mike Christian Jul 16 '12 at 18:24
1

@MikeChristian - I posted the link with the unsafe solution. I just provided an alternate solution example that doesnt use unsafe code. Feel free to use the unsafe one if you are worried about exposing the plain text. – SwDevMan81 Jul 16 '12 at 18:27
@SwDevMan81 - Removed down vote, resultant to posting of unsafe code. Thank you! – Mike Christian Jul 16 '12 at 19:23
@MikeChristian, you're arguing over syntactic sugar all of these methods more or less have the same plain-text memory issues which is due to the fact that by design the only way to compare two `SecureStrings` is to convert them back to their original non-encrypted state. – Gregor y Jan 13 '18 at 01:16
@SarahVessels The problem isn't what you see in the debugger, the problem is that you have no control over the lifetime of the sensitive data. That's the whole reason why `SecureString` exists - converting secure strings to strings defeats the purpose. The unsafe version only exposes the data for the duration of the comparison - the `string` version has no control over when the data is zeroed. However, the unsafe version has another problem - it doesn't handle Unicode properly. A better solution (if you target Windows) would be to use `wcscmp`. – Luaan Jun 25 '20 at 09:00
Why do you throw on `null`? Just return a factual answer (`true` if both null, and `false` if either is null) – Mike Bruno Jul 28 '22 at 18:04
To let the caller of the function know they likely provided a bad input argument. I dont think anyone expects a SecureString to be null ;) – SwDevMan81 Jul 29 '22 at 20:58

score 1 · Answer 3 · answered Mar 10 '17 at 20:41

Translating @NikolaNovák answer to plain PowerShell:

param(
[Parameter(mandatory=$true,position=0)][SecureString]$ss1,
[Parameter(mandatory=$true,position=1)][SecureString]$ss2
)

function IsEqualTo{
   param(
    [Parameter(mandatory=$true,position=0)][SecureString]$ss1,
    [Parameter(mandatory=$true,position=1)][SecureString]$ss2
   )

  begin{
    [IntPtr] $bstr1 = [IntPtr]::Zero;
    [IntPtr] $bstr2 = [IntPtr]::Zero;
    [bool]$answer=$true;
  }

  process{
    try{
        $bstr1 = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($ss1);
        $bstr2 = [System.Runtime.InteropServices.Marshal]::SecureStringToBSTR($ss2);
        [int]$length1 = [System.Runtime.InteropServices.Marshal]::ReadInt32($bstr1, -4);
        [int]$length2 = [System.Runtime.InteropServices.Marshal]::ReadInt32($bstr2, -4);

        if ($length1 -eq $length2){
            for ([int]$x -eq 0; $x -lt $length1; ++$x){
                [byte]$b1 = [System.Runtime.InteropServices.Marshal]::ReadByte($bstr1, $x);
                [byte]$b2 = [System.Runtime.InteropServices.Marshal]::ReadByte($bstr2, $x);
                if ($b1  -ne $b2){
                    $answer=$false;
                }
            }
        }
        else{ $answer=$false;}
    }
    catch{
    }
    finally
    {
        if ($bstr2 -ne [IntPtr]::Zero){ [System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($bstr2)};
        if ($bstr1 -ne [IntPtr]::Zero){ [System.Runtime.InteropServices.Marshal]::ZeroFreeBSTR($bstr1)};
    }
  }
  END{
    return $answer
  }
}
IsEqualTo -ss1 $ss1  -ss2 $ss2

Simon Mourier · Answer 4 · 2018-04-13T17:38:26.103

If the code is running on Windows Vista or higher, here is a version that's based on the CompareStringOrdinal Windows function, so there's no plain text, all buffers stay unmanaged. Bonus is it supports case-insensitive comparison.

public static bool EqualsOrdinal(this SecureString text1, SecureString text2, bool ignoreCase = false)
{
    if (text1 == text2)
        return true;

    if (text1 == null)
        return text2 == null;

    if (text2 == null)
        return false;

    if (text1.Length != text2.Length)
        return false;

    var b1 = IntPtr.Zero;
    var b2 = IntPtr.Zero;
    try
    {
        b1 = Marshal.SecureStringToBSTR(text1);
        b2 = Marshal.SecureStringToBSTR(text2);
        return CompareStringOrdinal(b1, text1.Length, b2, text2.Length, ignoreCase) == CSTR_EQUAL;
    }
    finally
    {
        if (b1 != IntPtr.Zero)
        {
            Marshal.ZeroFreeBSTR(b1);
        }

        if (b2 != IntPtr.Zero)
        {
            Marshal.ZeroFreeBSTR(b2);
        }
    }
}

public static bool EqualsOrdinal(this SecureString text1, string text2, bool ignoreCase = false)
{
    if (text1 == null)
        return text2 == null;

    if (text2 == null)
        return false;

    if (text1.Length != text2.Length)
        return false;

    var b = IntPtr.Zero;
    try
    {
        b = Marshal.SecureStringToBSTR(text1);
        return CompareStringOrdinal(b, text1.Length, text2, text2.Length, ignoreCase) == CSTR_EQUAL;
    }
    finally
    {
        if (b != IntPtr.Zero)
        {
            Marshal.ZeroFreeBSTR(b);
        }
    }
}

private const int CSTR_EQUAL = 2;

[DllImport("kernel32")]
private static extern int CompareStringOrdinal(IntPtr lpString1, int cchCount1, IntPtr lpString2, int cchCount2, bool bIgnoreCase);

[DllImport("kernel32")]
private static extern int CompareStringOrdinal(IntPtr lpString1, int cchCount1, [MarshalAs(UnmanagedType.LPWStr)] string lpString2, int cchCount2, bool bIgnoreCase);

score 0 · Answer 5 · answered Aug 25 '17 at 08:33

You could take a different approach. I hit the same problem in my code, the comparison of a password and a confirmation, both of type SecureString. I realised that the end goal was that the new password needs to be stored in the database as a base-64 string. So what I did was simply pass the confirmation string through the same code as if I were going to write it to the database. Then, when I have two base-64 strings, I compare them at that point, which is a simple string comparison.

It does take a bit more plumbing to communicate any failure all the way back to the UI layer, but the end result seemed acceptable. This code hopefully is enough to give the basic idea.

private string CalculateHash( SecureString securePasswordString, string saltString )
{
    IntPtr unmanagedString = IntPtr.Zero;
    try
    {
        unmanagedString = Marshal.SecureStringToGlobalAllocUnicode( securePasswordString );
        byte[] passwordBytes = Encoding.UTF8.GetBytes( Marshal.PtrToStringUni( unmanagedString ) );
        byte[] saltBytes = Encoding.UTF8.GetBytes( saltString );
        byte[] passwordPlusSaltBytes = new byte[ passwordBytes.Length + saltBytes.Length ];
        Buffer.BlockCopy( passwordBytes, 0, passwordPlusSaltBytes, 0, passwordBytes.Length );
        Buffer.BlockCopy( saltBytes, 0, passwordPlusSaltBytes, passwordBytes.Length, saltBytes.Length );
        HashAlgorithm algorithm = new SHA256Managed();
        return Convert.ToBase64String( algorithm.ComputeHash( passwordPlusSaltBytes ) );
    }
    finally
    {
        if( unmanagedString != IntPtr.Zero )
            Marshal.ZeroFreeGlobalAllocUnicode( unmanagedString );
    }
}

string passwordSalt = "INSERT YOUR CHOSEN METHOD FOR CONSTRUCTING A PASSWORD SALT HERE";
string passwordHashed = CalculateHash( securePasswordString, passwordSalt );
string confirmPasswordHashed = CalculateHash( secureConfirmPasswordString, passwordSalt );
if( passwordHashed == confirmPasswordHashed )
{
    // Both matched so go ahead and persist the new password.
}
else
{
    // Strings don't match, so communicate the failure back to the UI.
}

I am a bit of a newbie at security programming, so I welcome any suggestions for improvement.

C# - compare two SecureStrings for equality

5 Answers5

Linked