git » sdk » commit 6bcf141

OMEMO: Refactor encryption status reporting

author Matthew Wild
2025-05-16 10:22:47 UTC
committer Stephen Paul Weber
2025-09-29 13:43:04 UTC
parent 8579964b021428d01353a3307227a6cec388cb72

OMEMO: Refactor encryption status reporting

Previously we put the encryption status into the stanza, as this was a
convenient way to pass the information around with the stanza. A downside is
that this could be forged by a sender (or intermediate hop) unless we make
sure to always strip it. It was also a bit more clunky and less performant
than simply creating and passing around an in-memory structure. So that's what
we do now - though some API function signatures have necessarily changed.

doc/OMEMO.md +1 -1
snikket/Client.hx +4 -4
snikket/EncryptionInfo.hx +22 -45
snikket/Message.hx +4 -2
snikket/MessageSync.hx +3 -1
snikket/OMEMO.hx +56 -22

diff --git a/doc/OMEMO.md b/doc/OMEMO.md
index 69fcc0c..c130206 100644
--- a/doc/OMEMO.md
+++ b/doc/OMEMO.md
@@ -16,9 +16,9 @@ compile with the NO_OMEMO flag.
 - [x] Encrypt outgoing messages to the sending account's other devices
 - [x] Persistence: IndexedDB (for web)
 - [x] Use cache for remote contact devices
+- [x] Fix that encryption status reported by the API can be forged by sender
 - [ ] Persistence: SQLite backend (for native)
 - [ ] API to control encryption of outgoing messages
 - [ ] API to determine cryptographic identity of message sender
-- [ ] Fix that encryption status reported by the API can be forged by sender
 - [ ] Group chat support
 
