Verify events at RelayConnection

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 <daniel@daquino.me>

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<T: Encodable>(_ val: T) -> String? {
     let encoder = JSONEncoder()
     encoder.outputFormatting = .withoutEscapingSlashes

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))
                 }

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)
             }