mobile icon indicating copy to clipboard operation
mobile copied to clipboard

[PS-1608] Use .NET's cross-platform cryptographic APIs.

Open teo-tsirpanis opened this issue 2 years ago • 5 comments

Type of change

  • [ ] Bug fix
  • [ ] New feature development
  • [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • [ ] Build/deploy pipeline (DevOps)
  • [ ] Other

Objective

.NET provides a plethora of cross-platform cryptographic APIs but nevertheless the BitWarden mobile apps don't use them at all, resorting to either directly using platform-specific dependencies like Apple's CommonCrypto, managed implementations like Bouncy Castle, or wrapper libraries like PCLCrypto.

With this PR all cryptography (except for HKDF whose APIs are on .NET 6+) is performed using .NET's System.Security.Cryptography types. Bouncy Castle and PCLCrypto dependencies were removed.

Code changes

  • The main changes are in the PclCryptoFunctionService class which was renamed to CryptoFunctionService. Each of its functions was rewritten to use the System.Security.Cryptography types. Reviewing the code should be easy; the method and type names match.
  • I also removed ICryptoPrimitiveService and its implementations; there's not much value in having a separate implementation just for PBKDF2, now that they have the same implementation across platforms.
  • The project changes and the rest of the C# code changes involve renaming and removing types and dependencies.

Before you submit

  • If this change requires a documentation update - notify the documentation team
    • https://bitwarden.com/help/what-encryption-is-used/#invoked-crypto-libraries will need to be changed.

teo-tsirpanis avatar Oct 04 '22 22:10 teo-tsirpanis

CLA assistant check
All committers have signed the CLA.

Thank you for your contribution! We've added this to our internal Community PR board for review. ID: PS-1608

This is great @teo-tsirpanis ! The historical reason for the native libs approach was related to runtime performance. Back then, .NET implementation was slow in comparison. Knowing that a lot changed since then, are you aware of recent performance tests or do you happen to have done them yourself?

vvolkgang avatar Oct 05 '22 20:10 vvolkgang

Hmm, upon further examination .NET uses CCKeyDerivationPBKDF only on macOS. I opened dotnet/runtime#76684 to ask why. I don't think the situation with Xamarin is any better.

I will revert the iOS PBDKF2 implementation; Android will still use the system APIs; they are at least not worse than Bouncy Castle. Once the app moves to modern .NET and MAUI and the linked issue gets resolved, we can remove the platform-specific implementation.