Improvement to 04.md #572

earonesty · 2023-05-30T13:46:18Z

Remove warning about "unsafe practices" and instead list exactly what the unsafe information is, and the nature of the safety (archival storage with size and pubkey leakage)
Add hkdf, random salt, and use GCM - making the key clean and the cipher operate in accordance with archival data standards
Update impl sample to include backward compatible decryption
Priorities are
- Fix the gaping security vulns in the existing implementation
- Use standards
- Make it as simple as possible for existing clients to upgrade
Backward, not forward compatible DM's
- Old clients fail to show DM's from new - but should see that an attempt was made
- New clients can show DM's from old and new
Optional blind-signing prevents metadata leakage

An alternative would be to create another kind. Would appreciate reasoning as to why this would benefit users or builders. If consensus is "new kind", will open up a new PR

Remove warning about "unsafe practices" and instead show exactly what the unsafe information is. Add in hkdf, random salt, and use GCM - making the key clean and the cipher operate in accordance with archival data standards.

paulmillr · 2023-05-30T13:57:21Z

This should not be NIP4, it should be a separate nip, because it's totally different and NOT compatible with the old one. Servers signalling for NIP4 would not be bogus: is it GCM, or CBC? Also AES-GCM cannot decrypt AES-CBC encrypted messages.

Overall the cryptography choice is bad. I have opened pull request for (allegedly) NIP-44 Encrypted Direct Message (Versioned): #574

earonesty · 2023-05-30T14:07:10Z

i think it might be better for old clients to simply experience an error when seeing new data, and then update the client. i really don't like all those weak iv's and unhashed keys sitting around. i removed any leftover refs to cbc. easier to rev if you comment on the change and we can resolve it. in the future all new versions can report a much nicer error. i added simple padding so you can't trivially attack from short messages based on size. its now suitable, imo, for archival data. which is really what this is. removed the "authorized relay stuff" because that's just trusting someone else's archive...and isn't "best practices"

paulmillr · 2023-05-30T14:10:40Z

So you'll break all existing clients just because you don't like current encryption?

A new NIP is needed. That's what NIPs are for. Relays signal which features they support by signalling available NIPs.

earonesty · 2023-05-30T14:11:26Z

it won't break existing clients. they are already robust to failed decryptions. if they weren't... they're already broken. all data from the internets is untrusted by default. as an experiment, ive decided to send a dm like this to everyone i know.

paulmillr · 2023-05-30T14:12:55Z

The decryptions will fail on all existing clients. That's a breakage. New messages would silently fail from a user perspective. You'll be breaking existing, perfectly working, behavior.

earonesty · 2023-05-30T14:14:05Z

If we introduce a new kind: "New messages would silently fail from a user perspective."

paulmillr · 2023-05-30T14:15:42Z

If we introduce a new kind, servers and clients would echo their support for NIP99 (or whatever), which means proper direct messages are supported, and if a client lists NIP04, means they are not. It's easy to distinguish in client feature list.

earonesty · 2023-05-30T14:18:30Z

i would agree, if there was any server behavior that differed. kinds are really about server behavior and user-display. since the message is encrypted, the user-display issue is a non-issue. and there is no server behavior difference. if we're lucky a client will show the message and report an error. it's only beneficial to reuse the kind. the worst case is the same as if we used a new kind. this is not a case of "the old one is ok, go ahead and keep using it". this is a case of "we made a mistake, please fix your client"

i would like some popular client authors to weigh in here instead of two crypto-guys talking about it actually. @fiatjaf might know some

paulmillr · 2023-05-30T14:20:23Z

04.md

