Skip to content
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.

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

Eliminate key import helper methods #47

Open
steveluscher opened this issue Jan 2, 2025 · 10 comments
Open

Eliminate key import helper methods #47

steveluscher opened this issue Jan 2, 2025 · 10 comments
Labels
enhancement New feature or request

Comments

@steveluscher
Copy link
Collaborator

steveluscher commented Jan 2, 2025

Motivation

Any time private key bytes are handled in JavaScript code, a new attack surface is created. In other words, malicious code inserted in the codepaths through which private key bytes flow can result in a leak of those bytes. This exact scenario unfolded in December of 2024.

This issue proposes to remove those codepaths from @solana/web3.js to eliminate those attack surfaces.

Example use case

Before

An application that needs to handle private key material for a purpose including but not limited to:

  • importing a key from a file or environment variable
  • importing a key supplied by a user
  • importing a key derived from a seed phrase

…might use one of the extant helpers in @solana/keys such as:

  • createPrivateKeyFromBytes()
  • createKeyPairFromBytes()
  • createKeyPairFromPrivateKeyBytes()

…or one of the extant helpers in @solana/signers such as:

  • createKeyPairSignerFromBytes()
  • createKeyPairSignerFromPrivateKeyBytes()

Furthermore, an application that wants to derive the public key from some private key bytes might create an extractable private key for use with the getPublicKeyFromPrivateKey() method of @solana/keys.

After

Instead of using any of those methods, which we would delete, developers would import private key material by calling the built-in SubtleCrypto::importKey with a PKCS#8 formatted key:

const privateKey = await crypto.subtle.importKey(
    'pkcs8',
    new Uint8Array([...PKCS8_ED25519_HEADER, ...privateKeyBytes]),
    'Ed25519', /* algorithm */
    false, /* extractable */
    ['sign'], /* keyUsages */
);

To derive a public key from private key bytes, developers would do the following:

// Create an extractable private key
const privateKey = await crypto.subtle.importKey(
    'pkcs8',
    new Uint8Array([...PKCS8_ED25519_HEADER, ...privateKeyBytes]),
    'Ed25519', /* algorithm */
    true, /* extractable */
    ['sign'], /* keyUsages */
);

// Export it as a JWK (Java Web Key)
const jwk = await crypto.subtle.exportKey('jwk', privateKey);

// Import the JWK as a public key
const publicKey = await crypto.subtle.importKey(
    'jwk',
    {
        crv /* curve */: 'Ed25519',
        ext /* extractable */: true,
        key_ops /* key operations */: ['verify'],
        kty /* key type */: 'OKP' /* octet key pair */,
        x /* public key x-coordinate */: jwk.x,
    },
    'Ed25519', /* algorithm */
    true, /* extractable */
    ['verify'], /* keyUsages */
);

Q&A

How does this make anything more secure?
Removing these codepaths from @solana/web3.js moves the attack surface from a system that developers don't control (ie. @solana/web3.js on the Node Package Manager, npm) to a system that they do control (ie. their own source control and deployment system).

What's to stop someone from re-establishing the attack surface on npm by publishing the old helpers as a new package?
Nothing.

Isn't it better if these helpers are part of the ‘official’ package?
If everyone uses the same helpers to handle private key material, there develops a single point of failure to compromise all apps. In a world where there are several helpers to choose from, and where some developers choose to roll their own, a compromise of any one codebase should in theory impact fewer apps.

This is not a question, but this makes it harder for me to import keypairs.
It does. Ideally, your application should not handle key material at all. It could instead delegate signing operations to a wallet, or a hardware/software security module, so as to increase your isolation from JavaScript supply chain attacks.

Details

  • delete createPrivateKeyFromBytes(), createKeyPairFromBytes(), and createKeyPairFromPrivateKeyBytes() from @solana/keys.
  • delete createKeyPairSignerFromBytes() and createKeyPairSignerFromPrivateKeyBytes() from @solana/signers.
  • export the PKCS#8 header for an Ed25519 key as PKCS8_ED25519_HEADER from @solana/keys, for convenience.
  • document the above use cases in the README of @solana/keys and optionally @solana/signers.
@steveluscher steveluscher added the enhancement New feature or request label Jan 2, 2025
@steveluscher
Copy link
Collaborator Author

