Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#60 Imrpovements to SecureEdit/ProtectedString (primarily)

closed
nobody
None
5
2010-02-23
2009-11-15
Mitch Capper
No

Main changes of patch:
SecureEdit now used ProtectedStrings rather than its own internal classes, I removed the ToUtf8 / GetAsString, technically these could be added back in (just calling the protected string functiosn) for plugins, and should just be marked depreceated to throw up warnings. This will encourage plugins to move to getting the ProtectedString as they then may also not need to decrypt it.
ProtectedString - Allow copying another ProtectedString without actually decrypting the other string. Allow comparing to another secure string (does require decrypting the strings a byte at a time for securestrings, but a bit better than just straight up decrypting the entire string into managed memory). Added RemoveInsert to be able to manipulate a ProtectedString (needed for SecureEdit). Finally modified ReadXorredString, to not just decrypt the entire securestring into a byte array for outputting.
XorredBuffer - Added Copy and Equals. Both which do so without decrypting the string, Copy does result in both strings having the same xor key, this could be changed is a cryptosource was passed in, but I am not sure its overly needed.

The primary goal is to avoid decrypting protected strings whenever possible. Now the only time a string is decrypted is when its actually going to be used. There is one exception to this, and thats the quality calculator. Personally I believe this feature should be changed to be an option rather than always done, as it does cause it to be decrypted into memory and some people may not find it necessary.

Unrelated Additions:
One of the ClipboardCopy methods wasn't checking policy prior to trying to copy, rather than checking later. Added a check in (MainForm.cs), this prevents the password from being decrypted if not allowed anyway.
I found was there was no decent way to properly track database changes (primarily only when the database was saved). To solve this I improved the touch functionality, ensuring that the Group always gets notified on child edits. (One can subscribe to the root group and get updates any time an entry is touched).

Patch at:
http://mitchcapper.com/keepass/secure_updates.patch

Discussion

  • Mitch Capper
    Mitch Capper
    2009-11-16

    Sorry I should also mention the ReadXorredString has a pretty poor implementation for UTF support. I added it in a bit last minute, but unfortunately am not sure of a better way to do it.

     
  • Mitch Capper
    Mitch Capper
    2009-11-19

    And the UTF implementation was nicely rushed in and has a bug, very sorry will see about rewriting it and updating later today.

     
  • Mitch Capper
    Mitch Capper
    2010-01-16

    Sorry I meant to reply by sooner on this. Unfortunately I failed to figure out a good way to avoid decrypting the entire SecureString into memory on save. There is no way I can see to support proper international formats and do this. While the byte array atleast we can manually zero out after, so it is not the end of the world, it would be nice if on every save this did not occur for every secure string. One option would be on save the ProtectedString stores the password in the newly XorredBuffer form in memory. That way if it is not touched at all future saves in the session would be able to just use the XorredBuffer form rather than having to continue to decrypt the securestring.

    The donwside here is obviously if it does go to be edited /used again it is once again decrypted from the XorredBuffer into a SecureString. To avoid this it would be possible on save to not only keep it in the newly created XorredBuffer but also leave it in SecureString. Thus if any editing/reading/etc was to be done we simply null out the XorredBuffer and it continues to use the SecureString version and we avoid that initial decrypt.

     
  • Dominik Reichl
    Dominik Reichl
    2010-02-23

    See e-mail.

    Thanks and best regards
    Dominik

     
  • Dominik Reichl
    Dominik Reichl
    2010-02-23

    • status: open --> closed