search

opendnssec / SoftHSMv2

SoftHSM version 2
http://www.softhsm.org/
Other
763 stars 339 forks source link

Use authenticated encryption in SecureDataManager #623

Open rgerganov opened 3 years ago

rgerganov commented 3 years ago

Right now the SecureDataManager is using AES in CBC mode to protect the master key derived from the PIN and sensitive object attributes. The problem with this approach is that there is no way to guarantee the integrity of the object store. If an attacker gets access to the filesystem, they can modify the object files leaving this undetected by the SoftHSM. In this situation users will get incorrect results when using SoftHSM instead of error saying that the store has been tampered.

This problem can be easily solved by replacing AES-CBC with some of the authenticated modes of AES such as AES-GCM. SoftHSM already supports AES-GCM, so the code changes in SecureDataManager.cpp would be minimal. However, there will be some work for backward compatibility with old tokens.

I wonder what others think about this. I can start working on a PR if we reach some agreement.

rijswijk commented 3 years ago

Sounds like a worthwhile enhancement, if you would be willing to submit a PR that would be much appreciated. I do think this is a major change that requires at least a bump in minor version number, maybe a configurable option and a conversion tool for existing tokens, though. @halderen do you have thoughts on this?

rgerganov commented 3 years ago

I put some notes while working on this here: https://xakcop.com/post/softhsmv2/ Could be helpful to other people interested into how SoftHSM works.

Aearsis commented 3 years ago

If I'm not missing anything, this still does not protect fully from tampering. As the store isn't encrypted (and authenticated) as a whole, an attacker can still easily modify unencrypted attributes (like key usage flags), delete objects or object attributes, effectively turning them into default values. This is possible for both file and database backends.

Also, this PR doesn't protect against replaying old values (backups), but that's inevitable in case of SoftHSM - just a reminder.

Otherwise, the change itself looks good to me and is still better than no integrity at all :+1: