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

NIP-49: Private key encryption #133

Merged
merged 23 commits into from
Jan 29, 2024

Conversation

mikedilger
Copy link
Contributor

This is a scheme for exporting/importing a private key and sharing it on nostr so that it can be moved between clients securely without exposing it (displaying it, cutting and pasting it, etc).

This is currently implemented in gossip for saving/loading to its internal database. But if other clients implement this I will make a gossip function to export/import from other clients.

@mikedilger
Copy link
Contributor Author

As for the 11 bytes, I pulled them out of my arse. The point was to verify that a decryption is correct. Due to AES needing padding, most failed decryptions will result in an AES padding error, but I still like having these 11 bytes for more certainty.

@mikedilger
Copy link
Contributor Author

Changes made based on feedback from SPA.

I increased the number of rounds and added a version number. SPA mentioned use of 10,000,000 rounds in borderwallets.com. OWASP recommended 310,000 rounds for password hashing in 2021.

I based the number of rounds (100,000) on what would require one core of my computer (AMD Ryzen 9 5900X) to compute for about a second. We can increase it more if people lean that way.

I also made the salt random, however I think it is unnecessary. The private key already salts the hell out of the password. But leaving it out makes some people feel less secure and it doesn't hurt to throw in a random salt. It also made a convenient place to put a versioning byte in case we need it.

@fiatjaf
Copy link
Member

fiatjaf commented Jan 7, 2023

Who is SPA?

