A few zap forgery issues in Damus

30 views
Skip to first unread message

William Casarin

unread,
Jul 9, 2023, 1:57:25 AM7/9/23
to pat...@damus.io, d...@damus.io, nostr-p...@googlegroups.com, public...@w3.org, William Casarin, Tony Giorgio, benthecarman, Vitor Pamplona, Kieran, Jonathan Staab, Mike Dilger
Hey gang,

Ben found a note zap forgery bug in damus and I'm just emailing the
patches to you guys as a heads up to see if ya'll the same issue. This
was the first time someone was able to forge a zap on one of my notes so
it was surprising!

It came down to the way I determine the "zapper" for a note. The zapper
is the authorized zap-note creator. The zap spec expects clients to
lookup the nostrPubkey on the lnurl endpoint and make sure that it
matches the note that is being zapped.

The issue in damus was that it was looking up the zapper based on the
first p-tag on the zap note. This is wrong since that can be forged and
different from the pubkey of the note being zapped. This allows an
attacker to slip a fake zap onto a note, because the zapper that is
checked is actually a valid zapper for that ptag, but it's the wrong
ptag.

Damus now looks up the note in the cache and uses that pubkey instead of
whatever is on the note.

There's a similar issue with profile zaps which I have a fix for in here
as well.

Cheers,
Will

Cc: Tony Giorgio <tonyg...@protonmail.com>
Cc: benthecarman <benthe...@live.com>
Cc: Vitor Pamplona <vi...@vitorpamplona.com>
Cc: Kieran <kie...@harkin.me>
Cc: Jonathan Staab <sht...@gmail.com>
Cc: Mike Dilger <mi...@mikedilger.com>

William Casarin (2):
Fix fake note zaps with forged p-tags
Prevent forged profile zap attacks

damus/Models/HomeModel.swift | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)

--
2.39.2 (Apple Git-143)

William Casarin

unread,
Jul 9, 2023, 1:57:28 AM7/9/23
to pat...@damus.io, d...@damus.io, nostr-p...@googlegroups.com, public...@w3.org, William Casarin, Tony Giorgio, benthecarman, Vitor Pamplona, Kieran, Jonathan Staab, Mike Dilger
The fake note zap attack made me realize that there is a way to do fake
profile zaps using a similar technique. Since damus only checks the
first ptag if it is a profile zap, this means you could include multiple
ptags, the first one being the fake profile with the fake zapper, and
the second p tag as the real target.

This would allow a fake zapper to create a fake a zap, while the zap
notification would still appear for the second ptag because damus
listens for zap events via #p, and that would match the second ptag.

To fix this, ensure that zaps only have at most 1 ptag and 0 or 1 etag.
my CLN zapper checks this but if we don't check this here as well then
we run into fake zap issues.

Changelog-Fixed: Fix potential fake profile zap attacks
Cc: Tony Giorgio <tonyg...@protonmail.com>
Cc: benthecarman <benthe...@live.com>
Cc: Vitor Pamplona <vi...@vitorpamplona.com>
Cc: Kieran <kie...@harkin.me>
Cc: Jonathan Staab <sht...@gmail.com>
Cc: Mike Dilger <mi...@mikedilger.com>
Signed-off-by: William Casarin <jb...@jb55.com>
---
damus/Models/HomeModel.swift | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/damus/Models/HomeModel.swift b/damus/Models/HomeModel.swift
index e727c9c1..b52a0d23 100644
--- a/damus/Models/HomeModel.swift
+++ b/damus/Models/HomeModel.swift
@@ -1239,11 +1239,24 @@ enum ProcessZapResult {
// securely get the zap target's pubkey. this can be faked so we need to be
// careful
func get_zap_target_pubkey(ev: NostrEvent, events: EventCache) -> String? {
- if let etag = event_tag(ev, name: "e") {
+ let etags = ev.referenced_ids
+
+ if let etag = etags.first {
+ // ensure that there is only 1 etag to stop fake note zap attacks
+ guard etags.count == 1 else {
+ return nil
+ }
// we can't trust the p tag on note zaps because they can be faked
return damus_state.events.lookup(etag)?.pubkey
} else {
- return event_tag(ev, name: "p")
+ let ptags = ev.referenced_pubkeys
+
+ // ensure that there is only 1 ptag to stop fake profile zap attacks
+ guard ptags.count == 1 else {
+ return nil
+ }
+
+ return ptags.first?.id
}
}

--
2.39.2 (Apple Git-143)

William Casarin

unread,
Jul 9, 2023, 1:57:30 AM7/9/23
to pat...@damus.io, d...@damus.io, nostr-p...@googlegroups.com, public...@w3.org, William Casarin, benthecarman, Tony Giorgio, Vitor Pamplona, Kieran, Jonathan Staab, Mike Dilger
This fixes a zap issue where someone could send a fake zap with a zapper
that doesn't match the user's nostrPubkey zapper. This is possible
because damus looks up the zapper via the ptag on note zaps.

Fix this by first looking up the cached event's ptag instead. This
prevents zappers from trying to trick Damus into picking the wrong
zapper.

Fixes: #1357
Changelog-Fixed: Fix issue where malicious zappers can send fake zaps to another user's posts
Reported-by: benthecarman <benthe...@live.com>
Cc: Tony Giorgio <tonyg...@protonmail.com>
Cc: benthecarman <benthe...@live.com>
Cc: Vitor Pamplona <vi...@vitorpamplona.com>
Cc: Kieran <kie...@harkin.me>
Cc: Jonathan Staab <sht...@gmail.com>
Cc: Mike Dilger <mi...@mikedilger.com>
Signed-off-by: William Casarin <jb...@jb55.com>
---
damus/Models/HomeModel.swift | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/damus/Models/HomeModel.swift b/damus/Models/HomeModel.swift
index 556ca4ea..e727c9c1 100644
--- a/damus/Models/HomeModel.swift
+++ b/damus/Models/HomeModel.swift
@@ -1239,7 +1239,12 @@ enum ProcessZapResult {
// securely get the zap target's pubkey. this can be faked so we need to be
// careful
func get_zap_target_pubkey(ev: NostrEvent, events: EventCache) -> String? {
- return event_tag(ev, name: "p")
+ if let etag = event_tag(ev, name: "e") {
+ // we can't trust the p tag on note zaps because they can be faked
+ return damus_state.events.lookup(etag)?.pubkey
+ } else {
+ return event_tag(ev, name: "p")
+ }
}

func process_zap_event(damus_state: DamusState, ev: NostrEvent, completion: @escaping (ProcessZapResult) -> Void) {
--
2.39.2 (Apple Git-143)

Reply all
Reply to author
Forward
0 new messages