@@ -40,13 +44,50 @@ let event = {
  created_at: Math.floor(Date.now() / 1000),
  kind: 4,
  tags: [['p', theirPublicKey]],
-  content: encryptedMessage + '?iv=' + ivBase64
+  content: encryptedMessage + '??' + ivBase64 + "??1"


meh, now we're constructing a weird strings with weird delimiters. Bad for performance, bad for parsing, non-standard

it's actually faster than having ?iv=, ?ver=. benchmark it.

I'm talking about algorithmic complexity in lower-level languages. A parser will need to walk through every ciphertext character and stop on ??.

Why? We already have JSON.

json is no faster. and "?" never appears in base64. this is always going to be faster than JSON. i will change it to one "%". it will always be order N no matter what. im ok wth json, but... it's kinda bloated. and the version thing really protects us

updated to a single delimiter.

i would like to point out how much i appreciate you working on this. i know i can be a bit difficult, but i think we both agree it's a terrible idea to leave it as-is!

04.md

paulmillr · 2023-05-30T14:22:58Z

04.md

+let sharedX = sharedPoint.slice(1, 33)
+let key
+if (version == "1")
+    key = hkdf(sha256, normalizedKey, iv, undefined, 32)


This is unnecessary. It's safe to reuse the same key for billions of messages and even more.

HKDF would also slow down decryption of individual messages. Every decryption would need hkdf call. Decrypting 5000 messages on a slow phone will add 1 second to decryption time.

hkdf is equivalent in speed to sha256 twice

That's false. It's 10 times slower than sha256.

no, it's not safe to reuse the key if the iv is short. reusing the same iv allows the attacker to decrypt messages, and there is a birthday attack here.

2 this is not a performance hit. i tested it. the EC operations are 95% of the time. so you're optimizing for 5%. which has a negligible effect. hkdf exists for a reason. it's easier not to accidentally screw things up. we can replace it with a sha of the (ss + iv), that's ok - since length extension is a non-issue here (we're not verifying the hash)

indeed, it's slower to use sha256... unless you're careful to concatenate the buffers correctly. the buffer manipulation is more expensive than the hash. overall we're talking a 30% performance hit, and it's all because of GCM - which is totally worth it

1000 encryptions on a slow emulator:

test: 2406 ms version 0 test: 2969 ms version 1 sha256 test: 3043 ms version 1 hkdf

hkdf is negligible compared to the GCM hit and it's best practice. lets stick with it!

nice thing about the new standard is that clients should simply display an "encryption version not supported" message when getting messages with a version they don't know. then we can introduce faster/better versions and not have bad behavior

no, it's not safe to reuse the key if the iv is short. reusing the same iv allows the attacker to decrypt messages, and there is a birthday attack here.

So just use xchacha instead of these "constructions".

Care sharing a benchmark file?

i literally just used console.time("test") and constole.timeEnd("time") on the js posted above. seems like the hkdf/sha is irrelevant compared to "doing nothing" and "gcm itself", which eat most of the time. still it's a small diff. and doing nothing is not ok

added mention of the pkcs7 standard

04.md

moonsettler · 2023-05-30T22:34:37Z

so let me get this straight... the bike shedding is about old clients not being able to read DMs with the new safer encryption until they update? nostr clients push out updates every other day. the web ones even faster.

paulmillr · 2023-05-31T08:05:50Z

I don't care much about the issue, what i do care is bad choice of cryptography.

moonsettler · 2023-05-31T08:24:25Z

looks to me the bad choices were already made.

paulmillr · 2023-05-31T11:54:09Z

@moonsettler exactly, that's why it's important to make it right during this iteration.

fiatjaf · 2023-05-31T12:14:12Z

This must be a new kind, a new NIP, definitely. But I don't like it because it doesn't fix all the problems of NIP-04, just makes it marginally better, so it isn't worth the trouble.

I think a new NIP should either propose adoption of the SimpleX protocol or define a variant of it that fits better into Nostr clients (not necessarily using the canonical Nostr event JSON format).

I'd like to hear what you think about SimpleX.

paulmillr · 2023-05-31T12:20:23Z

@fiatjaf simplex is interesting, but we need a simple way to message people over nostr.

Simplex is very complicated and implementing it would take a lot of effort. Which kinda defies nostr simplicity. It would take men-months to implement it properly on different platforms (web, ios, android).

