From 4570ba797c3c0a90de404ece109af5b27fd099f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20D=E2=80=99Aquino?= Date: Wed, 30 Jul 2025 10:50:10 -0700 Subject: [PATCH] Verify events at RelayConnection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces a verification step at the relay connection level, to help ensure notes get validated at the source and prevent security issues associated with untrusted relays. `RelayConnection.swift` — the source that initially handles WebSocket messages — was analyzed, and measures were put in place to prevent (or at least minimize) unverified nostr event data being spread throughout the app. The following measures were taken: 1. A note verification step was added prior to the `self.handleEvent(.nostr_event(ev))` call (which sends a Nostr response to the rest of the app for logical handling). a. From code analysis, there is only one such call in `RelayConnection.swift`. 2. `NostrConnectionEvent`, the object that gets passed to event handlers, had its interface modified to remove the "message" case, since: a. that could be a source of unverified nostr events. b. it is redundant an unneeded due to the `.nostr_event` case. c. there were no usages of it around the codebase 3. The raw websocket event handler had its label renamed to "handleUnverifiedWSEvent", to make it clear to the caller about the verification status of the data. a. Usages of this were inspected and no significant risk was detected. 4. A new `verify` method in NdbNote was created to verify Nostr notes, and unit tests were added to confirm tampering detections around all the major fields in a Nostr note. 5. Care was taken to ensure the performance regression is as little as possible. It is worth noting that we will not need this once the local relay model architecture is introduced, since that architecture ensures note validation before it reaches the rest of the application and the user. In other words, this is a temporary fix. However, since the migration to that new architecture is a major undertaking that will take some time to be completed, this fix was written in order to address security concerns while the migration is unfinished. This fix was written in a way that attempts to be as effective as possible in reducing security risks without a risky and lenghty refactor of the code that would delay the fix from being published. Changelog-Fixed: Improved security around note validation Closes: https://github.com/damus-io/damus/issues/1341 Signed-off-by: Daniel D’Aquino --- damus/Core/Nostr/NostrEvent.swift | 21 +++++++++ damus/Core/Nostr/RelayConnection.swift | 38 +++++++++++++--- damus/Core/Nostr/RelayPool.swift | 8 ++-- .../Onboarding/Views/SaveKeysView.swift | 2 +- .../Profile/Models/ProfileModel.swift | 2 +- .../Search/Models/SearchHomeModel.swift | 2 +- .../Features/Search/Models/SearchModel.swift | 2 +- .../Features/Timeline/Models/HomeModel.swift | 2 +- damusTests/RequestTests.swift | 2 +- damusTests/damusTests.swift | 45 +++++++++++++++++++ nostrdb/NdbNote.swift | 22 +++++++++ 11 files changed, 131 insertions(+), 15 deletions(-) diff --git a/damus/Core/Nostr/NostrEvent.swift b/damus/Core/Nostr/NostrEvent.swift index 225cd5f8..d3ea206d 100644 --- a/damus/Core/Nostr/NostrEvent.swift +++ b/damus/Core/Nostr/NostrEvent.swift @@ -334,6 +334,27 @@ func decode_nostr_event(txt: String) -> NostrResponse? { return NostrResponse.owned_from_json(json: txt) } +func decode_and_verify_nostr_response(txt: String) -> NostrResponse? { + guard let response = NostrResponse.owned_from_json(json: txt) else { return nil } + guard verify_nostr_response(response: response) == true else { return nil } + return response +} + +func verify_nostr_response(response: borrowing NostrResponse) -> Bool { + switch response { + case .event(_, let event): + return event.verify() + case .notice(_): + return true + case .eose(_): + return true + case .ok(_): + return true + case .auth(_): + return true + } +} + func encode_json(_ val: T) -> String? { let encoder = JSONEncoder() encoder.outputFormatting = .withoutEscapingSlashes diff --git a/damus/Core/Nostr/RelayConnection.swift b/damus/Core/Nostr/RelayConnection.swift index 1df94875..987e1bd5 100644 --- a/damus/Core/Nostr/RelayConnection.swift +++ b/damus/Core/Nostr/RelayConnection.swift @@ -9,8 +9,32 @@ import Combine import Foundation enum NostrConnectionEvent { - case ws_event(WebSocketEvent) + /// Other non-message websocket events + case ws_connection_event(WSConnectionEvent) + /// A nostr response case nostr_event(NostrResponse) + + /// Models non-messaging websocket events + /// + /// Implementation note: Messaging events should use `.nostr_event` in `NostrConnectionEvent` + enum WSConnectionEvent { + case connected + case disconnected(URLSessionWebSocketTask.CloseCode, String?) + case error(Error) + + static func from(full_ws_event: WebSocketEvent) -> Self? { + switch full_ws_event { + case .connected: + return .connected + case .message(_): + return nil + case .disconnected(let closeCode, let string): + return .disconnected(closeCode, string) + case .error(let error): + return .error(error) + } + } + } } final class RelayConnection: ObservableObject { @@ -31,11 +55,11 @@ final class RelayConnection: ObservableObject { init(url: RelayURL, handleEvent: @escaping (NostrConnectionEvent) -> (), - processEvent: @escaping (WebSocketEvent) -> ()) + processUnverifiedWSEvent: @escaping (WebSocketEvent) -> ()) { self.relay_url = url self.handleEvent = handleEvent - self.processEvent = processEvent + self.processEvent = processUnverifiedWSEvent } func ping() { @@ -115,6 +139,7 @@ final class RelayConnection: ObservableObject { } private func receive(event: WebSocketEvent) { + assert(!Thread.isMainThread, "This code must not be executed on the main thread") processEvent(event) switch event { case .connected: @@ -152,7 +177,8 @@ final class RelayConnection: ObservableObject { } } DispatchQueue.main.async { - self.handleEvent(.ws_event(event)) + guard let ws_connection_event = NostrConnectionEvent.WSConnectionEvent.from(full_ws_event: event) else { return } + self.handleEvent(.ws_connection_event(ws_connection_event)) } if let description = event.description { @@ -190,7 +216,9 @@ final class RelayConnection: ObservableObject { private func receive(message: URLSessionWebSocketTask.Message) { switch message { case .string(let messageString): - if let ev = decode_nostr_event(txt: messageString) { + // NOTE: Once we switch to the local relay model, + // we will not need to verify nostr events at this point. + if let ev = decode_and_verify_nostr_response(txt: messageString) { DispatchQueue.main.async { self.handleEvent(.nostr_event(ev)) } diff --git a/damus/Core/Nostr/RelayPool.swift b/damus/Core/Nostr/RelayPool.swift index 6aafda85..5f74727a 100644 --- a/damus/Core/Nostr/RelayPool.swift +++ b/damus/Core/Nostr/RelayPool.swift @@ -126,7 +126,7 @@ class RelayPool { } let conn = RelayConnection(url: desc.url, handleEvent: { event in self.handle_event(relay_id: relay_id, event: event) - }, processEvent: { wsev in + }, processUnverifiedWSEvent: { wsev in guard case .message(let msg) = wsev, case .string(let str) = msg else { return } @@ -214,9 +214,9 @@ class RelayPool { var eoseSent = false self.subscribe(sub_id: sub_id, filters: filters, handler: { (relayUrl, connectionEvent) in switch connectionEvent { - case .ws_event(let ev): + case .ws_connection_event(let ev): // Websocket events such as connect/disconnect/error are already handled in `RelayConnection`. Do not perform any handling here. - // For the future, perhaps we should abstract away `.ws_event` in `RelayPool`? Seems like something to be handled on the `RelayConnection` layer. + // For the future, perhaps we should abstract away `.ws_connection_event` in `RelayPool`? Seems like something to be handled on the `RelayConnection` layer. break case .nostr_event(let nostrResponse): guard nostrResponse.subid == sub_id else { return } // Do not stream items that do not belong in this subscription @@ -366,7 +366,7 @@ class RelayPool { record_seen(relay_id: relay_id, event: event) // run req queue when we reconnect - if case .ws_event(let ws) = event { + if case .ws_connection_event(let ws) = event { if case .connected = ws { run_queue(relay_id) } diff --git a/damus/Features/Onboarding/Views/SaveKeysView.swift b/damus/Features/Onboarding/Views/SaveKeysView.swift index 7ceb5765..3e92c8de 100644 --- a/damus/Features/Onboarding/Views/SaveKeysView.swift +++ b/damus/Features/Onboarding/Views/SaveKeysView.swift @@ -162,7 +162,7 @@ struct SaveKeysView: View { func handle_event(relay: RelayURL, ev: NostrConnectionEvent) { switch ev { - case .ws_event(let wsev): + case .ws_connection_event(let wsev): switch wsev { case .connected: let metadata = create_account_to_metadata(account) diff --git a/damus/Features/Profile/Models/ProfileModel.swift b/damus/Features/Profile/Models/ProfileModel.swift index b4f0eab7..0bfb5519 100644 --- a/damus/Features/Profile/Models/ProfileModel.swift +++ b/damus/Features/Profile/Models/ProfileModel.swift @@ -155,7 +155,7 @@ class ProfileModel: ObservableObject, Equatable { private func handle_event(relay_id: RelayURL, ev: NostrConnectionEvent) { switch ev { - case .ws_event: + case .ws_connection_event: return case .nostr_event(let resp): guard resp.subid == self.sub_id || resp.subid == self.prof_subid || resp.subid == self.conversations_subid else { diff --git a/damus/Features/Search/Models/SearchHomeModel.swift b/damus/Features/Search/Models/SearchHomeModel.swift index 3fd356cf..a8393865 100644 --- a/damus/Features/Search/Models/SearchHomeModel.swift +++ b/damus/Features/Search/Models/SearchHomeModel.swift @@ -150,7 +150,7 @@ func load_profiles(context: String, profiles_subid: String, relay_id: RelayUR let now = UInt64(Date.now.timeIntervalSince1970) switch conn_ev { - case .ws_event: + case .ws_connection_event: break case .nostr_event(let ev): guard ev.subid == profiles_subid, rid == relay_id else { return } diff --git a/damus/Features/Search/Models/SearchModel.swift b/damus/Features/Search/Models/SearchModel.swift index 03e1851c..fbb96ab9 100644 --- a/damus/Features/Search/Models/SearchModel.swift +++ b/damus/Features/Search/Models/SearchModel.swift @@ -109,7 +109,7 @@ func event_matches_filter(_ ev: NostrEvent, filter: NostrFilter) -> Bool { func handle_subid_event(pool: RelayPool, relay_id: RelayURL, ev: NostrConnectionEvent, handle: (String, NostrEvent) -> ()) -> (String?, Bool) { switch ev { - case .ws_event: + case .ws_connection_event: return (nil, false) case .nostr_event(let res): diff --git a/damus/Features/Timeline/Models/HomeModel.swift b/damus/Features/Timeline/Models/HomeModel.swift index 1cd31491..a18fd438 100644 --- a/damus/Features/Timeline/Models/HomeModel.swift +++ b/damus/Features/Timeline/Models/HomeModel.swift @@ -460,7 +460,7 @@ class HomeModel: ContactsDelegate { @MainActor func handle_event(relay_id: RelayURL, conn_event: NostrConnectionEvent) { switch conn_event { - case .ws_event(let ev): + case .ws_connection_event(let ev): switch ev { case .connected: if !done_init { diff --git a/damusTests/RequestTests.swift b/damusTests/RequestTests.swift index aa5348b6..b86bb981 100644 --- a/damusTests/RequestTests.swift +++ b/damusTests/RequestTests.swift @@ -22,7 +22,7 @@ final class RequestTests: XCTestCase { let url = RelayURL("wss://example.com")! let relayDescriptor = RelayPool.RelayDescriptor(url: url, info: .readWrite) let relayConnection = RelayConnection(url: url) { _ in - } processEvent: { _ in + } processUnverifiedWSEvent: { _ in } let relay = RelayPool.Relay(descriptor: relayDescriptor, connection: relayConnection) diff --git a/damusTests/damusTests.swift b/damusTests/damusTests.swift index 4c6d708c..1ee83b30 100644 --- a/damusTests/damusTests.swift +++ b/damusTests/damusTests.swift @@ -18,6 +18,51 @@ class damusTests: XCTestCase { override func tearDownWithError() throws { // Put teardown code here. This method is called after the invocation of each test method in the class. } + + func testEventVerify() throws { + let test_valid_note_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898578,"content":"Test 1102","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[]} + """ + let test_invalid_note_tampered_sig_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898578,"content":"Test 1102","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f153","tags":[]} + """ + let test_invalid_note_tampered_id_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e600","created_at":1753898578,"content":"Test 1102","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[]} + """ + let test_invalid_note_tampered_date_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898579,"content":"Test 1102","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[]} + """ + let test_invalid_note_tampered_pubkey_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898578,"content":"Test 1102","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084b","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[]} + """ + let test_invalid_note_tampered_content_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898578,"content":"Test 1103","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[]} + """ + let test_invalid_note_tampered_kind_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898578,"content":"Test 1102","kind":2,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[]} + """ + let test_invalid_note_tampered_tags_text = """ + {"id":"f4a5635d78d4c1ec2bf7d15d33bd8d5e0afdb8a5a24047f095842281c744e6a3","created_at":1753898578,"content":"Test 1102","kind":1,"pubkey":"056b5b5966f500defb3b790a14633e5ec4a0e8883ca29bc23d0030553edb084a","sig":"d03f0beee7355a8b6ce437b43e01f2d3be8c0f3f17b41a8dec8a9b9804d44ab639b7906c545e4b51820f00b09d00cfa5058916e93126e8a11a65e2623f95f152","tags":[["t", "foo"]]} + """ + + let test_valid_note = NdbNote.owned_from_json(json: test_valid_note_text)! + let test_invalid_note_tampered_sig = NdbNote.owned_from_json(json: test_invalid_note_tampered_sig_text)! + var test_invalid_note_tampered_id = NdbNote.owned_from_json(json: test_invalid_note_tampered_id_text)! + let test_invalid_note_tampered_date = NdbNote.owned_from_json(json: test_invalid_note_tampered_date_text)! + let test_invalid_note_tampered_pubkey = NdbNote.owned_from_json(json: test_invalid_note_tampered_pubkey_text)! + let test_invalid_note_tampered_content = NdbNote.owned_from_json(json: test_invalid_note_tampered_content_text)! + let test_invalid_note_tampered_kind = NdbNote.owned_from_json(json: test_invalid_note_tampered_kind_text)! + let test_invalid_note_tampered_tags = NdbNote.owned_from_json(json: test_invalid_note_tampered_tags_text)! + + XCTAssertEqual(test_valid_note.verify(), true) + XCTAssertEqual(test_invalid_note_tampered_sig.verify(), false) + XCTAssertEqual(test_invalid_note_tampered_id.verify(), false) + XCTAssertEqual(test_invalid_note_tampered_date.verify(), false) + XCTAssertEqual(test_invalid_note_tampered_pubkey.verify(), false) + XCTAssertEqual(test_invalid_note_tampered_content.verify(), false) + XCTAssertEqual(test_invalid_note_tampered_kind.verify(), false) + XCTAssertEqual(test_invalid_note_tampered_tags.verify(), false) + } func testIdEquality() throws { let pubkey = test_pubkey diff --git a/nostrdb/NdbNote.swift b/nostrdb/NdbNote.swift index 4ec2345b..2ce94553 100644 --- a/nostrdb/NdbNote.swift +++ b/nostrdb/NdbNote.swift @@ -345,6 +345,28 @@ class NdbNote: Codable, Equatable, Hashable { json: cstr, json_len: UInt32(json.utf8.count), bufsize: bufsize) } } + + func verify() -> Bool { + let scratch_buf_len = MAX_NOTE_SIZE + let scratch_buf = malloc(scratch_buf_len) + defer { free(scratch_buf) } // Ensure we deallocate as soon as we leave this scope, regardless of the outcome + + let current_id = self.id + + // Calculate the ID based on the content + guard ndb_calculate_id(self.note.ptr, scratch_buf, Int32(scratch_buf_len)) == 1 else { return false } + + let computed_id = self.id + + // Ensure computed ID matches given id to prevent ID tampering + guard computed_id == current_id else { return false } + + // Verify the signature against the pubkey and the computed ID, to verify the validity of the whole note + var ctx = secp256k1_context_create(UInt32(SECP256K1_CONTEXT_VERIFY)) + guard ndb_note_verify(&ctx, ndb_note_pubkey(self.note.ptr), ndb_note_id(self.note.ptr), ndb_note_sig(self.note.ptr)) == 1 else { return false } + + return true + } static func owned_from_json_cstr(json: UnsafePointer, json_len: UInt32, bufsize: Int = 2 << 18) -> NdbNote? { let data = malloc(bufsize)