diff --git a/snikket/Client.hx b/snikket/Client.hx
index f11b598..c98c300 100644
--- a/snikket/Client.hx
+++ b/snikket/Client.hx
@@ -371,9 +371,9 @@ class Client extends EventEmitter {
 			}
 #if !NO_OMEMO
 			if(stanza.hasChild("encrypted", NS.OMEMO)) {
-				omemo.decryptMessage(stanza).then((decryptedStanza) -> {
+				omemo.decryptMessage(stanza).then((decryptionResult) -> {
 					trace("OMEMO: Decrypted message, now processing...");
-					processLiveMessage(decryptedStanza);
+					processLiveMessage(decryptionResult.stanza, decryptionResult.encryptionInfo);
 					return true;
 				});
 				return EventHandled;
@@ -596,7 +596,7 @@ class Client extends EventEmitter {
 	}
 
 	@:allow(snikket)
-	private function processLiveMessage(stanza:Stanza):Void {
+	private function processLiveMessage(stanza:Stanza, ?encryptionInfo:EncryptionInfo):Void {
 		final from = stanza.attr.get("from") == null ? null : JID.parse(stanza.attr.get("from"));
 
 		if (stanza.attr.get("type") == "error" && from != null) {
@@ -620,7 +620,7 @@ class Client extends EventEmitter {
 			if (chat == null && stanza.attr.get("type") != "groupchat") chat = getDirectChat(builder.chatId());
 			if (chat == null) return builder;
 			return chat.prepareIncomingMessage(builder, stanza);
-		});
+		}, encryptionInfo);
 
 		switch (message.parsed) {
 			case ChatMessageStanza(chatMessage):
diff --git a/snikket/EncryptionInfo.hx b/snikket/EncryptionInfo.hx
index c5315ee..df661b6 100644
--- a/snikket/EncryptionInfo.hx
+++ b/snikket/EncryptionInfo.hx
@@ -46,53 +46,30 @@ class EncryptionInfo {
         return el;
     }
 
+    // Typically used to deduce an EncryptionInfo when none has been provided
+    // May return null if the stanza is not recognizably encrypted.
     static public function fromStanza(stanza:Stanza):Null<EncryptionInfo> {
-        final decryptionStatus = stanza.getChild("decryption-status", "https://snikket.org/protocol/sdk");
 		final emeElement = stanza.getChild("encryption", "urn:xmpp:eme:0");
-
-        if(decryptionStatus != null) {
-            if(decryptionStatus.attr.get("result") == "failure") {
-                // We attempted to decrypt this stanza, but something
-                // went wrong. The decryption-status element contains
-                // more information.
-                final ns = decryptionStatus.attr.get("encryption");
-                return new EncryptionInfo(
-                    DecryptionFailure,
-                    ns??"unknown",
-                    ns != null?knownEncryptionSchemes.get(ns):"Unknown encryption",
-                    decryptionStatus.getChildText("reason"), // Machine-readable reason (depends on encryption method)
-                    decryptionStatus.getChildText("text"), // Human-readable explanation
-                );
-            } else {
-                final encryptionMethod = decryptionStatus.attr.get("encryption")??"unknown";
-                return new EncryptionInfo(
-                    DecryptionSuccess,
-                    encryptionMethod,
-                    knownEncryptionSchemes.get(encryptionMethod)??"Unknown encryption"
-                );
-            }
-        } else {
-			// We did not decrypt this stanza, so check for any signs
-			// that it was encrypted in the first place...
-			var ns = null, name = null;
-			if(emeElement != null) {
-				ns = emeElement.attr.get("namespace");
-				name = emeElement.attr.get("name");
-			} else if(stanza.getChild("encrypted", "eu.siacs.conversations.axolotl") != null) {
-				// Special handling for OMEMO without EME, just because it is
-				// so widely used.
-				ns = "eu.siacs.conversations.axolotl";
-			}
-            if(ns != null) {
-                return new EncryptionInfo(
-                    DecryptionFailure,
-                    ns??"unknown",
-                    knownEncryptionSchemes.get(ns)??name??"Unknown encryption",
-                    "unsupported-encryption",
-                    "Unsupported encryption method"
-                );
-            }
-		}
+        // We did not decrypt this stanza, so check for any signs
+        // that it was encrypted in the first place...
+        var ns = null, name = null;
+        if(emeElement != null) {
+            ns = emeElement.attr.get("namespace");
+            name = emeElement.attr.get("name");
+        } else if(stanza.getChild("encrypted", "eu.siacs.conversations.axolotl") != null) {
+            // Special handling for OMEMO without EME, just because it is
+            // so widely used.
+            ns = "eu.siacs.conversations.axolotl";
+        }
+        if(ns != null) {
+            return new EncryptionInfo(
+                DecryptionFailure,
+                ns??"unknown",
+                knownEncryptionSchemes.get(ns)??name??"Unknown encryption",
+                "unsupported-encryption",
+                "Unsupported encryption method"
+            );
+        }
         return null; // Probably not encrypted
     }
 }
diff --git a/snikket/Message.hx b/snikket/Message.hx
index c307446..f3e8835 100644
--- a/snikket/Message.hx
+++ b/snikket/Message.hx
@@ -48,10 +48,12 @@ class Message {
 		this.encryption = encryption;
 	}
 
-	public static function fromStanza(stanza:Stanza, localJid:JID, ?addContext: (ChatMessageBuilder, Stanza)->ChatMessageBuilder):Message {
+	public static function fromStanza(stanza:Stanza, localJid:JID, ?addContext: (ChatMessageBuilder, Stanza)->ChatMessageBuilder, ?encryptionInfo:EncryptionInfo):Message {
 		final fromAttr = stanza.attr.get("from");
 		final from = fromAttr == null ? localJid.domain : fromAttr;
-		final encryptionInfo = EncryptionInfo.fromStanza(stanza);
+		if(encryptionInfo==null) {
+			encryptionInfo = EncryptionInfo.fromStanza(stanza);
+		}
 
 		if (stanza.attr.get("type") == "error") {
 			return new Message(from, from, null, ErrorMessageStanza(stanza), encryptionInfo);
diff --git a/snikket/MessageSync.hx b/snikket/MessageSync.hx
index 2fe08ec..70229d8 100644
--- a/snikket/MessageSync.hx
+++ b/snikket/MessageSync.hx
@@ -93,12 +93,14 @@ class MessageSync {
 			if (originalMessage.hasChild("encrypted", NS.OMEMO)) {
 #if !NO_OMEMO
 				trace("MAM: Processing OMEMO message from " + originalMessage.attr.get("from"));
-				promisedMessages.push(client.omemo.decryptMessage(originalMessage).then((decryptedStanza) -> {
+				promisedMessages.push(client.omemo.decryptMessage(originalMessage).then((decryptionResult) -> {
+					final decryptedStanza = decryptionResult.stanza;
 					trace("MAM: Decrypted stanza: "+decryptedStanza);
 
 					final msg = Message.fromStanza(decryptedStanza, client.jid, (builder, stanza) -> {
 						builder.serverId = result.attr.get("id");
 						builder.serverIdBy = serviceJID;
+						builder.encryption = decryptionResult.encryptionInfo;
 						if (timestamp != null && builder.timestamp == null) builder.timestamp = timestamp;
 						return contextHandler(builder, stanza);
 					}).parsed;
diff --git a/snikket/OMEMO.hx b/snikket/OMEMO.hx
index f28440f..b496948 100644
--- a/snikket/OMEMO.hx
+++ b/snikket/OMEMO.hx
@@ -304,6 +304,16 @@ class OMEMOEncryptionResult {
 	}
 }
 
+class OMEMODecryptionResult {
+	public final stanza:Stanza;
+	public final encryptionInfo:EncryptionInfo;
+
+	public function new(stanza:Stanza, encryptionInfo:EncryptionInfo) {
+		this.stanza = stanza;
+		this.encryptionInfo = encryptionInfo;
+	}
+}
+
 //@:nullSafety(Strict)
 class OMEMO {
 	private final client: Client;
@@ -840,48 +850,72 @@ class OMEMO {
 		});
 	}
 
-	public function decryptMessage(stanza: Stanza):Promise<Stanza> {
+	public function decryptMessage(stanza: Stanza):Promise<OMEMODecryptionResult> {
 		final header = OMEMOPayload.fromMessageStanza(stanza);
 		return client.omemo.getDeviceId().then((deviceId:Int) -> {
 			final deviceKey = header.findKey(deviceId);
 			if(deviceKey == null) {
 				trace("OMEMO: Message not encrypted for our device (looked for "+deviceId+")");
 				stanza.removeChildren("encrypted", NS.OMEMO);
-				return Promise.resolve(stanza);
+				return Promise.resolve(
+					new OMEMODecryptionResult(
+						stanza,
+						new EncryptionInfo(
+							DecryptionFailure,
+							NS.OMEMO,
+							"missing-key",
+							"Sender did not include this device in recipients"
+						)
+					)
+				);
 			}
 			// FIXME: Identify correct JID for group chats
 			trace("OMEMO: Decrypting payload...");
 			final from = JID.parse(stanza.attr.get("from")).asBare();
 			final promPayload = decryptPayload(deviceId, deviceKey, from.asString(), header);
 			return promPayload.then((decryptedPayload:BytesData) -> {
-				if(decryptedPayload != null) {
-					stanza.removeChildren("body");
-					// FIXME: Verify valid UTF-8, etc.
-					stanza.textTag("body", Bytes.ofData(decryptedPayload).toString());
-					stanza.tag("decryption-status", {
-							xmlns: "https://snikket.org/protocol/sdk",
-							result: "success",
-							encryption: "eu.siacs.conversations.axolotl",
-					});
-					trace("OMEMO: Payload decrypted OK!");
-				} else {
+				if(decryptedPayload == null) {
 					trace("OMEMO: Decrypted payload is null?");
+					return Promise.resolve(new OMEMODecryptionResult(
+						stanza,
+						new EncryptionInfo(
+							DecryptionFailure,
+							NS.OMEMO,
+							"invalid-payload",
+							"The encrypted message was malformed"
+						)
+					));
 				}
-				return Promise.resolve(stanza);
+
+				stanza.removeChildren("body");
+				// FIXME: Verify valid UTF-8, etc.
+				stanza.textTag("body", Bytes.ofData(decryptedPayload).toString());
+				trace("OMEMO: Payload decrypted OK!");
+				return Promise.resolve(new OMEMODecryptionResult(
+					stanza,
+					new EncryptionInfo(
+						DecryptionSuccess,
+						NS.OMEMO,
+					)
+				));
 			}, (err:Any) -> {
 				trace("OMEMO: Failed to decrypt message: " + err);
-				stanza.tag("decryption-status", {
-					xmlns: "https://snikket.org/protocol/sdk",
-					result: "failure",
-					encryption: "eu.siacs.conversations.axolotl",
-					reason: "generic",
-					text: err,
-				});
+				// FIXME: Rebuilding the session should not be unconditional, as this
+				// can be triggered by a MITM and effectively bypass the double ratchet
+				// part of the protocol.
 				buildSession(deviceId, from.asString(), header.sid).then((session) -> {
 					// Broken session? Send key to start new session...
 					sendKeyExchange(deviceId, from.asString(), header.sid);
 				});
-				return Promise.resolve(stanza);
+				return Promise.resolve(new OMEMODecryptionResult(
+					stanza,
+					new EncryptionInfo(
+						DecryptionFailure,
+						NS.OMEMO,
+						"generic",
+						err,
+					)
+				));
 			});
 		});
 	}