From cf689919a4b1b886b00642090a234d14a4effaec Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 17 Apr 2024 11:00:00 +0100 Subject: [PATCH] Fix tokens encrypted with Node crypto implementation being undecryptable in new Rust implementation. (#930) * Encrypt with new padding algo when possible. * formatting * changelog --- Cargo.lock | 33 ++++++++++++++++++++ Cargo.toml | 3 +- changelog.d/930.bugfix | 2 ++ src/tokens/UserTokenStore.ts | 13 +++----- src/tokens/mod.rs | 46 ++++++++++++++++++++++------ tests/tokens/tokenencryption.spec.ts | 25 ++++++++++++--- 6 files changed, 100 insertions(+), 22 deletions(-) create mode 100644 changelog.d/930.bugfix diff --git a/Cargo.lock b/Cargo.lock index 4889bca6..03bfa95c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -191,6 +191,15 @@ version = "0.8.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "06ea2b9bc92be3c2baa9334a323ebca2d6f074ff852cd1d7b11064035cd3868f" +[[package]] +name = "cpufeatures" +version = "0.2.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53fe5e26ff1b7aef8bca9c6080520cfb8d9333c7568e1829cef191a9723e5504" +dependencies = [ + "libc", +] + [[package]] name = "crypto-common" version = "0.1.6" @@ -727,6 +736,7 @@ dependencies = [ "serde", "serde_derive", "serde_json", + "sha1", "url", ] @@ -1316,6 +1326,7 @@ dependencies = [ "pkcs1", "pkcs8", "rand_core", + "sha2", "signature", "spki", "subtle", @@ -1579,6 +1590,28 @@ dependencies = [ "serde", ] +[[package]] +name = "sha1" +version = "0.10.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3bf829a2d51ab4a5ddf1352d8470c140cadc8301b2ae1789db023f01cedd6ba" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + +[[package]] +name = "sha2" +version = "0.10.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "793db75ad2bcafc3ffa7c68b215fee268f537982cd901d132f89c6343f3a3dc8" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "signature" version = "2.2.0" diff --git a/Cargo.toml b/Cargo.toml index 0711159f..dfe77d57 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,8 @@ atom_syndication = "0.12" ruma = { version = "0.9", features = ["events", "html"] } reqwest = "0.11" rand = "0.8.5" -rsa = "0.9.6" +rsa = { version = "0.9.6", features = ["sha2"] } base64ct = { version = "1.6.0", features = ["alloc"] } +sha1 = "0.10.6" [build-dependencies] napi-build = "2" diff --git a/changelog.d/930.bugfix b/changelog.d/930.bugfix new file mode 100644 index 00000000..cd02e65e --- /dev/null +++ b/changelog.d/930.bugfix @@ -0,0 +1,2 @@ +Track which key was used to encrypt secrets in storage, and encrypt/decrypt secrets in Rust. + diff --git a/src/tokens/UserTokenStore.ts b/src/tokens/UserTokenStore.ts index 7ea2ce3b..9a30df73 100644 --- a/src/tokens/UserTokenStore.ts +++ b/src/tokens/UserTokenStore.ts @@ -16,7 +16,7 @@ import { JiraOnPremClient } from "../jira/client/OnPremClient"; import { JiraCloudClient } from "../jira/client/CloudClient"; import { TokenError, TokenErrorCode } from "../errors"; import { TypedEmitter } from "tiny-typed-emitter"; -import { hashId, TokenEncryption } from "../libRs"; +import { hashId, TokenEncryption, stringToAlgo } from "../libRs"; const ACCOUNT_DATA_TYPE = "uk.half-shot.matrix-hookshot.github.password-store:"; const ACCOUNT_DATA_GITLAB_TYPE = "uk.half-shot.matrix-hookshot.gitlab.password-store:"; @@ -32,7 +32,7 @@ export const AllowedTokenTypes = ["github", "gitlab", "jira"]; interface StoredTokenData { encrypted: string|string[]; keyId: string; - algorithm: 'rsa'; + algorithm: 'rsa'|'rsa-pkcs1v15'; instance?: string; } @@ -102,7 +102,7 @@ export class UserTokenStore extends TypedEmitter { const data: StoredTokenData = { encrypted: tokenParts, keyId: this.keyId, - algorithm: "rsa", + algorithm: "rsa-pkcs1v15", instance: instanceUrl, }; await this.intent.underlyingClient.setAccountData(key, data); @@ -148,18 +148,15 @@ export class UserTokenStore extends TypedEmitter { return null; } // For legacy we just assume it's the current configured key. - const algorithm = obj.algorithm ?? "rsa"; + const algorithm = stringToAlgo(obj.algorithm ?? "rsa"); const keyId = obj.keyId ?? this.keyId; - if (algorithm !== 'rsa') { - throw new Error(`Algorithm for stored data is '${algorithm}', but we only support RSA`); - } if (keyId !== this.keyId) { throw new Error(`Stored data was encrypted with a different key to the one currently configured`); } const encryptedParts = typeof obj.encrypted === "string" ? [obj.encrypted] : obj.encrypted; - const token = this.tokenEncryption.decrypt(encryptedParts); + const token = this.tokenEncryption.decrypt(encryptedParts, algorithm); this.userTokens.set(key, token); return token; } catch (ex) { diff --git a/src/tokens/mod.rs b/src/tokens/mod.rs index 60e42941..56a59a4b 100644 --- a/src/tokens/mod.rs +++ b/src/tokens/mod.rs @@ -5,7 +5,8 @@ use napi::bindgen_prelude::Buffer; use napi::Error; use rsa::pkcs1::DecodeRsaPrivateKey; use rsa::pkcs8::DecodePrivateKey; -use rsa::{Pkcs1v15Encrypt, RsaPrivateKey, RsaPublicKey}; +use rsa::{Oaep, Pkcs1v15Encrypt, RsaPrivateKey, RsaPublicKey}; +use sha1::Sha1; static MAX_TOKEN_PART_SIZE: usize = 128; @@ -31,6 +32,24 @@ enum DecryptError { FromUtf8(FromUtf8Error), } +#[napi] +pub enum Algo { + RSAOAEP, + RSAPKCS1v15, +} + +#[napi] +pub fn string_to_algo(algo_str: String) -> Result { + match algo_str.as_str() { + "rsa" => Ok(Algo::RSAOAEP), + "rsa-pkcs1v15" => Ok(Algo::RSAPKCS1v15), + _ => Err(Error::new( + napi::Status::GenericFailure, + "Unknown algorithm", + )), + } +} + impl TokenEncryption { pub fn new(private_key_data: Vec) -> Result { let data = String::from_utf8(private_key_data).map_err(TokenEncryptionError::FromUtf8)?; @@ -72,11 +91,11 @@ impl JsTokenEncryption { } #[napi] - pub fn decrypt(&self, parts: Vec) -> Result { + pub fn decrypt(&self, parts: Vec, algo: Algo) -> Result { let mut result = String::new(); for v in parts { - match self.decrypt_value(v) { + match self.decrypt_value(v, algo) { Ok(new_value) => { result += &new_value; Ok(()) @@ -90,13 +109,22 @@ impl JsTokenEncryption { Ok(result) } - fn decrypt_value(&self, value: String) -> Result { + fn decrypt_value(&self, value: String, algo: Algo) -> Result { let raw_value = Base64::decode_vec(&value).map_err(DecryptError::Base64)?; - let decrypted_value = self - .inner - .private_key - .decrypt(Pkcs1v15Encrypt, &raw_value) - .map_err(DecryptError::Decryption)?; + let decrypted_value = match algo { + Algo::RSAOAEP => { + let padding = Oaep::new::(); + self.inner + .private_key + .decrypt(padding, &raw_value) + .map_err(DecryptError::Decryption) + } + Algo::RSAPKCS1v15 => self + .inner + .private_key + .decrypt(Pkcs1v15Encrypt, &raw_value) + .map_err(DecryptError::Decryption), + }?; let utf8_value = String::from_utf8(decrypted_value).map_err(DecryptError::FromUtf8)?; Ok(utf8_value) } diff --git a/tests/tokens/tokenencryption.spec.ts b/tests/tokens/tokenencryption.spec.ts index d57aec9c..fcbb67d4 100644 --- a/tests/tokens/tokenencryption.spec.ts +++ b/tests/tokens/tokenencryption.spec.ts @@ -1,5 +1,5 @@ -import { TokenEncryption } from "../../src/libRs"; -import { RSAKeyPairOptions, generateKeyPair } from "node:crypto"; +import { Algo, TokenEncryption } from "../../src/libRs"; +import { RSAKeyPairOptions, generateKeyPair, publicEncrypt } from "node:crypto"; import { expect } from "chai"; describe("TokenEncryption", () => { @@ -8,6 +8,18 @@ describe("TokenEncryption", () => { async function createTokenEncryption() { return new TokenEncryption(await keyPromise); } + + async function legacyEncryptFunction(token: string) { + const MAX_TOKEN_PART_SIZE = 128; + const tokenParts: string[] = []; + let tokenSource = token; + while (tokenSource && tokenSource.length > 0) { + const part = tokenSource.slice(0, MAX_TOKEN_PART_SIZE); + tokenSource = tokenSource.substring(MAX_TOKEN_PART_SIZE); + tokenParts.push(publicEncrypt(await keyPromise, Buffer.from(part)).toString("base64")); + } + return tokenParts; + } before('generate RSA key', () => { // Generate this once since it will take an age. @@ -49,7 +61,7 @@ describe("TokenEncryption", () => { it('should be able to decrypt from a single part into a string', async() => { const tokenEncryption = await createTokenEncryption(); const value = tokenEncryption.encrypt('hello world'); - const result = tokenEncryption.decrypt(value); + const result = tokenEncryption.decrypt(value, Algo.RSAPKCS1v15); expect(result).to.equal('hello world'); }); it('should be able to decrypt from many parts into string', async() => { @@ -58,7 +70,7 @@ describe("TokenEncryption", () => { const tokenEncryption = await createTokenEncryption(); const value = tokenEncryption.encrypt(plaintext); expect(value).to.have.lengthOf(2); - const result = tokenEncryption.decrypt(value); + const result = tokenEncryption.decrypt(value, Algo.RSAPKCS1v15); expect(result).to.equal(plaintext); }); it('should support pkcs1 format keys', async() => { @@ -66,4 +78,9 @@ describe("TokenEncryption", () => { const result = tokenEncryption.encrypt('hello world'); expect(result).to.have.lengthOf(1); }); + it('should be to decrypt a string from the old crypto implementation', async() => { + const legacyString = await legacyEncryptFunction('hello world'); + const tokenEncryption = await createTokenEncryption(); + expect(tokenEncryption.decrypt(legacyString, Algo.RSAOAEP)).to.equal('hello world'); + }); });