I suggest we draft/implement a NIP that would replace NIP4 and after that we can prototype simplex. The reason is: simplex may get never implemented. We need something more or less secure in the meantime.

it doesn't fix all the problems of NIP-04

The new NIP would immediately fix padding oracle attacks (AES-CBC) and remove residue structure from AES keys. May not be too much, but it's important.

I don't think it's possible to implement a truly private messenger over nostr. Metadata would leak. NSA said "we kill people based on metadata". There would need to be a lot of compromises. Nevertheless i'd be happy to prototype any ideas.

paulmillr · 2023-05-31T12:33:45Z

Benchmarking encryption of 256byte messages...

xchacha20 x 426,985 ops/sec @ 2μs/op ± 1.63% (min: 1μs, max: 5ms)
xsalsa20 x 437,254 ops/sec @ 2μs/op ± 2.02% (min: 1μs, max: 2ms)
aes-256-gcm hkdf x 31,855 ops/sec @ 31μs/op

@earonesty it's 10x slower, really

starbackr-dev · 2023-05-31T12:43:35Z

Yes. SimpleX is complex and the use-case they are going after does not have much overlap with Nostr. I wrote about it here.

#468 (comment)

Also agree that this should be new kind and new NIP. In that case, why can't we include this into NIP-59 which introduces another kind and a solution for metadata leaks? It can solve both in one shot.

Please review this NIP-59

#468

moonsettler · 2023-05-31T13:24:58Z

This must be a new kind, a new NIP, definitely. But I don't like it because it doesn't fix all the problems of NIP-04, just makes it marginally better, so it isn't worth the trouble.

I think a new NIP should either propose adoption of the SimpleX protocol or define a variant of it that fits better into Nostr clients (not necessarily using the canonical Nostr event JSON format).

I'd like to hear what you think about SimpleX.

so if there would be a comprehensive fix, a new kind is probably warranted for that. imo breaking behavior that endangers the users (like leaking private/session keys) is not inherently bad especially considering that this is not consensus code or anything. people would not be able to read some of their DMs until they update? that's a huge deal? migration would just be faster by breaking things.

i liked some parts of the approach simplex makes to avoid passive metadata based social linkage, but maybe nostr would be better served by a simpler way of doing things.

paulmillr · 2023-05-31T14:07:16Z

I have opened pull request for (allegedly) NIP-44 Encrypted Direct Message (Versioned): #574

It uses versioning, XChaCha20 and hashed shared secret. Algorithm choice rationale is described in the pull request. New Versioning feature explicitly requires non-compatible versions to throw user-visible errors.

Please take a look at it! Any comments are welcome.

arthurfranca · 2023-05-31T14:16:09Z

What happens with the NIPs referencing NIP-04 for encrypting stuff like private lists if this becomes a new NIP instead of a new NIP-04 version?

earonesty · 2023-05-31T17:49:04Z

if nip44 merges and chat clients actually use it, then this is obsolete. it handles the 2 important things: versioning and hashing of the shared secret. the whole hvkf vs sha was bikeshedding. perfectly happy to use it instead. i still think it should include the nonce in the production of a non-fixed aes key. that way if a single aes key leaks somehow, the rest of the messages are intact.

nip-4 would, technically, keep working though... which is the downside of not reusing the kind, and breaking old clients.

all references to nip-4 will need to be updates

i feel like the back-compat version has some advantages (reuse the same kind, use versioning to disambiguate).

maybe ill just update this to specify the xchacha20 cipher, and we can let the top 3 or 4 nostr client authors worry about "new kind vs not new kind"

earonesty · 2023-06-20T19:18:07Z

im leaving this as-is, and adding in the metadata blinding, because that's actually what im using, and the fixed-key stuff seems not quite right. just going to reflect what we're using here. perfectly happy to switch to something else that has the same properties (no metadata leakage, correct iv/key use). but if not, i'm sticking with kdf-derived keys and optionally metadata-blind dm's (which also incidentally spam resistant)

