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

Is NIP-07 decryptEvent(event) a good idea? #1439

Closed
arthurfranca opened this issue Aug 22, 2024 · 41 comments
Closed

Is NIP-07 decryptEvent(event) a good idea? #1439

arthurfranca opened this issue Aug 22, 2024 · 41 comments

Comments

@arthurfranca
Copy link
Contributor

Currently, asking an extension to decrypt something may end up granting a too broad of a permission to an app.

With a decryptEvent method, the extension is able, e.g., to grant just private bookmarks decryption permisson to an app, which wouldn't allow the app to decrypt DMs.

It would be something like async window.nostr.nip44.decryptEvent(event)?

That are some complications:

  • I'm not sure what such a method should return, because while most event kinds encrypt just the event.content field, some may encrypt one or more tags.
  • The extension should be smart enough to know which pubkey to use, depending on what is the event.kind.
@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Aug 22, 2024

You will need to define a separate return type for each event type. Lists would be the tag list inside of the .content. GiftWraps will return Events. Drafts return unsigned event templates. ZapEvents with private zaps should return the Private Zap event or null. Etc.

@fiatjaf
Copy link
Member

fiatjaf commented Aug 22, 2024

I agree. Granting full decryption rights is bad.

@vitorpamplona
Copy link
Collaborator

Maybe the return should be the same event, but replacing the encrypted part by the decrypted string in place. So, it might replace the .content, or tags, or both.

For spreadsheets and forms, for instance, the signer must try to decrypt every p or key it finds. The receiver must compare the object with the original to find out which tag positions were changed.

@staab
Copy link
Member

staab commented Aug 22, 2024

replacing the encrypted part by the decrypted string in place

This would break signatures, so decrypted events should be validated too.

The receiver must compare the object with the original to find out which tag positions were changed.

Or, the caller could specify a list of paths to attempt decryption on. For example [["content"], ["tags", 3, 1]]

@arthurfranca
Copy link
Contributor Author

arthurfranca commented Aug 22, 2024

I was wondering if there is a smarter way that wouldn't require a decryptEvent(event) method, avoiding pushing complexity to NIP-07 extensions.

I don't know if it is possible with this nip44 version @paulmillr. But I imagine such a thing exists in the encryption world: what if, when encrypting, the client could pass a metadata (an event kind number) as argument, to be added to the ciphertext somehow. Like this window.nostr.nip44.encrypt(pubkey, plaintext, eventKind)

Then, when app calls window.nostr.nip44.decrypt(pubkey, ciphertext), the extension would decrypt it, see to which event kind it is related to and then ask the user to grant permission for decrypting things scoped to that kind.

@paulmillr
Copy link
Contributor

we can make a generic encryption scheme with nip44. I just need how an API would look like

@arthurfranca
Copy link
Contributor Author

@paulmillr Like i mentioned above: this encrypt(pubkey, plaintext, eventKind, salt='nip44-v2') instead of how nip44 does currently which is encrypt(pubkey, plaintext, salt='nip44-v2')

