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 <[email protected]> * 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 <[email protected]>
bitwarden · Dec 4, 2024 · 864e675 · 864e675
1 parent 1dce7f5
commit 864e675
Show file tree

Hide file tree

Showing 15 changed files with 140 additions and 26 deletions.
diff --git a/apps/desktop/desktop_native/Cargo.lock b/apps/desktop/desktop_native/Cargo.lock
diff --git 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
@@ -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"[email protected]";
+        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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,