earonesty · 2023-06-28T12:21:29Z

taking feedback from others, im closing this in favor of yet another design

earonesty added 4 commits May 30, 2023 06:45

Improvement to 04.md

2aa7862

Remove warning about "unsafe practices" and instead show exactly what the unsafe information is. Add in hkdf, random salt, and use GCM - making the key clean and the cipher operate in accordance with archival data standards.

Update 04.md

4aff279

Update 04.md

f690c95

Update 04.md

a7728b1

earonesty added 2 commits May 30, 2023 07:01

Update 04.md

902c3d9

Update 04.md

59b45f9

Update 04.md

3195703

paulmillr suggested changes May 30, 2023

View reviewed changes

earonesty added 7 commits May 30, 2023 07:29

Update 04.md

40fc4dc

Update 04.md

d69d78b

Update 04.md

902b680

Update 04.md

c74680f

Update 04.md

80b82b2

added mention of the pkcs7 standard

Update 04.md

40e9920

Update 04.md

600b5db

arthurfranca reviewed May 30, 2023

View reviewed changes

04.md Outdated Show resolved Hide resolved

arthurfranca reviewed May 30, 2023

View reviewed changes

04.md Outdated Show resolved Hide resolved

Update 04.md

bf73c08

earonesty added 7 commits June 20, 2023 12:41

Update 04.md

31cb7d9

Update 04.md

fab3be1

Update 04.md

c49f643

Update 04.md

562efb0

Update 04.md

932539e

Update 04.md

4e9a6a9

Update 04.md

348f9a5

earonesty closed this Jun 28, 2023

vitorpamplona mentioned this pull request Aug 7, 2024

nip46: abandon nip04 entirely and just use nip44 #1248

Merged

Improvement to 04.md #572

Improvement to 04.md #572

Conversation

earonesty commented May 30, 2023 • edited Loading

paulmillr commented May 30, 2023 • edited Loading

earonesty commented May 30, 2023 • edited Loading

paulmillr commented May 30, 2023

earonesty commented May 30, 2023 • edited Loading

paulmillr commented May 30, 2023

earonesty commented May 30, 2023

paulmillr commented May 30, 2023

earonesty commented May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earonesty May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earonesty May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earonesty May 30, 2023 • edited Loading

Choose a reason for hiding this comment

earonesty May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earonesty May 30, 2023 • edited Loading

Choose a reason for hiding this comment

moonsettler commented May 30, 2023

paulmillr commented May 31, 2023

moonsettler commented May 31, 2023

paulmillr commented May 31, 2023

fiatjaf commented May 31, 2023 • edited Loading

paulmillr commented May 31, 2023 • edited Loading

paulmillr commented May 31, 2023

starbackr-dev commented May 31, 2023

moonsettler commented May 31, 2023 • edited Loading

paulmillr commented May 31, 2023

arthurfranca commented May 31, 2023

earonesty commented May 31, 2023 • edited Loading

earonesty commented Jun 20, 2023 • edited Loading

earonesty commented Jun 28, 2023

earonesty commented May 30, 2023 •

edited

Loading

paulmillr commented May 30, 2023 •

edited

Loading

earonesty commented May 30, 2023 •

edited

Loading

earonesty commented May 30, 2023 •

edited

Loading

earonesty commented May 30, 2023 •

edited

Loading

earonesty May 30, 2023 •

edited

Loading

earonesty May 30, 2023 •

edited

Loading

earonesty May 30, 2023 •

edited

Loading

earonesty May 30, 2023 •

edited

Loading

earonesty May 30, 2023 •

edited

Loading

fiatjaf commented May 31, 2023 •

edited

Loading

paulmillr commented May 31, 2023 •

edited

Loading

moonsettler commented May 31, 2023 •

edited

Loading

earonesty commented May 31, 2023 •

edited

Loading

earonesty commented Jun 20, 2023 •

edited

Loading