cc/ Contentious proposal notification: @nickfrosty, @lorisleiva, @buffalojoec, @mcintyre94, @jsx

@lorisleiva
Copy link
Member

I'm not against this proposal. I think it's super valuable to be able to say "this attack couldn't have happened on v2".

However, this is the very first thing people do when they try a new library: create a key pair, airdrop some money and send a transaction. This is a building block for testing programs e2e.

I am convinced that someone else will end up creating a mini library for these helpers, moving the single point of failure elsewhere. Therefore we would just pass the bucket to someone else.

@buffalojoec
Copy link

Removing these codepaths from @solana/web3.js moves the attack surface from a system that developers don't control (ie. @solana/web3.js on the Node Package Manager, npm) to a system that they do control (ie. their own source control and deployment system).

In general, this isn't a bad rule of thumb. However, you could make this argument about SubtleCrypto as well. There's the small but not impossible chance of a vulnerability in Mozilla's library, and then there's the polyfill and the libraries it uses.

If everyone uses the same helpers to handle private key material, there develops a single point of failure to compromise all apps. In a world where there are several helpers to choose from, and where some developers choose to roll their own, a compromise of any one codebase should in theory impact fewer apps.

This point makes the proposed approach hold a little more weight. Only those downstream applications who used custom helpers to create CryptoKeyPair instances which are extractable would be at risk. @solana/keys would still support generateKeyPair, and a supply chain hacker could make those generated key pairs extractable, but that's still a smaller attack spread.

As a side note, I think generateKeyPair is much more useful than createKeyPairFromBytes (and similar), but it would still stink for developers to not be able to easily create Ed25519 keypairs through an API on @solana/keys. Literally everyone will copy-paste the code snippet you put here in every test suite.

Can we get the best of both worlds with a separate library? One that:

  • Does not ship with the base library.
  • Is labeled as handling sensitive secret key bytes.
  • Is marketed as primarily recommended for testing only.

@nickfrosty
Copy link

nickfrosty commented Jan 3, 2025

I understand the rationale for wanting to remove these functions from the "default libraries", and like Loris also pointed out: there is value in being able to say "this could not happen with v2". But...

I am against removing them from the library entirely. For the following reasons:

  • like Loris pointed out: generating and using a keypair is one of the first things most new devs do, and is also commonly done in tests
  • with the updated and VERY restrictive npm publishing practices we migrated to after the v1 hack, I would argue that someone being able to modify these underlying functions becomes very difficult for an attacker (we could even make it harder like requiring multiple PR approvers to merge a PR that modifies key material functions)
  • someone(s) will surely create a helper function(s) elsewhere that the ecosystem would adopt. moving the attack surface to that repo, which almost surely will have worse security practices (or more likely none at all), making the ecosystem very vulnerable. vice being published more safely via this repo and CI/CD
  • the developer experience would be degraded, making it more complex for devs to perform common tasks (making them to recreate and publish those helper functions, mentioned above)

edit: So I am against removing them from this library, and I think it is actually more secure for the ecosystem for them to remain published via this monorepo

@steveluscher
Copy link
Collaborator Author

…the very first thing people do when they try a new library: create a key pair, airdrop some money and send a transaction.
…but it would still stink for developers to not be able to easily create Ed25519 keypairs through an API on @solana/keys.
…generating and using a keypair is one of the first things most new devs do, and is also commonly done in tests

You would still be able to do that. Key generation helpers would be unaffected. This includes necessary key generation helpers for account creation, where creating the account relies on the account's own ability to sign for itself. Only key import helpers would be removed.

@steveluscher
Copy link
Collaborator Author

At some level, we've designed this version of web3.js to ‘use the platform.’ This means JavaScript's native Ed25519 crypto API, SubtleCrypto. Love it or hate it, it's ‘how you do Ed25519 in JavaScript.’

As the SubtleCrypto APIs become more ergonomic to use, the utility of our userspace wrappers will become less and less.

@mcintyre94
Copy link

mcintyre94 commented Jan 6, 2025

