[Mono-dev] [PATCH] Fix for Membership.{Encrypt, Decrypt}Password

[Marek Habersack]

On Tue, 21 Nov 2006 08:20:38 -0500, Sebastien Pouliot
<sebastien.pouliot at gmail.com> scribbled:

> Hello Marek,
Hey Sebastien,

> On Tue, 2006-11-21 at 11:37 +0100, Marek Habersack wrote:
> > Hello,
> > 
> >   Attached is a diff which reimplements the two methods mentioned in
> > the subject. The current implementation would break if the password
> > passed was larger than the symmetric algorithm block size and it
> > wouldn't take into account the IV embedded into the encrypted
> > password when decrypting. Also, it assumet the IV (salt) size to be
> > 16 bytes,
> 
> Note that a salt and an IV value are, often confused, but different
> things. The IV, when required by the cipher mode, must be equal to the
> symmetric algorithm cipher block. A salt doesn't have this
> restriction.
> 
> Now the real question becomes "is it a salt or an IV ?"
In this case it's the IV. SqlMembershipProvider calls it 'salt', but
the encryption functions require the IV to be provided for the algo
(since it is not specified in machineKey)

> > which isn't forward-compatible. The reimplementation uses
> > CryptoStream and the size of the algorithm's IV to fix both of the
> > problems.
> 
> CryptoStream is nice as it does everything - but it comes at a high
> price (performance wise as it involves, by design, many object/memory
> allocations). I suggest people to avoid it unless they already deal
> with streams.
How would you suggest to approach this situation? We don't know the
length of the password beforehand and it's not limited, so I thought
CryptoStream would fit the bill here.

> >   At first I thought that embedding the IV in the password passed to
> > {Encrypt,Decrypt}Password is incorrect according to the MS.NET
> > documentation, but the tests show that it's the documentation that's
> > wrong. 
> 
> Are the tests available somewhere ?
Too late... I erased the code already :( - basically what I did was to
pass several strings to MS.NET's EncryptPassword and look at the output
and then compare it with the result we get. Also, Chris Toshok did the
tests long ago (he was the person that made me change my original
opinion that our implementation of EncryptPassword is wrong, since it
contradicts the MS.NET docs - he said he tested it when he was
implementing the code and that the tests showed our implementation is
right)

[snip]
> +               internal protected SymmetricAlgorithm Algorithm
> +               {
> 
>         I don't think this is part of the public API so it should be
it's not need - please read the other mail I sent to the list regarding
the issue.

>         private or internal (if reused outside this class). Also the
>         { should be on the same line as the declaration for
> properties.
sorry for the formatting, my emacs style isn't adjusted to the hanging
braces after the properties.

[snip]
[snip]
> +               byte [] EncryptDecryptPassword (byte [] data,
> ICryptoTransform transform)
> +               {
> +                       MemoryStream ms = new MemoryStream ();
> +                       CryptoStream cs = new CryptoStream (ms,
> transform, CryptoStreamMode.Write);
> +                       cs.Write (data, 0, data.Length);
> +                       cs.FlushFinalBlock ();
> +
> +                       byte [] ret = ms.ToArray ();
> +                       cs.Close ();
> +                       return ret;
> +               }
> 
>         If you use streams you should dispose them (better yet use
>         "using" when creating them). This is important for ASP.NET
> code as you can't know how many simultaneous instance could be
>         created (and how long the could be retained in memory).
ok

[snip]
> -                                       return rv;
> +                                       byte [] buf =
> EncryptDecryptPassword (password, decryptor);
> +                                       return buf;
> 
> 
>         Please call Array.Clear on arrays that contains "secret" data.
ok.

Please review the attached diff with the changes.

best regards,

marek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MembershipEncryption.diff
Type: text/x-patch
Size: 4890 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20061121/df79952d/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20061121/df79952d/attachment-0001.bin