nostr-protocol / nips

Nostr Implementation Possibilities
2.41k stars 586 forks source link

NIP-46 still uses NIP-04 encryption #1095

Open alexgleason opened 9 months ago

alexgleason commented 9 months ago

NIP-04 is considered harmful. It could leak your secret key. We did so much work to ensure we made the right choices this time in NIP-44 and even did a proper security audit.

NIP-46 still depends on the insecure encryption. This goes against the entire fundamental purpose of NIP-46, which is to protect your private key.

There was pushback against changing NIP-46 because it would result in breaking changes. But a breaking change to NIP-44 is necessary. Otherwise why did we do all of that? And what's the point of protecting your key using encryption that could reveal your key?

So now we are stuck: not wanting to change NIP-46 so it doesn't have breaking changes, but using an outdated and deprecated form of encryption that goes against the goals of NIP-46.

What should we do?

fiatjaf commented 9 months ago

I'll believe it is insecure when I see a stolen key.

alexgleason commented 9 months ago

I'll believe it is insecure when I see a stolen key.

Then why did we do all of that? I thought it was the whole point.

cc @paulmillr

paulmillr commented 9 months ago

@fiatjaf

I'll believe it is insecure when I see a stolen key.

Then it would be too late.

You know there are people who can steal your keys using radio, right? https://eprint.iacr.org/2015/170 Have you “seen” it?

Why did Signal and iMessage switch to post quantum cryptography? There are no pq computers today. Have you seen them?

@alexgleason i’m a security guy who saw a problem and wanted to get it done. If the community is not interested in this, there’s not much I can do 🤷‍♂️ It was complicated enough to even release nip44 out. I don’t want to spend more time agitating for “using” it.

what can be done by someone: modifying 46 to include proper encryption.

mikedilger commented 9 months ago

I wouldn't worry about the breaking change. NIP-46 was broken recently. What does it matter that we break it again? Just please document it in BREAKING.md (which isn't being kept up to date so far)

tyiu commented 8 months ago

For the kinds in NIP-46 that depend on NIP-04 encryption, we could just deprecate them in favor of new kinds. Not sure why there's pushback. NIP-04 is already officially unrecommended. So moving away from its usage is in line with that decision. The protocol is still relatively early in infancy so I don't think making breaking changes in how we do encryption to strengthen security (even if people believe it is theoretical in nature) is all that outlandish.

vitorpamplona commented 8 months ago

The only barrier is the lack of support for nip44 on extensions, but once they are there, nip46 should change as well.

And yes, let's do the change before it becomes too late.

I think the push back was more on the how to do it than on the migration itself.

VnUgE commented 7 months ago

Bump on this? I have been building noscrypt without nip04 encryption in the API in favor of nip44 all together. Id like to be phasing out "consensus based" insecure forms of cryptography as fast as possible in my libraries.

alexgleason commented 7 months ago

It's possible to detect whether the encrypted blob is nip44 or not just by looking at the first few characters.

If you're building a remote signer, I think you should detect it and support both. If you're building a client, I think you should accept an option like encryption?: 'nip04' | 'nip44' and default to nip04.

mikedilger commented 7 months ago

It's possible to detect whether the encrypted blob is nip44 or not just by looking at the first few characters.

I think you have to look for the "?iv=" string to detect NIP-04, and base64 decode and then look at the first few characters to detect the version of NIP-44.

mikedilger commented 7 months ago

The change needs to happen in 2 phases:

1) Everybody's code needs to be able to detect and decrypt either NIP-04 or NIP-44. Once we feel that has rolled out sufficiently, 2) Everybody's code starts producing NIP-44 encrypted data instead of NIP-04.

EDIT: Gossip has done (1) and is waiting for some community organiser (@fiatjaf ?) to nudge all the clients towards completing it so we can move on to step (2).

VnUgE commented 7 months ago

I think you have to look for the "?iv=" string to detect NIP-04, and base64 decode and then look at the first few characters to detect the version of NIP-44.

That is correct. There is no discriminating leading byte markers between 04 & 44 as utf8-encoded base64 text. '?' is not base64 and would cause a decoding error, so it needs to be parsed out first. If it does not exist, then it can be tested for nip-44, by decoding and looking for a leading version byte.

vitorpamplona commented 7 months ago

iv is always 24 chars, so don't "search" for it, just check the reference directly:

Given v as the NIP-04 string:

v[v.length-28] == '?' && v[v.length-27] == 'i' && v[v.length-26] == 'v' && v[v.length-25] == '='
alexgleason commented 6 months ago

I added NIP-44 support to my remote signer class in Nostrify: https://nostrify.dev/sign/connect#encryption

I did not end up automatically detecting the decrypt method. It just uses an option to switch the encryption used: https://gitlab.com/soapbox-pub/nostrify/-/blob/main/src/NConnectSigner.ts?ref_type=heads#L159

haorendashu commented 2 months ago

The secret key only use to decrypt the client-server msg here. Those key used to encrypt msgs (localSecretKey and remoteSignerSecretKey) both are temp key. They will delete when this connection disconect. So, i think it's not a big problem.

What i think matter is the client-server msgs, these msgs with a p tag contains remote-pubkey. I think it should change to the remote-signer-pubkey. It it better that we can don't expose the remote-pubkey to others.