I'm not necessarily against this proposal, but when we say "this could not happen with v2" what exactly are we referring to? There would still be many attack vectors if an attacker could publish a version of this library that (I think) could cause at least as bad a result as we saw previously, even if it's not the exact same attack.

  • We have a library that polyfills CryptoKey, if you want it to, extremely carefully. An earlier version only required importing it, before we had the install function. So couldn't an attacker just make importing our library polyfill CryptoKey to send them all private key material? Obviously that's not unique to our library and could happen to any NPM package, but it's the same result.
  • I think the attacker previously injected into Keypair.generate too. There's obviously a lot of noise there, and maybe it's never used for anything meaningful, but you could still do that if we have helpers like generateKeyPair
  • We'd still have functions to use the CryptoKey to sign a transaction, even if you can't extract the bytes. A sophisticated attacker could modify transaction signing to inspect the signer, and if it's worthwhile to swap out the transaction for something that steals funds. I agree that's not as bad as losing keys, but this happening to one big account is effectively the same impact as what we saw before.

I also agree that any third party helper libraries would likely be less secure than ours. But there's still a benefit to dispersing the responsibility, not tying it to such a core library for the ecosystem, and, more selfishly, it not being us.

I had the same thought about a separate library, just making it not part of the npm install @solana/web3.js would I think cause it to be treated a bit differently. But it'd become that central point where everyone who imports keypairs gets the helpers to do that. I don't think people would take much notice if we said to only use it for testing either.

@steveluscher
Copy link
Collaborator Author

…you could still do that if we have helpers like generateKeyPair

Mmm. True.

We have a library that polyfills CryptoKey…

True, but it's lifespan is limited to {however long it takes for Chrome to turn it on by default}.

@AlaaZorkane
Copy link

I understand the security concerns driving this proposal, but I believe removing these helper methods would be detrimental to the DX ecosystem imo for several reasons:

  • Developers creating their own implementations, potentially with worse security practices
  • A fragmented ecosystem of key management utilities as nick mentioned
  • Increased friction in the development process, especially for new developers

The current helper methods serve as crucial stepping stones for developers learning Solana development. When developers first interact with web3.js, they often start by experimenting with key pairs, sending test transactions, and understanding the fundamentals. Making this initial experience more complex (or fragmented if they opt for 3rd party libraries) by requiring them to learn CryptoKey extraction and raw key import processes creates an unnecessary barrier to entry. (And yes, I understand that learning the platform is important, and eventually a developer has to, but abstraction while already learning something new, v2 here, is nice)

The current helper methods abstract away these low-level details, allowing developers to focus on learning core Solana concepts rather than getting bogged down in key material handling specifics.

Ironically, I came across this issue while trying to search for issues history on these helpers because I was going to suggest to allow an optionally passed extractable (that defaults to false of course) here:

https://github.com/anza-xyz/solana-web3.js/blob/8111c88f3b98503ce445bac9c761f33d584fc5de/packages/keys/src/key-pair.ts#L14C2-L21C2

I am just testing and experimenting, not currently building a production app with v2, so I will probably copy paste all that generateKeyPair() code and force extractable.


A bit off-topic, and maybe extreme for this case, but a critical lesson from blockchain history that often goes unmentioned is how technical choices can inadvertently kill adoption through fragmented DX. Take Cardano - while choosing Haskell for smart contract development seemed theoretically sound, it created what the community often calls a massive "skill issue". Despite having solid technology, many developers simply couldn't or wouldn't deal with the steep learning curve of both blockchain concepts AND Haskell.

Nevertheless, I want to acknowledge what an impressive achievement web3.js v2 represents - making functional programming patterns feel natural and approachable is no small feat. Given the thoughtful architecture you've created, I'm confident you can find elegant solutions that enhance security while preserving the excellent developer experience this library will surely be known for :)

@mcintyre94
Copy link

True, but its lifespan is limited to {however long it takes for Chrome to turn it on by default}.

It's not actually our polyfill or its lifetime I'm concerned about, it's the fact that a JS library can do what we do in that polyfill.

AFAICT that means an attacker with publish access to the web3js code base could polyfill CryptoKey with a backdoor. That would allow them to steal key material for all created/imported keys regardless what helpers/no helpers you're using.

I guess what I'm really saying is that the whole platform here is fatally insecure against malicious packages and I'm not sure we can actually achieve "this can't happen on v2" in any meaningful sense by just removing functionality from our API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants