From 864e6759fdb8afd5a96f011a5cc13f60a7701d6b Mon Sep 17 00:00:00 2001
From: Bernd Schoolmann <mail@quexten.com>
Date: Wed, 4 Dec 2024 12:54:55 +0100
Subject: [PATCH] Switch to rustcrypto argon2 on desktop (#11753)

* Switch to rustcrypto argon2 on desktop

* Make argon2 use zeroize

* Remove argon2 native modules from electron-builder config

* Clean rust implementation of argon2

* Update cargo.lock

* Update apps/desktop/desktop_native/napi/src/lib.rs

Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>

* Add tests

* Clean up test

* Remove argon2 external from webpack main

* Fix build

* Fix argon2 module causing a startup crash

---------

Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
---
 apps/desktop/desktop_native/Cargo.lock        | 42 +++++++++++++--
 apps/desktop/desktop_native/core/Cargo.toml   |  1 +
 .../desktop_native/core/src/crypto/crypto.rs  | 52 ++++++++++++++++++-
 apps/desktop/desktop_native/core/src/error.rs |  8 +++
 apps/desktop/desktop_native/napi/Cargo.toml   |  2 +-
 apps/desktop/desktop_native/napi/index.d.ts   |  3 ++
 apps/desktop/desktop_native/napi/src/lib.rs   | 19 +++++++
 apps/desktop/electron-builder.json            |  7 +--
 .../renderer-crypto-function.service.ts       |  7 +++
 apps/desktop/src/package-lock.json            |  3 +-
 apps/desktop/src/package.json                 |  3 +-
 .../main/main-crypto-function.service.ts      | 11 ++--
 apps/desktop/src/platform/preload.ts          |  4 +-
 apps/desktop/webpack.main.js                  |  2 -
 .../services/node-crypto-function.service.ts  |  2 +-
 15 files changed, 140 insertions(+), 26 deletions(-)

diff --git a/apps/desktop/desktop_native/Cargo.lock b/apps/desktop/desktop_native/Cargo.lock
index 16a5a48cd0..5e6a697c6c 100644
--- a/apps/desktop/desktop_native/Cargo.lock
+++ b/apps/desktop/desktop_native/Cargo.lock
@@ -83,6 +83,19 @@ dependencies = [
  "x11rb",
 ]
 
+[[package]]
+name = "argon2"
+version = "0.5.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3c3610892ee6e0cbce8ae2700349fcf8f98adb0dbfbee85aec3c9179d29cc072"
+dependencies = [
+ "base64ct",
+ "blake2",
+ "cpufeatures",
+ "password-hash",
+ "zeroize",
+]
+
 [[package]]
 name = "async-broadcast"
 version = "0.7.1"
@@ -320,6 +333,15 @@ dependencies = [
  "tokio-util",
 ]
 
+[[package]]
+name = "blake2"
+version = "0.10.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "46502ad458c9a52b69d4d4d32775c788b7a1b85e8bc9d482d92250fc0e3f8efe"
+dependencies = [
+ "digest",
+]
+
 [[package]]
 name = "block-buffer"
 version = "0.10.4"
@@ -660,6 +682,7 @@ dependencies = [
  "aes",
  "anyhow",
  "arboard",
+ "argon2",
  "async-stream",
  "base64",
  "bitwarden-russh",
@@ -1418,9 +1441,9 @@ checksum = "e1c0f5d67ee408a4685b61f5ab7e58605c8ae3f2b4189f0127d804ff13d5560a"
 
 [[package]]
 name = "napi-derive"
-version = "2.16.12"
+version = "2.16.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "17435f7a00bfdab20b0c27d9c56f58f6499e418252253081bfff448099da31d1"
+checksum = "7cbe2585d8ac223f7d34f13701434b9d5f4eb9c332cccce8dee57ea18ab8ab0c"
 dependencies = [
  "cfg-if",
  "convert_case",
@@ -1432,9 +1455,9 @@ dependencies = [
 
 [[package]]
 name = "napi-derive-backend"
-version = "1.0.74"
+version = "1.0.75"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "967c485e00f0bf3b1bdbe510a38a4606919cf1d34d9a37ad41f25a81aa077abe"
+checksum = "1639aaa9eeb76e91c6ae66da8ce3e89e921cd3885e99ec85f4abacae72fc91bf"
 dependencies = [
  "convert_case",
  "once_cell",
@@ -1726,6 +1749,17 @@ dependencies = [
  "windows-targets 0.52.6",
 ]
 
+[[package]]
+name = "password-hash"
+version = "0.5.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "346f04948ba92c43e8469c1ee6736c7563d71012b17d40745260fe106aac2166"
+dependencies = [
+ "base64ct",
+ "rand_core",
+ "subtle",
+]
+
 [[package]]
 name = "pbkdf2"
 version = "0.12.2"
diff --git a/apps/desktop/desktop_native/core/Cargo.toml b/apps/desktop/desktop_native/core/Cargo.toml
index 6abf7fbb0a..f9b58b7a27 100644
--- a/apps/desktop/desktop_native/core/Cargo.toml
+++ b/apps/desktop/desktop_native/core/Cargo.toml
@@ -27,6 +27,7 @@ anyhow = "=1.0.93"
 arboard = { version = "=3.4.1", default-features = false, features = [
     "wayland-data-control",
 ] }
+argon2 = { version = "=0.5.3", features = ["zeroize"] }
 async-stream = "=0.3.6"
 base64 = "=0.22.1"
 byteorder = "=1.5.0"
diff --git a/apps/desktop/desktop_native/core/src/crypto/crypto.rs b/apps/desktop/desktop_native/core/src/crypto/crypto.rs
index b1fd4426fa..4427138cb1 100644
--- a/apps/desktop/desktop_native/core/src/crypto/crypto.rs
+++ b/apps/desktop/desktop_native/core/src/crypto/crypto.rs
@@ -5,7 +5,7 @@ use aes::cipher::{
     BlockEncryptMut, KeyIvInit,
 };
 
-use crate::error::{CryptoError, Result};
+use crate::error::{CryptoError, KdfParamError, Result};
 
 use super::CipherString;
 
@@ -37,3 +37,53 @@ pub fn encrypt_aes256(
 
     Ok(CipherString::AesCbc256_B64 { iv, data })
 }
+
+pub fn argon2(
+    secret: &[u8],
+    salt: &[u8],
+    iterations: u32,
+    memory: u32,
+    parallelism: u32,
+) -> Result<[u8; 32]> {
+    use argon2::*;
+
+    let params = Params::new(memory, iterations, parallelism, Some(32)).map_err(|e| {
+        KdfParamError::InvalidParams(format!("Argon2 parameters are invalid: {e}",))
+    })?;
+    let argon = Argon2::new(Algorithm::Argon2id, Version::V0x13, params);
+
+    let mut hash = [0u8; 32];
+    argon
+        .hash_password_into(secret, &salt, &mut hash)
+        .map_err(|e| KdfParamError::InvalidParams(format!("Argon2 hashing failed: {e}",)))?;
+
+    // Argon2 is using some stack memory that is not zeroed. Eventually some function will
+    // overwrite the stack, but we use this trick to force the used stack to be zeroed.
+    #[inline(never)]
+    fn clear_stack() {
+        std::hint::black_box([0u8; 4096]);
+    }
+    clear_stack();
+    Ok(hash)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_argon2() {
+        let test_hash: [u8; 32] = [
+            112, 200, 85, 209, 100, 4, 246, 146, 117, 180, 152, 44, 103, 198, 75, 14, 166, 77, 201,
+            22, 62, 178, 87, 224, 95, 209, 253, 68, 166, 209, 47, 218,
+        ];
+        let secret = b"supersecurepassword";
+        let salt = b"mail@example.com";
+        let iterations = 3;
+        let memory = 1024 * 64;
+        let parallelism = 4;
+
+        let hash = argon2(secret, salt, iterations, memory, parallelism).unwrap();
+        assert_eq!(hash, test_hash,);
+    }
+}
diff --git a/apps/desktop/desktop_native/core/src/error.rs b/apps/desktop/desktop_native/core/src/error.rs
index d3104cb6f4..34624ed630 100644
--- a/apps/desktop/desktop_native/core/src/error.rs
+++ b/apps/desktop/desktop_native/core/src/error.rs
@@ -9,6 +9,8 @@ pub enum Error {
 
     #[error("Cryptography Error, {0}")]
     Crypto(#[from] CryptoError),
+    #[error("KDF Parameter Error, {0}")]
+    KdfParam(#[from] KdfParamError),
 }
 
 #[derive(Debug, Error)]
@@ -29,6 +31,12 @@ pub enum CryptoError {
     KeyDecrypt,
 }
 
+#[derive(Debug, Error)]
+pub enum KdfParamError {
+    #[error("Invalid KDF parameters: {0}")]
+    InvalidParams(String),
+}
+
 // Ensure that the error messages implement Send and Sync
 #[cfg(test)]
 const _: () = {
diff --git a/apps/desktop/desktop_native/napi/Cargo.toml b/apps/desktop/desktop_native/napi/Cargo.toml
index 644ff8a51d..6efd662b35 100644
--- a/apps/desktop/desktop_native/napi/Cargo.toml
+++ b/apps/desktop/desktop_native/napi/Cargo.toml
@@ -19,7 +19,7 @@ hex = "=0.4.3"
 anyhow = "=1.0.93"
 desktop_core = { path = "../core" }
 napi = { version = "=2.16.13", features = ["async"] }
-napi-derive = "=2.16.12"
+napi-derive = "=2.16.13"
 tokio = { version = "=1.41.1" }
 tokio-util = "=0.7.12"
 tokio-stream = "=0.1.15"
diff --git a/apps/desktop/desktop_native/napi/index.d.ts b/apps/desktop/desktop_native/napi/index.d.ts
index 956b2d726d..009f29333a 100644
--- a/apps/desktop/desktop_native/napi/index.d.ts
+++ b/apps/desktop/desktop_native/napi/index.d.ts
@@ -124,3 +124,6 @@ export declare namespace ipc {
     send(message: string): number
   }
 }
+export declare namespace crypto {
+  export function argon2(secret: Buffer, salt: Buffer, iterations: number, memory: number, parallelism: number): Promise<Buffer>
+}
diff --git a/apps/desktop/desktop_native/napi/src/lib.rs b/apps/desktop/desktop_native/napi/src/lib.rs
index 4c7bc8eaa9..c5ca655bce 100644
--- a/apps/desktop/desktop_native/napi/src/lib.rs
+++ b/apps/desktop/desktop_native/napi/src/lib.rs
@@ -528,3 +528,22 @@ pub mod ipc {
         }
     }
 }
+
+#[napi]
+pub mod crypto {
+    use napi::bindgen_prelude::Buffer;
+
+    #[napi]
+    pub async fn argon2(
+        secret: Buffer,
+        salt: Buffer,
+        iterations: u32,
+        memory: u32,
+        parallelism: u32,
+    ) -> napi::Result<Buffer> {
+        desktop_core::crypto::argon2(&secret, &salt, iterations, memory, parallelism)
+            .map_err(|e| napi::Error::from_reason(e.to_string()))
+            .map(|v| v.to_vec())
+            .map(|v| Buffer::from(v))
+    }
+}
diff --git a/apps/desktop/electron-builder.json b/apps/desktop/electron-builder.json
index 9a8bc45ae2..e4649a083a 100644
--- a/apps/desktop/electron-builder.json
+++ b/apps/desktop/electron-builder.json
@@ -18,12 +18,7 @@
     "**/*",
     "!**/node_modules/@bitwarden/desktop-napi/**/*",
     "**/node_modules/@bitwarden/desktop-napi/index.js",
-    "**/node_modules/@bitwarden/desktop-napi/desktop_napi.${platform}-${arch}*.node",
-
-    "!**/node_modules/argon2/**/*",
-    "**/node_modules/argon2/argon2.cjs",
-    "**/node_modules/argon2/package.json",
-    "**/node_modules/argon2/build/Release/argon2.node"
+    "**/node_modules/@bitwarden/desktop-napi/desktop_napi.${platform}-${arch}*.node"
   ],
   "electronVersion": "32.1.1",
   "generateUpdatesFilesForAllChannels": true,
diff --git a/apps/desktop/src/app/services/renderer-crypto-function.service.ts b/apps/desktop/src/app/services/renderer-crypto-function.service.ts
index 604e70b1eb..ee80dc2593 100644
--- a/apps/desktop/src/app/services/renderer-crypto-function.service.ts
+++ b/apps/desktop/src/app/services/renderer-crypto-function.service.ts
@@ -19,6 +19,13 @@ export class RendererCryptoFunctionService
     memory: number,
     parallelism: number,
   ): Promise<Uint8Array> {
+    if (typeof password === "string") {
+      password = new TextEncoder().encode(password);
+    }
+    if (typeof salt === "string") {
+      salt = new TextEncoder().encode(salt);
+    }
+
     return await ipc.platform.crypto.argon2(password, salt, iterations, memory, parallelism);
   }
 }
diff --git a/apps/desktop/src/package-lock.json b/apps/desktop/src/package-lock.json
index 085181e34b..0300b0b93c 100644
--- a/apps/desktop/src/package-lock.json
+++ b/apps/desktop/src/package-lock.json
@@ -9,8 +9,7 @@
       "version": "2024.12.0",
       "license": "GPL-3.0",
       "dependencies": {
-        "@bitwarden/desktop-napi": "file:../desktop_native/napi",
-        "argon2": "0.41.1"
+        "@bitwarden/desktop-napi": "file:../desktop_native/napi"
       }
     },
     "../desktop_native/napi": {
diff --git a/apps/desktop/src/package.json b/apps/desktop/src/package.json
index 0f82609b76..9a3c56cf17 100644
--- a/apps/desktop/src/package.json
+++ b/apps/desktop/src/package.json
@@ -12,7 +12,6 @@
     "url": "git+https://github.com/bitwarden/clients.git"
   },
   "dependencies": {
-    "@bitwarden/desktop-napi": "file:../desktop_native/napi",
-    "argon2": "0.41.1"
+    "@bitwarden/desktop-napi": "file:../desktop_native/napi"
   }
 }
