From 61a5c500af889e033b5d0731ec511a3a314b1b5b Mon Sep 17 00:00:00 2001
From: Sergei Razmetov <razmser@gmail.com>
Date: Fri, 12 Dec 2025 14:10:01 +0300
Subject: [PATCH] Fix SSH ECDSA signature mpint encoding

OpenSSHSignatureWriter was emitting non-canonical mpints (keeping
fixed-width leading 0x00 bytes), which breaks strict parsers.
Canonicalize r/s mpints and add a regression test.
---
 .../OpenSSH/OpenSSHSignatureWriter.swift      |  31 +++--
 .../OpenSSHSignatureWriterTests.swift         | 110 ++++++++++++++++++
 2 files changed, 130 insertions(+), 11 deletions(-)
 create mode 100644 Sources/Packages/Tests/SecretKitTests/OpenSSHSignatureWriterTests.swift

diff --git a/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHSignatureWriter.swift b/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHSignatureWriter.swift
index b713d53..6b31de5 100644
--- a/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHSignatureWriter.swift
+++ b/Sources/Packages/Sources/SecretKit/OpenSSH/OpenSSHSignatureWriter.swift
@@ -29,19 +29,28 @@ public struct OpenSSHSignatureWriter: Sendable {
 
 extension OpenSSHSignatureWriter {
 
+    /// Converts a fixed-width big-endian integer (e.g. r/s from CryptoKit rawRepresentation) into an SSH mpint.
+    /// Strips unnecessary leading zeros and prefixes `0x00` if needed to keep the value positive.
+    private func mpint(fromFixedWidthPositiveBytes bytes: Data) -> Data {
+        // mpint zero is encoded as a string with zero bytes of data.
+        guard let firstNonZeroIndex = bytes.firstIndex(where: { $0 != 0x00 }) else {
+            return Data()
+        }
+
+        let trimmed = Data(bytes[firstNonZeroIndex...])
+
+        if let first = trimmed.first, first >= 0x80 {
+            var prefixed = Data([0x00])
+            prefixed.append(trimmed)
+            return prefixed
+        }
+        return trimmed
+    }
+
     func ecdsaSignature(_ rawRepresentation: Data, keyType: KeyType) -> Data {
         let rawLength = rawRepresentation.count/2
-        // Check if we need to pad with 0x00 to prevent certain
-        // ssh servers from thinking r or s is negative
-        let paddingRange: ClosedRange<UInt8> = 0x80...0xFF
-        var r = Data(rawRepresentation[0..<rawLength])
-        if paddingRange ~= r.first! {
-            r.insert(0x00, at: 0)
-        }
-        var s = Data(rawRepresentation[rawLength...])
-        if paddingRange ~= s.first! {
-            s.insert(0x00, at: 0)
-        }
+        let r = mpint(fromFixedWidthPositiveBytes: Data(rawRepresentation[0..<rawLength]))
+        let s = mpint(fromFixedWidthPositiveBytes: Data(rawRepresentation[rawLength...]))
 
         var signatureChunk = Data()
         signatureChunk.append(r.lengthAndData)
diff --git a/Sources/Packages/Tests/SecretKitTests/OpenSSHSignatureWriterTests.swift b/Sources/Packages/Tests/SecretKitTests/OpenSSHSignatureWriterTests.swift
new file mode 100644
index 0000000..51ff4a3
--- /dev/null
+++ b/Sources/Packages/Tests/SecretKitTests/OpenSSHSignatureWriterTests.swift
@@ -0,0 +1,110 @@
+import Foundation
+import Testing
+@testable import SecretKit
+@testable import SmartCardSecretKit
+
+@Suite struct OpenSSHSignatureWriterTests {
+
+    private let writer = OpenSSHSignatureWriter()
+
+    @Test func ecdsaMpintStripsUnnecessaryLeadingZeros() throws {
+        let secret = Constants.ecdsa256Secret
+
+        // r has a leading 0x00 followed by 0x01 (< 0x80): the mpint must not keep the leading zero.
+        let rBytes: [UInt8] = [0x00] + (1...31).map { UInt8($0) }
+        let r = Data(rBytes)
+        // s has two leading 0x00 bytes followed by 0x7f (< 0x80): the mpint must not keep the leading zeros.
+        let sBytes: [UInt8] = [0x00, 0x00, 0x7f] + Array(repeating: UInt8(0x01), count: 29)
+        let s = Data(sBytes)
+        let rawRepresentation = r + s
+
+        let response = writer.data(secret: secret, signature: rawRepresentation)
+        let (parsedR, parsedS) = try parseEcdsaSignatureMpints(from: response)
+
+        #expect(parsedR == Data((1...31).map { UInt8($0) }))
+        #expect(parsedS == Data([0x7f] + Array(repeating: UInt8(0x01), count: 29)))
+    }
+
+    @Test func ecdsaMpintPrefixesZeroWhenHighBitSet() throws {
+        let secret = Constants.ecdsa256Secret
+
+        // r starts with 0x80 (high bit set): mpint must be prefixed with 0x00.
+        let r = Data([UInt8(0x80)] + Array(repeating: UInt8(0x01), count: 31))
+        let s = Data([UInt8(0x01)] + Array(repeating: UInt8(0x02), count: 31))
+        let rawRepresentation = r + s
+
+        let response = writer.data(secret: secret, signature: rawRepresentation)
+        let (parsedR, parsedS) = try parseEcdsaSignatureMpints(from: response)
+
+        #expect(parsedR == Data([0x00, 0x80] + Array(repeating: UInt8(0x01), count: 31)))
+        #expect(parsedS == Data([0x01] + Array(repeating: UInt8(0x02), count: 31)))
+    }
+
+}
+
+private extension OpenSSHSignatureWriterTests {
+
+    enum Constants {
+        static let ecdsa256Secret = SmartCard.Secret(
+            id: Data(),
+            name: "Test Key (ECDSA 256)",
+            publicKey: Data(repeating: 0x01, count: 65),
+            attributes: Attributes(
+                keyType: KeyType(algorithm: .ecdsa, size: 256),
+                authentication: .notRequired,
+                publicKeyAttribution: "test@example.com"
+            )
+        )
+    }
+
+    enum ParseError: Error {
+        case eof
+        case invalidLength
+        case invalidAlgorithm
+    }
+
+    struct Reader {
+        var data: Data
+        var offset: Int = 0
+
+        mutating func readU32() throws -> Int {
+            guard offset + 4 <= data.count else { throw ParseError.eof }
+            let value = data[offset..<offset + 4].reduce(0 as UInt32) { ($0 << 8) | UInt32($1) }
+            offset += 4
+            return Int(value)
+        }
+
+        mutating func readBytes(count: Int) throws -> Data {
+            guard count >= 0 else { throw ParseError.invalidLength }
+            guard offset + count <= data.count else { throw ParseError.eof }
+            let out = data[offset..<offset + count]
+            offset += count
+            return Data(out)
+        }
+
+        mutating func readString() throws -> Data {
+            let length = try readU32()
+            return try readBytes(count: length)
+        }
+    }
+
+    func parseEcdsaSignatureMpints(from openSSHSignedData: Data) throws -> (r: Data, s: Data) {
+        var reader = Reader(data: openSSHSignedData)
+
+        let outerLength = try reader.readU32()
+        guard outerLength == (openSSHSignedData.count - 4) else { throw ParseError.invalidLength }
+
+        let algorithm = try reader.readString()
+        guard String(data: algorithm, encoding: .utf8) == "ecdsa-sha2-nistp256" else {
+            throw ParseError.invalidAlgorithm
+        }
+
+        let signatureChunk = try reader.readString()
+        var sigReader = Reader(data: signatureChunk)
+        let r = try sigReader.readString()
+        let s = try sigReader.readString()
+        return (r, s)
+    }
+
+}
+