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.

Solution 1

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);
        }
     }
  }

Solution 2

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.

Solution 3

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);

Solution 4

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

View more solutions

Comments

[Will Vousden]

SecureString doesn't override Equals, though, so this also just checks for reference equality.

[Sarah Vessels]

SwDevMan81: that social.msdn link's suggestion worked for me. I used 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]

@SwDevMan81: wrong link! ;) The code at social.msdn.microsoft.com/Forums/en-US/clr/thread/… is what I used successfully for comparing two SecureStrings.

[Sarah Vessels]

@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.

[Mike Christian]

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.

[SwDevMan81]

@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.

[Mike Christian]

@SwDevMan81 - Removed down vote, resultant to posting of unsafe code. Thank you!

[venkat]

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.

[Nikola Novak]

@AlexJ, thanks, I've modified the code to fix the leak.

[stackuser83]

something like this or more 'bro-bust' should be built into the class like SecureString.Equals(secstringone, secstringtwo); usage, nice work

[CptRobby]

@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]

@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? ;)

[DaveRandom]

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.

[Gregor y]

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]

@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]

@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.

[Nikola Novak]

@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.

[Gregor y]

@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" msdn.microsoft.com/en-us/library/…

[Nikola Novak]

@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.

[Luaan]

@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.