diff --git a/apps/desktop/src/platform/main/main-crypto-function.service.ts b/apps/desktop/src/platform/main/main-crypto-function.service.ts
index 848e33113e..2fc3fde1db 100644
--- a/apps/desktop/src/platform/main/main-crypto-function.service.ts
+++ b/apps/desktop/src/platform/main/main-crypto-function.service.ts
@@ -1,6 +1,7 @@
 import { ipcMain } from "electron";
 
 import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service";
+import { crypto } from "@bitwarden/desktop-napi";
 import { NodeCryptoFunctionService } from "@bitwarden/node/services/node-crypto-function.service";
 
 export class MainCryptoFunctionService
@@ -13,16 +14,16 @@ export class MainCryptoFunctionService
       async (
         event,
         opts: {
-          password: string | Uint8Array;
-          salt: string | Uint8Array;
+          password: Uint8Array;
+          salt: Uint8Array;
           iterations: number;
           memory: number;
           parallelism: number;
         },
       ) => {
-        return await this.argon2(
-          opts.password,
-          opts.salt,
+        return await crypto.argon2(
+          Buffer.from(opts.password),
+          Buffer.from(opts.salt),
           opts.iterations,
           opts.memory,
           opts.parallelism,
diff --git a/apps/desktop/src/platform/preload.ts b/apps/desktop/src/platform/preload.ts
index 519ed68432..30e248d352 100644
--- a/apps/desktop/src/platform/preload.ts
+++ b/apps/desktop/src/platform/preload.ts
@@ -99,8 +99,8 @@ const nativeMessaging = {
 
 const crypto = {
   argon2: (
-    password: string | Uint8Array,
-    salt: string | Uint8Array,
+    password: Uint8Array,
+    salt: Uint8Array,
     iterations: number,
     memory: number,
     parallelism: number,
diff --git a/apps/desktop/webpack.main.js b/apps/desktop/webpack.main.js
index d52a947e36..25a68d8c86 100644
--- a/apps/desktop/webpack.main.js
+++ b/apps/desktop/webpack.main.js
@@ -81,8 +81,6 @@ const main = {
   externals: {
     "electron-reload": "commonjs2 electron-reload",
     "@bitwarden/desktop-napi": "commonjs2 @bitwarden/desktop-napi",
-
-    argon2: "commonjs2 argon2",
   },
 };
 
diff --git a/libs/node/src/services/node-crypto-function.service.ts b/libs/node/src/services/node-crypto-function.service.ts
index c8d676eeaa..b4126b0813 100644
--- a/libs/node/src/services/node-crypto-function.service.ts
+++ b/libs/node/src/services/node-crypto-function.service.ts
@@ -1,6 +1,5 @@
 import * as crypto from "crypto";
 
-import * as argon2 from "argon2";
 import * as forge from "node-forge";
 
 import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service";
@@ -40,6 +39,7 @@ export class NodeCryptoFunctionService implements CryptoFunctionService {
     const nodePassword = this.toNodeValue(password);
     const nodeSalt = this.toNodeBuffer(this.toUint8Buffer(salt));
 
+    const argon2 = await import("argon2");
     const hash = await argon2.hash(nodePassword, {
       salt: nodeSalt,
       raw: true,