I like the idea of having private keys be encrypted according to a standard. But I would like it even more if they were encoded like the NIP-19 entities, bech32 with a different prefix, say, ncryptsec (I don't know), just so everything follows the same pattern. Or is this a bad idea?

I didn't read this very carefully, but I think it would be just taking whatever the bytes we have here at the end and bech32ing them.

Another thing I would like to standardize is a location in the computer to store encrypted private keys so multiple apps could access them, but I guess this is another topic, and one that is much more hairy.

@mikedilger
Copy link
Contributor Author

SPA: 4a38463c2a75e68c24416e7720a3b3befbb0ea6872d5a04692c39e18e8f2dcac , someone that contacted me on nostr about it.

I like the ncryptsec bech32 thing. We could drop the base64 encoding and switch to that instead. Keeps things more consistent. I'll make the change.

@mikedilger mikedilger changed the title Key export/import as implemented by gossip NIP-49 Encrypted Private Key export/import Jan 14, 2023
@mikedilger
Copy link
Contributor Author

mikedilger commented Jan 15, 2023

I'm going to respond to comments from the twitterati.

https://twitter.com/ColbySerpa/status/1614233927312605184 This one is off topic. It's a suggestion to tightly tie nostr to bitcoin, the benefit of which seems rather nil to me.

https://twitter.com/4moonsettler/status/1614607087568392193
Apparently there is a BIP-38 that aims to do something similar https://github.com/bitcoin/bips/blob/master/bip-0038.mediawiki I will look at it later.
The suggestion to use Diffie Hellman isn't applicable. There is no online back-and-forth protocol between old client and new client. In fact, when "old client" writes the encrypted key to a relay, the user may have no idea what his new client is going to be yet. Further, if they somehow established a secure channel, that doesn't authenticate the new client to the old client, it just creates a secure channel to an unknown party.

https://twitter.com/pandrewhk/status/1614245968597352449
Again on the Diffie Hellman. He is right this is for "non-interactive backups". It is what gossip uses to store the key in its local database. Avoiding private key transfer over the protocol would be great, but nostr doesn't support mutiple-keys-per-identity and I wasn't trying to crack that nut.

https://twitter.com/isaacfain/status/1614264974012039169
This looks like a better comment, but at first glance I think he's wrong about most of what he said. He doesn't like that I appended a checksum in step 1, then he doesn't like that there is no checksum on the input before encryption. Maybe he didn't have his coffee yet. And where is the IV reuse? I'm using IV in the standard way, am I not? Show me how I'm not using IV exactly how you are supposed to? If you don't append the IV to the ciphertext, it cannot be deciphered. Notice all the examples on this page https://docs.rs/cbc/latest/cbc/ require an IV to decrypt. How can you get the IV if you didn't append it? The 'bool' creates 7 bits of known plaintext, maybe it could change to a single bit. XChaCha-Poly1305 > AES is probably correct but I'm not going to get my panties in a twist about it. And bech32 is more uniform with the rest of nostr and has nothing to do with the crypto part.

https://twitter.com/samecwilliams/status/1614492981943480323
This guy has a good idea. That is to let the user choose how many cycles to put PBKDF2 through. They can ramp it up and spend half an hour encrypting their private key if they want, or do like I do and encrytped it in less than a second. So I will add this. Someone go like his post for me.

https://twitter.com/sj_mackenzie/status/1614536989239566337
This guy wants 1 million PBKDF2 cycles. Given the previous comment, he can have that. It slows down password cracking so if you want to use terribly short guessable passwords I guess you can mitigate that by ramping up the PBKDF2. I use long hard to crack passwords with many many bits. But to each his own. As for XChaCha-Poly1305 versus AES, everybody has their favorites. AES isn't my favorite, I was just reusing something used elsewhere in nostr.

Nobody mentioned that the salt was useless. I would expect a good cryptographer to notice that. So these people on Twitter are not good cryptographers, or simply didn't spend any real time on this. I bet if I didn't put it in there, though, someone would point out it was missing.

And now a point from me: When people are cutting-and-pasting private keys around on their computer and into web pages on their browser, nobody seems to see anything wrong with that. But as soon as you suggest a much more secure method of private key handling, because it's not quite perfect everybody gets their panties in a twist. Funny how people are.

Happy to take feedback on my feedback.

@mikedilger
Copy link
Contributor Author

Actually isaacfain did say one thing of use I didn't comment on. Forcing 11 known bytes into the plaintext helps an attacker because they can quickly determine in each iteration if they cracked your AES or not. It would be better not to have known plaintext. The sacrifice is that you couldn't detect if the decryption succeeded or not. Maybe a 'checksum' is better because it is not a fixed known sequence. Perhaps it is I that hadn't had my coffee yet.

@mikedilger
Copy link
Contributor Author

mikedilger commented Jan 15, 2023

So I propose to change this scheme to work like this instead (I'll write it up later):

  • Use XChaCha20-Poly1305 -- people like this better, it's less associated with the US government, it's faster (in software) and the Poly1305 creates a checksum. The checksum is useful, but we have no 'associated data' to include.
  • Encrypt ONLY the 32-byte nostr private key with nothing else inside of the encryption, so there can be no concerns about partial known plaintext attacks.
  • Put the key security flag outside of the encryption
  • Specify a user-selectable number of rounds for PBKDF2 based on powers of 2, recommending from 16-24 or so, but letting the user decide.
  • Drop the salt. It's purpose (making multiple hashes of the same password all different) makes no difference here because we are using PBKDF2 for key derivation not for password hashing.

@fiatjaf
Copy link
Member

fiatjaf commented Jan 17, 2023

I think the scheme shouldn't use too much arcane stuff or be too complex otherwise no one is going to implement it.

@mikedilger
Copy link
Contributor Author

I think the scheme shouldn't use too much arcane stuff or be too complex otherwise no one is going to implement it.

So... remove the bech32 encoding? 😝

My other reply could be: That is why I made it simple.

XChaCha20-Poly1305 and PBKDF2 are top-line very standard and common cryptographic algorithms that BTW nobody should try to implement, they should use a cryptographic library.

If someone wants to come up with a different scheme though, I'm all ears. The scheme of the original PR was born out of necessity, and modified to suit others.

@fiatjaf
Copy link
Member

fiatjaf commented Jan 17, 2023

Yes, I was thinking about using a library.

I didn't mean to say this scheme was not simple, I didn't read it fully or thought about the implementation, I just got afraid, by reading all the names of these algorithms, that if I were to implement it I would have to import a bunch of new libraries which would come with a lot of dependencies. And in Scala Native I would have to write C bindings to random C libraries that implement these things that I would find on GitHub from unknown developers.

@mikedilger
Copy link
Contributor Author

mikedilger commented Jan 17, 2023

I think of it like 2 steps (2 algorithms) but technically it's 5. PBKDF2 using HMAC-SHA-256 is essentially a single process, but technically using three algorithms in a pattern. And XChaCha20-Poly1305 is a second single process, but technically a combination of an encryption algorithm and a digital signature algorithm. Generally if a library has PBKDF2 it has the other two (because that is by far the most common way to run PBKDF2). And if a library has XChaCha20 it has the Poly1305 (because that is commonly used to use XChaCha20 as AEAD (authenticated encryption with associated data))

I understand not wanting to pull in a bunch of libraries you aren't already pulling in.

BIP-38 solves the same problem: passphrase encryption of private key material. PKCS#8 also solves the same thing.

BIP-38 uses scrypt (which is like PBKDF2 but less preferred by cryptographers I may be wrong about this. Now I'm reading somewhere that scrypt is stronger, but also about argon2 and balloon hashing being even better. Remember that I am not a cryptographer! Sorry.), it uses salting (I skipped salting because it makes no sense to me in this context), it uses AES256 (again a less preferred algorithm, but really not bad at all IMHO),
does some splitting and XORing again which I do not understand the purpose of, and some base58 encoding (weird bitcoin world stuff, but probably in your library).

It also has a mode where you cannot encrypt an existing private key, you can only encrypt-generate together, and uses some secp256k1 magic that I do not understand to do it. That offers some assurance that nobody knows the seed words because you just generated it.

scrypt and AES256 aren't terrible. They were probably the right choice in 2012. PBKDF2-HMAC-SHA256 and argon2 and XChaCha20-Poly1305 are probably the right choice today if cryptographic strength was the only concern.

EDIT: From what I'm reading today, scrypt is newer and better than PBKDF2, although PBKDF2 is still in widespread usage. Argon2 is better than both of those.

Finally (and sorry this is so long), I'm having second thoughts about suggesting that people publish these encrypted private keys. Because there will be people who use bad passphrases that will have their identity stolen.

@mikedilger
Copy link
Contributor Author

The last changes I made to this specified scrypt for slow KDF and XChaCha20--Poly1305 for encryption. This has been in use in gossip for some time, albeit in gossip we hard coded the scrypt parameter n=18 (whereas this standard says the user should be able to choose).

README.md Outdated Show resolved Hide resolved
@fiatjaf fiatjaf changed the title NIP-49 Encrypted Private Key export/import NIP-49: Private key encryption Jan 26, 2024
Copy link
Member

@staab staab left a comment

Choose a reason for hiding this comment

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

Let's fix the readme to remove the weird 10002 line and change the nip name to match the nip.

@pablof7z
Copy link
Member

Oh this is awesome; going to implement this scheme in nsecbunker

@DanConwayDev
Copy link
Contributor

Using good encryption standards is important for encrypting a secret with a potentially weak password but I'm not sure there should a nostr specific standard. The string shouldn't be shared so there is no need for cross compatibility. I suppose if the standard is not here then where should it be?

Have you implemented this in ngit @DanConwayDev?

Mostly - thanks for the good work on this @mikedilger.

To quote from my commit message: DanConwayDev/ngit-cli@96660a9

The approach to encryption draws heavily from that used by the gossip
nostr client.
...
There is UX trade-off between decryption speed and key-stretching
computation. This UX challenge is exacerbated in a cli tool as
decryption must take place more regularly. Thought was put into the
selected n_log and a heavily reduced value is provided for long
passwords where security benefits are smaller.

A more granular reducing in computation was also considered by
rejected to avoided to revealing just how weak a password is as most
weak passwords are reused.

I use 0x1 for the version number but most notably a log_n of 15 or 1 if the password is > 20 characters.

I'd recommend some flexibility around log_n depending on device and usage pattern.

The rust scrypt library recommended 17 at the time of my commit. It will need to go up as computing power increases.

README.md Outdated Show resolved Hide resolved
@AsaiToshiya
Copy link
Collaborator

Do we also need to add ncryptsec to NIP-19 after merging this PR?

@fiatjaf
Copy link
Member

fiatjaf commented Jan 27, 2024

@AsaiToshiya I think we shouldn't. At least for my libraries I decided it didn't make sense.

@fiatjaf
Copy link
Member

fiatjaf commented Jan 27, 2024

@DanConwayDev I don't get it. The current scheme already supports a flexible log_n number. What did you change in your implementation?

@DanConwayDev
Copy link
Contributor

@DanConwayDev I don't get it. The current scheme already supports a flexible log_n number. What did you change in your implementation?

I somehow missed the 'Symmetric Encryption Key derivation' section. That will teach me for reviewing something after 1am. I'm more of a morning person.

@fiatjaf fiatjaf merged commit 7ec0603 into nostr-protocol:master Jan 29, 2024
@vitorpamplona
Copy link
Collaborator

Hey @mikedilger @fiatjaf I noticed that you two have tests in the Go version of this with empty passwords. Do you plan to allow empty passwords in this scheme?

It looks like the behavior of an empty password is undefined in the Java version of Scrypt.

@fiatjaf
Copy link
Member

fiatjaf commented Feb 14, 2024

Good question. I don't know. I think we should allow and I don't see why it would be undefined there, but I'll let @mikedilger answer.

What library is that, if I may ask?

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 14, 2024

Lib is https://github.com/wg/scrypt/blob/master/src/main/java/com/lambdaworks/crypto/SCrypt.java

But the error comes from the Android implementation of HMAC that hashes the password before the pbkdf. Looks like an empty string is not a valid key for the HmacSHA256

@vitorpamplona
Copy link
Collaborator

Kotlin implementation otherwise seems to work well: vitorpamplona/amethyst@b94c1e2

@mikedilger
Copy link
Contributor Author

I think an empty key is allowable, but definitely not recommended.

There is a way to work around the java library restriction: https://stackoverflow.com/questions/56969153/can-the-key-be-empty-while-generating-hmacsha256-using-java

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

Successfully merging this pull request may close these issues.