eventKind argument is an unsigned integer that can go from 0 to 4294967295 (according to #636, or 0 to 65535 if using today's limits)

Then when decrypting, decrypt(pubkey, ciphertext) would return { plaintext: 'example', eventKind: 787 } instead of just example.

@paulmillr
Copy link
Contributor

What's pubkey? Who's pubkey is it? nip44 was made for conversations e.g. between Alice and Bob, so it's Alice's privkey and Bob's pubkey.

@arthurfranca
Copy link
Contributor Author

@paulmillr It is nip44, if possible, so Bob's pubkey in your example. The idea is to extend nip44 to allow for embedding an extra information (specifically, the kind number of the nostr event where the ciphertext will be inserted/used) besides the plaintext when encrypting.

Then when Bob's client asks an external signer/encryption-manager (NIP-07) to decrypt the ciphertext using his privkey and Alice's pubkey, the signer will decrypt the ciphertext but will only return the resulting plaintext to Bob's client if the client has permission to decrypt things associated to that specific kind number the signer got when decrypting.

@paulmillr
Copy link
Contributor

Understood. In this case, that seems like something which should be handled on a higher level than nip44.

I don’t see how nip44 is relevant here. Key storage should implement permission system, clients should implement it, then they do checks here and there and everything is working. Am I wrong?

@arthurfranca
Copy link
Contributor Author

arthurfranca commented Aug 23, 2024

Key storage is unable to implement a fine grained permission system if it doesn't know exactly what the client is asking it to decrypt. Is it a direct message, is it a private mute list, or a private bookmark list?

When encrypting an item, If Alice could also embed a description of what it is (the event kind number would act as that description), then when Bob's client tries to decrypt it calling bobsKeystorage.nip44.decrypt(alicePubkey, ciphertext), keystorage would be able to show a permission dialog like "Hey Bob, do you allow client recipes.xyz to decrypt your direct messages?" instead of "Hey Bob, do you allow client recipes.xyz to decrypt anything?"

So in the first step, Alice needs to be able to do alicesKeystorage.nip44.encrypt(bobpubkey, plaintext, eventKind) => ciphertextWithEventKindEmbedded when encrypting instead of alicesKeystorage.nip44.encrypt(bobPubkey, plaintext) => ciphertext that is what we have today.

@greenart7c3
Copy link
Contributor

I think you want something like I did in this pull request

#1354

I still haven't implemented in any apps but I will eventually

@arthurfranca
Copy link
Contributor Author

@greenart7c3 pretty close! Except that we cannot trust a client to tell the correct kind when decrypting, it can lie. This information does should be set by the client at the moment it encrypts something, because it has no reason to lie when encrypting.

The kind should be encrypted together with the plaintext message. What I propose is using something like this: https://en.wikipedia.org/wiki/Authenticated_encryption#Authenticated_encryption_with_associated_data, <- does nip44 has something similar available @paulmillr? Also @earonesty, @mikedilger, @Semisol and @mleku seem to know a lot about encryption, maybe they can help too.

A naive way would be to concatenate the real plaintext message with the event kind before encrypting, but this would be a massive breaking change, cause clients/keystorages not aware of this would think the event kind is part of the message when decrypting.

@paulmillr
Copy link
Contributor

Is it a direct message, is it a private mute list, or a private bookmark list?

Okay, so:

  1. Decrypt it, eventType = decrypted.type
  2. Utilize server-side storage to get permission list for the client X. allowedTypes = getTypesForClient(key, 'x')
  3. Check if eventType is in allowedTypes

Am I missing something here?

No, we don't support AEAD, because there was no need for it.

@arthurfranca
Copy link
Contributor Author

arthurfranca commented Aug 23, 2024

Decrypt it, eventType = decrypted.type

Where did decrypted.type came from? Currently when decrypting we only get back a plaintext.

The keystorage could only know the type if we called keystorage.decryptEVENT(WHOLE_EVENT) instead of keystorage.decrypt(ciphertext) (where ciphertext most times is event.content field - but not always).

No, we don't support AEAD, because there was no need for it.

Thats unfortunate. The idea behind using AEAD is that we would avoid sending the whole event as argument when decrypting. Cause of complications showed on the first 5 comments of this issue.

@paulmillr out of curitosity, how easy would it be for a nip44 v3 to include AEAD or would it require a completely new encryption scheme, a new nip?

Well, although AEAD is superior because we would be able to avoid the decryptEvent() addition, @staab's path argument idea atleast make decryptEvent() usage more palatable.

@paulmillr
Copy link
Contributor

I still don't understand what are the exact inputs and outputs. Can someone write code? e.g. something like

// client
data = ...
sendToServer(data)

// server
data = receiveFromClient()
...

how easy would it be for a nip44 v3 to include AEAD

Anybody could create any "new version". But v2 was audited. So unless there is some really good update, I would not want to update it. Any new detail is potential data leak etc.

@arthurfranca
Copy link
Contributor Author

Let me try.

Alice's webapp asks a nostr browser extension to encrypt a third event tag's value before sending the event to Bob:

eventKind = 98
// notice there is an eventKind argument when asking to encrypt (for AEAD magic)
encryptedTagValue3 = nip44EncryptAtClientSideBrowserExtensionThatHoldsAlicePrivkey(bobPubkey, 'hello mom', eventKind)
event = {
  kind: eventKind, id: 12, pubkey: 'ab..cd', created_at: 23, content: 'zi', sign: 'iu', tags: [
    ['tagName', 'tagValue']
    ['tagName2, 'tagValue2]
    ['tagName3, 'encryptedTagValue3']
  ]
}
sendEventToBob(event)

Bob is using primal.net webapp, which will ask a nostr browser extension to decrypt the ciphertext that is at the third event tag (the ciphertext could be at another tag or field, depending on which event kind).

event = getEventFromAlice()
ciphertext = event.tags[2][1]
nip44DecryptAtClientSideBrowserExtensionThatHoldsBobPrivkey(alicePubkey, ciphertext)
// Here the browser extension finds out the ciphertext includes additional data (AEAD magic), the event kind that is 98.
// Let's say kind 98 means that the event is a private medical information.
// Extension sees that primal.net doesn't have permission to decrypt messages related to kind 98.
// Extension shows a pop-up to Bob: "Do you want to allow primal.net to decrypt medical informations? [Yes] [No] [v] remember choice"
//
// Bob clicks "No", cause primal.net isn't supposed to be used for reading medical data
//
// above method nip44DecryptAtClientSideBrowserExtensionThatHoldsBobPrivkey throws error

So, it is important for the browser extension to know the event kind. Or else, the browser extension can't inform to Bob that primal.net wants to decrypt medical data, it could only say that primal.net wants permission to decrypt anything.

@paulmillr
Copy link
Contributor

  1. Why don't you put additional info like kind=98 into tags[3]?
  2. Why don't you make 'hello mom' something like {"kind": 98, "text": "hello mom"}?

@arthurfranca
Copy link
Contributor Author

{"kind": 98, "text": "hello mom"}

I return the question to you, why does AEAD even exists if we could just encrypt the additional data (AD) together with the message, like in your example?

I imagine there is some added benefit, like for example being able to know the additional data content before really fully decrypting the ciphertext?

@Semisol
Copy link
Collaborator

Semisol commented Aug 24, 2024

Currently, asking an extension to decrypt something may end up granting a too broad of a permission to an app.

With a decryptEvent method, the extension is able, e.g., to grant just private bookmarks decryption permisson to an app, which wouldn't allow the app to decrypt DMs.

It would be something like async window.nostr.nip44.decryptEvent(event)?

That are some complications:

* I'm not sure what such a method should return, because while most event kinds encrypt just the `event.content` field, some may encrypt one or more tags.

* The extension should be smart enough to know which pubkey to use, depending on what is the `event.kind`.

This idea may as well not exist unless we rework the encryption spec.

If I want to decrypt a DM from Bob to Alice, as Alice, without showing I want to decrypt a DM, as a marketplace app:

  1. The app could first analyze timing information to see if a manual prompt is involved
  2. The app could take the DM content from Bob to Alice, and create a marketplace message from Alice to Bob
  3. The app could then ask for the decryption of what the extension thinks is a marketplace message.

The only solution is to use AEAD or some other method to add the event type, and context (who is sending and receiving) to the encrypted blob.

I return the question to you, why does AEAD even exists if we could just encrypt the additional data (AD) together with the message, like in your example?

When you already know the AD before decryption, such as the source or destination public keys.

@vitorpamplona
Copy link
Collaborator

If all we want to do is to allow signers to manage decrypt permissions per kind, how about adding context to the current decrypt call?

window.nostr.nip44.decrypt(pubkey, ciphertext, context: Event)

ciphertext must be included in the context and the signer must be able to verify the event.

If context is absent, the app is asking for broad permissions. If the context is present, the app is asking for individual permissions. The user can accept or reject broad and specific permissions separately.

@arthurfranca
Copy link
Contributor Author

@vitorpamplona doesn't work, cause the pubkey argument isn't always the same as event.pubkey (e.g: your spreadsheets PR).
In these cases the app could generate a fake event to use as context argument.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Aug 24, 2024

@vitorpamplona doesn't work, cause the pubkey argument isn't always the same as event.pubkey (e.g: your spreadsheets PR).

The pubkey should not be part of the signer's checks anyway. The pubkey for the encryption/decryption should not be required to be in the event for privacy reasons. It can come from another event or another decryption on the same event.

In these cases the app could generate a fake event to use as context argument.

ciphertext still must be part of the event. The app cannot pass a fake event if the signer checks it.

@arthurfranca
Copy link
Contributor Author

Its like @Semisol described. Both window.nostr.nip44.decrypt(pubkey, ciphertext, context: Event) and window.nostr.nip44.decryptEvent(pubkey, event) are flawed.

If evil app wants to decrypt { kind: 123, ..., tags: ["anon", "<ciphertext-xyz>"] }, and it knows browser extension has already granted the app decryption rights for kind:666 (the first step of @Semisol example), the app can just create an event like { kind: 666, pubkey: <random>, content: <ciphertext-xyz>" } and pass this event to @vitorpamplona's window.nostr.nip44.decrypt(pubkey, ciphertext, context: Event). The browser extension would happily decrypt it without the user knowing.

@arthurfranca
Copy link
Contributor Author

@Semisol The only solution is to use AEAD or some other method to add the event type, and context (who is sending and receiving) to the encrypted blob.

Interesting idea to also include sender/receiver. Though I think just event kind would be good enough context regarding NIP-07, like it is easier for the user to decide if it wants to grant the app permission to handle any event of a specific kind, no matter the peers involved.

@paulmillr
Copy link
Contributor

paulmillr commented Aug 24, 2024

I return the question to you

It took a great deal of time to make everything polished. Then an auditor was hired for a bunch of money. Do you want to change something and stay secure? It's better to do the process again. Cryptography is not your average iterative code. Maybe ask BIP340 authors if they are iterating on a new version of it.

why does AEAD even exists

There is no native AAD in NIP44. AAD is supported in polynomial MACs such as AES-GCM and chacha20poly1305. We don't use polynomial macs.

NIP44 has "hmac-aad" abstraction, but it's already used for nonces. How do you properly concatenate nonce with other aad. Which default value are you going to use. Are there any things that one needs to keep in mind? Hmac was partially created because sha256(a + b) is unsafe (length extension attacks). So even concatenation is not always safe.

@arthurfranca
Copy link
Contributor Author

[...] an auditor was hired for a bunch of money. [...] better to do the process again

Money for auditing is a subject for @fiatjaf CEO and @staab Treasurer.

Which default value are you going to use.

Anything that isn't an event kind, it can't be a positive integer.

How do you properly concatenate nonce with other aad.

Sorry, I'm no cryptographer. Just pointing the problem of using current NIP-44 implementation in the context of an untrusted nostr client/app talking to a trusted NIP-07 browser extension or NIP-46 signer/keystorage, which is a very common way of using Nostr, instead of the much less secure practice of pasting user's nsec on apps directly.

Currently, user end up granting or rejecting the untrusted app permission to decrypt any event, no matter the event kind number. We need granularity by event kind number when user is choosing to grant/reject decryption permission to an app.

@paulmillr
Copy link
Contributor

So if you're not cryptographer, find a way to solve this without adjusting cryptography.

@arthurfranca
Copy link
Contributor Author

I raised a concern. If the nostr community acknowledges there is a problem in the way encryption is used on the protocol, I'm no way in charge of solving it.

If there really is a problem, and it looks like there is, the community can always choose to just hand-wave it away.

@arthurfranca
Copy link
Contributor Author

@fiatjaf @vitorpamplona @staab please review this comment. Is the logic wrong?

If the logic is right, the problem is big and we cannot rely on a decryptEvent method or similar to fix the broad nip07/46 decryption rights problem.

The fix is probably adding AD (additional data) capability to the encryption, as outlined by @Semisol comment: The only solution is to use AEAD or some other method to add the event type (kind) [...]

@paulmillr doesn't want to help with adding AD to nip44. I think it's because he doesn't know how nip07/46 works, so he doesn't acknowledge there is a problem that needs a fix.

We shoud find someone else to help... even repurposing nip04 simpler encryption to add AD if possible would be great, if anyone knows how to do it. Anyway I've heard on nostr telegram and elsewhere that nip04 drawbacks are farfetched anyway but I'm not able to give an opinion on that.

@Semisol
Copy link
Collaborator

Semisol commented Aug 30, 2024

The fix is probably adding AD (additional data) capability to the encryption, as outlined by @Semisol comment: The only solution is to use AEAD or some other method to add the event type (kind) [...]

You may also want to add who is the "sender" and "receiver" along with things like context (is this in a tag, or the content field, etc)

@arthurfranca
Copy link
Contributor Author

@Semisol my preliminary understanding is that the extra context you mention
would be useful only if using the encryption outside nostr events.

Currently I think that adding the event.kind is enough, cause would fulfil
the goal of a nip07/46 element being able to say: App x.y.com wants to decrypt "<a-specific-kind>". [Allow] [Always Allow] [Reject]

@Semisol
Copy link
Collaborator

Semisol commented Aug 30, 2024

It took a great deal of time to make everything polished. Then an auditor was hired for a bunch of money. Do you want to change something and stay secure? It's better to do the process again. Cryptography is not your average iterative code. Maybe ask BIP340 authors if they are iterating on a new version of it.

Simple schemes built on top of proven cryptographic primitives do not require an audit and can be proven to work, and your comparison to BIP340 is invalid.

A scheme built on (X)ChaCha20-Poly1305 with a hashed shared point being used as the encryption key can be implemented easily.

@arthurfranca
Copy link
Contributor Author

arthurfranca commented Aug 30, 2024

@Semisol I mean, sender and receiver is useful. But the nostr way is "trusting" (not the right word but) whatever the author signed. If the event's author copy and pastes someone else's ciphertext and adds it to their own event, it is similar to copy/pasting someone else's plaintext.

edit: hmm in fact, the ciphertext can only be decrypted with the right sender/receiver keys anyway, so the example of copy pasting was kinda useless, but the "(is this in a tag, or the content field, etc)" context apply to the copy/paste example I think

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Aug 30, 2024

We can do a simple version of this where the signer checks if the decrypted payload is an event and uses that information to manage permissions.

For DMs, for instance, the app will send the .content in the wrap, which will decrypt to show a kind 13 seal and decrypting the seal reveals a kind 14 DMs. The signer can decrypt first and they ask the user to authorize sending the decrypted payload to the app by kind.

In that way, if a NIP knows this is available, it can format the encrypted payload as an event (even without signatures).

NIP 51's encrypted tag list, for instance, could be turned into an event with kind and tag array instead of just the tag array. It's a simple change that can be done when we migrate from NIP-04 to NIP-44.

@arthurfranca
Copy link
Contributor Author

arthurfranca commented Aug 30, 2024

@vitorpamplona yeah this is like doing encrypt('{ "kind": 111, "text": "..." }'), similar effect to doing encryptWithAD(text='...', ad=1111).

The former would need to change the way all NIPs format plaintext before encrypting it, while the latter would change the encryption algo all NIPs use.

I suspect with AD, the "kind" is discoverable before finising the whole decryption process, adding to speed?

edit: but anything that solves the problem is ok. What @vitorpamplona said would be ok, or better a more generally applicable version (not only for seals) like the above encrypt('{ "kind": 111, "text": "..." }')

@paulmillr
Copy link
Contributor

@arthurfranca can you stop mentioning me? I've unsubscribed myself from this issue.

@Semisol you're free to develop custom encryption schemes for anything.

@Semisol
Copy link
Collaborator

Semisol commented Aug 30, 2024

We can do a simple version of this where the signer checks if the decrypted payload is an event and uses that information to manage permissions.

For DMs, for instance, the app will send the .content in the wrap, which will decrypt to show a kind 13 seal and decrypting the seal reveals a kind 14 DMs. The signer can decrypt first and they ask the user to authorize sending the decrypted payload to the app by kind.

In that way, if a NIP knows this is available, it can format the encrypted payload as an event (even without signatures).

NIP 51's encrypted tag list, for instance, could be turned into an event with kind and tag array instead of just the tag array. It's a simple change that can be done when we migrate from NIP-04 to NIP-44.

This does not provide much benefit compared to an AEAD scheme, and actually requires another layer of wrapping (event -> encryption -> another event -> actual data)

@vitorpamplona
Copy link
Collaborator

This does not provide much benefit compared to an AEAD scheme, and actually requires another layer of wrapping (event -> encryption -> another event -> actual data)

I'd say a custom AEAD scheme does not provide much benefit on top of a simple change like that.

The wrapping will have to exist no matter what. It's either wrapped before the encryption or during the encryption by the AEAD itself.

@arthurfranca
Copy link
Contributor Author

bump

There is currently no way on nostr to segregate private data
by their category (event kind) to selectively give a specific app
access to a category of them.

We can't expect an user to code their own client nor to
review the code of every client they want to use.
The fact that non-nostr apps are data silos of limited scope,
make trusting them MUCH BETTER than trusting a harmless looking micro nost app

that can access all kinds of user events.

If encrypting, we can use public relays. The problem is that
with nip07/46 we grant full decryption rights to an app,
no matter the event kind.

If not encrypting, user would have to self-host relays.
But self-hosting relays isn't for all users, and even if it was,
one can't limit a connection to just specific event kinds then
use nip42 event protected by a nip07/46 prompt.

Wen AEAD? Or we keep ignoring this big security concern?

@arthurfranca
Copy link
Contributor Author

Due to lack of interest (on enhancing encryption with AEAD) from those that usually discuss proposals here, the only viable solution is to optionally do string = JSON.stringify({ kind: <event kind number|string description>, ...any_other_fields }) before encrypting (nip07.nip44.encrypt(string)).

Then to use a nip07 extension that always decrypt things before prompting user. It would decrypt it, try to parse it to JSON and check if there is a kind field, so it can inform user about it or fallback to regular "permission to decrypt anything".

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

No branches or pull requests

7 participants