41

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.

5 Answers 5

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.

11
  • 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
    Commented May 15, 2014 at 16:22
  • 1
    something like this or more 'bro-bust' should be built into the class like SecureString.Equals(secstringone, secstringtwo); usage, nice work Commented Dec 17, 2014 at 22:42
  • 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
    Commented Nov 25, 2017 at 1:48
  • 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" msdn.microsoft.com/en-us/library/…
    – Gregor y
    Commented Apr 20, 2018 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. Commented Apr 23, 2018 at 0:07
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);
        }
     }
  }
11
  • 2
    SecureString doesn't override Equals, though, so this also just checks for reference equality. Commented Dec 21, 2010 at 18:44
  • 1
    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! Commented Dec 21, 2010 at 19:01
  • @SwDevMan81: wrong link! ;) The code at social.msdn.microsoft.com/Forums/en-US/clr/thread/… is what I used successfully for comparing two SecureStrings. Commented Dec 21, 2010 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. Commented Dec 21, 2010 at 22:11
  • 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
    Commented Jul 16, 2012 at 18:27
1

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
1

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

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.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.