Skip to content

perf(profile-sync-controller): Replace JS AES implementation with cryptography package #10621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
FrederikBolding merged 12 commits into from
Oct 1, 2026
Merged

perf(profile-sync-controller): Replace JS AES implementation with cryptography package #10621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7452189
chore(profile-sync-controller): Replace JS AES implementation with cr…
FrederikBolding Sep 30, 2026
fe4ce64
Fix lint
FrederikBolding Sep 30, 2026
9bf8c61
Update CHANGELOG
FrederikBolding Sep 30, 2026
836aad0
Restore nonce size constant
FrederikBolding Sep 30, 2026
8165e14
Update README
FrederikBolding Sep 30, 2026
9f91c23
Narrow Uint8Array types
FrederikBolding Sep 30, 2026
fd045a6
Fix SHA-256 output following usage of utils
FrederikBolding Oct 1, 2026
d6c135f
Fix tsconfig
FrederikBolding Oct 1, 2026
ee15675
Fix tsconfig
FrederikBolding Oct 1, 2026
5cc0186
Update suppressions
FrederikBolding Oct 1, 2026
0a3cbc4
Remove nonce size constant again
FrederikBolding Oct 1, 2026
8e2bd03
Bump version after rebase
FrederikBolding Oct 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@ linkStyle default opacity:0.5
profile_sync_controller --> address_book_controller;
profile_sync_controller --> base_controller;
profile_sync_controller --> controller_utils;
profile_sync_controller --> cryptography;
profile_sync_controller --> keyring_controller;
profile_sync_controller --> messenger;
profile_sync_controller --> seedless_onboarding_controller;
Expand Down
1 change: 1 addition & 0 deletions jest.config.packages.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ module.exports = {
'<rootDir>/../json-rpc-engine/src/v2/index.ts',
],
'^@metamask/utils/node$': ['<rootDir>/../utils/src/node.ts'],
'^@metamask/cryptography/(.+)$': ['<rootDir>/../cryptography/src/$1'],
'^@metamask/(.+)$': [
'<rootDir>/../$1/src',
// Some @metamask/* packages we are referencing aren't in this monorepo,
Expand Down
2 changes: 1 addition & 1 deletion oxlint-suppressions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5385,7 +5385,7 @@
"count": 1
},
"typescript/explicit-function-return-type": {
"count": 2
"count": 1
},
"typescript/no-unsafe-assignment": {
"count": 2
Expand Down
1 change: 1 addition & 0 deletions packages/profile-sync-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- Replace JS AES implementation with `@metamask/cryptography` ([#10621](https://github.com/MetaMask/core/pull/10621))
- Bump `immer` from `^9.0.21` to `^11.1.18` ([#10382](https://github.com/MetaMask/core/pull/10382))

### Fixed
Expand Down
2 changes: 1 addition & 1 deletion packages/profile-sync-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,13 @@
"@metamask/address-book-controller": "^8.0.0",
"@metamask/base-controller": "^10.0.0",
"@metamask/controller-utils": "^13.0.0",
"@metamask/cryptography": "^1.1.0",
"@metamask/key-tree": "^10.1.1",
"@metamask/keyring-controller": "^28.1.0",
"@metamask/messenger": "^3.0.0",
"@metamask/seedless-onboarding-controller": "^11.0.1",
"@metamask/superstruct": "^3.4.1",
"@metamask/utils": "^12.0.0",
"@noble/ciphers": "^1.3.0",
"@noble/curves": "^1.9.2",
"@noble/hashes": "^1.8.0",
"@signinwithethereum/siwe": "^4.2.1",
Expand Down
12 changes: 6 additions & 6 deletions packages/profile-sync-controller/src/shared/encryption/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import { SHARED_SALT } from './constants.js';
import { byteArrayToBase64 } from './utils.js';

type CachedEntry = {
salt: Uint8Array;
salt: Uint8Array<ArrayBuffer>;
base64Salt: string;
key: Uint8Array;
key: Uint8Array<ArrayBuffer>;
};

const MAX_PASSWORD_CACHES = 100;
Expand All @@ -14,7 +14,7 @@ const MAX_SALT_CACHES = 100;
* In-Memory Caching derived keys based from a given salt and password.
*/
type PasswordMemCachedKDF = {
[hashedPassword: string]: Map<string, Uint8Array>;
[hashedPassword: string]: Map<string, Uint8Array<ArrayBuffer>>;
};
let inMemCachedKDF: PasswordMemCachedKDF = {};
const getPasswordCache = (hashedPassword: string) => {
Expand All @@ -31,7 +31,7 @@ const getPasswordCache = (hashedPassword: string) => {
*/
export function getCachedKeyBySalt(
hashedPassword: string,
salt: Uint8Array,
salt: Uint8Array<ArrayBuffer>,
): CachedEntry | undefined {
const cache = getPasswordCache(hashedPassword);
const base64Salt = byteArrayToBase64(salt);
Expand Down Expand Up @@ -82,8 +82,8 @@ export function getCachedKeyGeneratedWithSharedSalt(
*/
export function setCachedKey(
hashedPassword: string,
salt: Uint8Array,
key: Uint8Array,
salt: Uint8Array<ArrayBuffer>,
key: Uint8Array<ArrayBuffer>,
): void {
// Max password caches
if (Object.keys(inMemCachedKDF).length > MAX_PASSWORD_CACHES) {
Expand Down
2 changes: 0 additions & 2 deletions packages/profile-sync-controller/src/shared/encryption/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// Nonce/Key Sizes
export const ALGORITHM_NONCE_SIZE = 12; // 12 bytes
export const ALGORITHM_KEY_SIZE = 16; // 16 bytes

// Scrypt settings
Expand Down
4 changes: 2 additions & 2 deletions packages/profile-sync-controller/src/shared/encryption/encryption.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,12 @@ describe('encryption tests', () => {
await expect(
encryption.decryptString(results[0], password2),
).rejects.toThrow(
'Unable to decrypt string - aes/gcm: invalid ghash tag',
'Unable to decrypt string - The operation failed for an operation-specific reason',
);
await expect(
encryption.decryptString(results[1], password1),
).rejects.toThrow(
'Unable to decrypt string - aes/gcm: invalid ghash tag',
'Unable to decrypt string - The operation failed for an operation-specific reason',
);
});

Expand Down
71 changes: 38 additions & 33 deletions packages/profile-sync-controller/src/shared/encryption/encryption.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { gcm } from '@noble/ciphers/aes';
import { randomBytes } from '@noble/ciphers/webcrypto';
import { decrypt, encrypt, IV_LENGTH } from '@metamask/cryptography/aes-gcm';
import {
getErrorMessage,
stringToBytes,
bytesToHex,
concatBytes,
remove0x,
} from '@metamask/utils';
import { scryptAsync } from '@noble/hashes/scrypt';
import { sha256 } from '@noble/hashes/sha256';
import { utf8ToBytes, concatBytes, bytesToHex } from '@noble/hashes/utils';

import type { NativeScrypt } from '../types/encryption.js';
import {
Expand All @@ -12,7 +17,6 @@ import {
} from './cache.js';
import {
ALGORITHM_KEY_SIZE,
ALGORITHM_NONCE_SIZE,
MAX_KDF_PROMISE_CACHE_SIZE,
SCRYPT_N,
SCRYPT_p,
Expand Down Expand Up @@ -53,7 +57,7 @@ class EncryptorDecryptor {
// Promise cache for ongoing KDF operations to prevent duplicate work
readonly #kdfPromiseCache = new Map<
string,
Promise<{ key: Uint8Array; salt: Uint8Array }>
Promise<{ key: Uint8Array<ArrayBuffer>; salt: Uint8Array<ArrayBuffer> }>
>();

async encryptString(
Expand All @@ -68,8 +72,7 @@ class EncryptorDecryptor {
nativeScryptCrypto,
);
} catch (e) {
const errorMessage = e instanceof Error ? e.message : JSON.stringify(e);
throw new Error(`Unable to encrypt string - ${errorMessage}`);
throw new Error(`Unable to encrypt string - ${getErrorMessage(e)}`);
}
}

Expand All @@ -93,8 +96,7 @@ class EncryptorDecryptor {
`Unsupported encrypted data payload - ${encryptedDataStr}`,
);
} catch (e) {
const errorMessage = e instanceof Error ? e.message : JSON.stringify(e);
throw new Error(`Unable to decrypt string - ${errorMessage}`);
throw new Error(`Unable to decrypt string - ${getErrorMessage(e)}`);
}
}

Expand All @@ -116,11 +118,11 @@ class EncryptorDecryptor {
);

// Encrypt and prepend salt.
const plaintextRaw = utf8ToBytes(plaintext);
const ciphertextAndNonceAndSalt = concatBytes(
const plaintextRaw = stringToBytes(plaintext);
const ciphertextAndNonceAndSalt = concatBytes([
salt,
this.#encrypt(plaintextRaw, key),
);
await this.#encrypt(plaintextRaw, key),
]);

// Convert to Base64
const encryptedData = byteArrayToBase64(ciphertextAndNonceAndSalt);
Expand Down Expand Up @@ -174,7 +176,7 @@ class EncryptorDecryptor {
);

// Decrypt and return result.
return bytesToUtf8(this.#decrypt(ciphertextAndNonce, key));
return bytesToUtf8(await this.#decrypt(ciphertextAndNonce, key));
}

getSalt(encryptedDataStr: string) {
Expand All @@ -198,8 +200,7 @@ class EncryptorDecryptor {
`Unsupported encrypted data payload - ${encryptedDataStr}`,
);
} catch (e) {
const errorMessage = e instanceof Error ? e.message : JSON.stringify(e);
throw new Error(`Unable to get salt - ${errorMessage}`);
throw new Error(`Unable to get salt - ${getErrorMessage(e)}`);
}
}

Expand All @@ -218,33 +219,37 @@ class EncryptorDecryptor {
return strSet.size === salts.length;
}

#encrypt(plaintext: Uint8Array, key: Uint8Array): Uint8Array {
const nonce = randomBytes(ALGORITHM_NONCE_SIZE);

async #encrypt(
plaintext: Uint8Array<ArrayBuffer>,
key: Uint8Array<ArrayBuffer>,
): Promise<Uint8Array<ArrayBuffer>> {
// Encrypt and prepend nonce.
const ciphertext = gcm(key, nonce).encrypt(plaintext);
Comment on lines -222 to -225

@mathieuartu mathieuartu Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not an expert on those matters, so please bear with me here. Genuine question: wasn't ALGORITHM_NONCE_SIZE of importance here?

@FrederikBolding FrederikBolding Oct 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metamask/cryptography uses a 12-byte nonce/IV as well! It is equivalent, just generated within the encryption function: https://github.com/MetaMask/core/blob/main/packages/cryptography/src/aes-gcm.ts#L5

@ccharly ccharly Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could have exported this constant? E.g we could have re-used it while decoding the bytes on the other part of the code?

So we avoid having drifiting values for ALGORITHM_NONCE_SIZE and AES_GCM_IV_LENGTH

@mathieuartu mathieuartu Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense, thanks! But then, if it changes one day in @metamask/cryptography, wouldn't we start having problems here:

async #decrypt(
    ciphertextAndNonce: Uint8Array<ArrayBuffer>,
    key: Uint8Array<ArrayBuffer>,
  ): Promise<Uint8Array<ArrayBuffer>> {
    // Create buffers of nonce and ciphertext.
    const nonce = ciphertextAndNonce.slice(0, ALGORITHM_NONCE_SIZE);
    const ciphertext = ciphertextAndNonce.slice(
    ...

The fact that rely both on a "silent" external constant AND a local one is making me a bit uncomfortable. WDYT?

@FrederikBolding FrederikBolding Oct 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, we can export it and re-use!

@FrederikBolding FrederikBolding Oct 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though tbf this shouldn't change, the defacto standard is 12 bytes.

@ccharly ccharly Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that's right, I feel like it still helps with the code here (especially for the encoding/decoding part)

@mathieuartu mathieuartu Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and am inclined to approve as-is. But for the sake of future readers, this asymmetry should probably be fixed in a follow up PR.

@FrederikBolding FrederikBolding Oct 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0a3cbc4

const { ciphertext, iv } = await encrypt(key, plaintext);

return concatBytes(nonce, ciphertext);
return concatBytes([iv, ciphertext]);
}

#decrypt(ciphertextAndNonce: Uint8Array, key: Uint8Array): Uint8Array {
async #decrypt(
ciphertextAndNonce: Uint8Array<ArrayBuffer>,
key: Uint8Array<ArrayBuffer>,
): Promise<Uint8Array<ArrayBuffer>> {
// Create buffers of nonce and ciphertext.
const nonce = ciphertextAndNonce.slice(0, ALGORITHM_NONCE_SIZE);
const nonce = ciphertextAndNonce.slice(0, IV_LENGTH);
const ciphertext = ciphertextAndNonce.slice(
ALGORITHM_NONCE_SIZE,
IV_LENGTH,
ciphertextAndNonce.length,
);

// Decrypt and return result.
return gcm(key, nonce).decrypt(ciphertext);
return decrypt(key, nonce, ciphertext);
Comment on lines +233 to +244

@ccharly ccharly Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: But I think we can use the right wording now, I'd go with Iv/iv instead of Nonce/nonce

@FrederikBolding FrederikBolding Oct 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are interchangeable tbh, do you feel strongly about this?

@ccharly ccharly Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that much 😄 I usually see IV rather than nonce in the context of AES, but that's not incorrect neither

}

async #getOrGenerateScryptKey(
password: string,
o: EncryptedPayload['o'],
salt?: Uint8Array,
salt?: Uint8Array<ArrayBuffer>,
nativeScryptCrypto?: NativeScrypt,
) {
): Promise<{ key: Uint8Array<ArrayBuffer>; salt: Uint8Array<ArrayBuffer> }> {
const hashedPassword = createSHA256Hash(password);

// Check if we already have the key cached
Expand Down Expand Up @@ -318,11 +323,11 @@ class EncryptorDecryptor {
async #performKdfOperation(
password: string,
o: EncryptedPayload['o'],
salt: Uint8Array,
salt: Uint8Array<ArrayBuffer>,
hashedPassword: string,
nativeScryptCrypto?: NativeScrypt,
): Promise<{ key: Uint8Array; salt: Uint8Array }> {
let newKey: Uint8Array;
): Promise<{ key: Uint8Array<ArrayBuffer>; salt: Uint8Array<ArrayBuffer> }> {
let newKey: Uint8Array<ArrayBuffer>;

if (nativeScryptCrypto) {
newKey = await nativeScryptCrypto(
Expand All @@ -334,12 +339,12 @@ class EncryptorDecryptor {
o.dkLen,
);
} else {
newKey = await scryptAsync(password, salt, {
newKey = (await scryptAsync(password, salt, {
N: o.N,
r: o.r,
p: o.p,
dkLen: o.dkLen,
});
})) as Uint8Array<ArrayBuffer>;
}

setCachedKey(hashedPassword, salt, newKey);
Expand All @@ -362,5 +367,5 @@ export default encryption;
*/
export function createSHA256Hash(data: string): string {
const hashedData = sha256(data);
return bytesToHex(hashedData);
return remove0x(bytesToHex(hashedData));
}
2 changes: 1 addition & 1 deletion packages/profile-sync-controller/src/shared/types/encryption.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ export declare type NativeScrypt = (
r: number,
p: number,
size: number,
) => Promise<Uint8Array>;
) => Promise<Uint8Array<ArrayBuffer>>;
3 changes: 3 additions & 0 deletions packages/profile-sync-controller/tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
{
"path": "../base-controller/tsconfig.build.json"
},
{
"path": "../cryptography/tsconfig.build.json"
},
{
"path": "../keyring-controller/tsconfig.build.json"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/profile-sync-controller/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
{
"path": "../base-controller"
},
{
"path": "../cryptography/"
},
{
"path": "../controller-utils"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/profile-sync-controller/tsconfig.lint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
},
{
"path": "../controller-utils/tsconfig.lint.json"
},
{
"path": "../cryptography/tsconfig.lint.json"
}
]
}
1 change: 1 addition & 0 deletions tsconfig.packages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"@metamask/profile-sync-controller/user-storage": [
"${configDir}../profile-sync-controller/src/controllers/user-storage/index.ts"
],
"@metamask/cryptography/*": ["${configDir}/../cryptography/src/*.ts"],
"@metamask/utils/node": ["${configDir}/../utils/src/node.ts"],
"@metamask/*": ["${configDir}/../*/src/index.ts"]
}
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6396,7 +6396,7 @@ __metadata:
languageName: node
linkType: hard

"@metamask/cryptography@workspace:packages/cryptography":
"@metamask/cryptography@npm:^1.1.0, @metamask/cryptography@workspace:packages/cryptography":
version: 0.0.0-use.local
resolution: "@metamask/cryptography@workspace:packages/cryptography"
dependencies:
Expand Down Expand Up @@ -8291,6 +8291,7 @@ __metadata:
"@metamask/auto-changelog": "npm:^6.2.1"
"@metamask/base-controller": "npm:^10.0.0"
"@metamask/controller-utils": "npm:^13.0.0"
"@metamask/cryptography": "npm:^1.1.0"
"@metamask/eth-hd-keyring": "npm:^15.0.0"
"@metamask/key-tree": "npm:^10.1.1"
"@metamask/keyring-api": "npm:^24.0.0"
Expand All @@ -8302,7 +8303,6 @@ __metadata:
"@metamask/seedless-onboarding-controller": "npm:^11.0.1"
"@metamask/superstruct": "npm:^3.4.1"
"@metamask/utils": "npm:^12.0.0"
"@noble/ciphers": "npm:^1.3.0"
"@noble/curves": "npm:^1.9.2"
"@noble/hashes": "npm:^1.8.0"
"@signinwithethereum/siwe": "npm:^4.2.1"
Expand Down
Loading