From 1ee32857cf2633783ee47e241c11f5de976f5b0f Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sat, 13 Dec 2025 15:28:52 +0100 Subject: [PATCH 1/2] Add more error checks to ASN1Key.cpp I/O operations might fail, so test if they succeed. While at it, use newly introduced VALIDATE_RETURN for parsePrivateHeader as well. --- src/sshagent/ASN1Key.cpp | 86 ++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/src/sshagent/ASN1Key.cpp b/src/sshagent/ASN1Key.cpp index abd28a31b..3016e6d2a 100644 --- a/src/sshagent/ASN1Key.cpp +++ b/src/sshagent/ASN1Key.cpp @@ -20,6 +20,8 @@ #include "BinaryStream.h" #include "OpenSSHKey.h" +#define VALIDATE_RETURN(call) if (!call) { return false; } + namespace { constexpr quint8 TAG_INT = 0x02; @@ -28,22 +30,22 @@ namespace bool nextTag(BinaryStream& stream, quint8& tag, quint32& len) { - stream.read(tag); + VALIDATE_RETURN(stream.read(tag)); quint8 lenByte; - stream.read(lenByte); + VALIDATE_RETURN(stream.read(lenByte)); if (lenByte & 0x80) { quint32 bytes = lenByte & ~0x80; if (bytes == 1) { - stream.read(lenByte); + VALIDATE_RETURN(stream.read(lenByte)); len = lenByte; } else if (bytes == 2) { quint16 lenShort; - stream.read(lenShort); + VALIDATE_RETURN(stream.read(lenShort)); len = lenShort; } else if (bytes == 4) { - stream.read(len); + VALIDATE_RETURN(stream.read(len)); } else { return false; } @@ -59,20 +61,20 @@ namespace quint8 tag; quint32 len; - nextTag(stream, tag, len); + VALIDATE_RETURN(nextTag(stream, tag, len)); if (tag != TAG_SEQUENCE) { return false; } - nextTag(stream, tag, len); + VALIDATE_RETURN(nextTag(stream, tag, len)); if (tag != TAG_INT || len != 1) { return false; } quint8 keyType; - stream.read(keyType); + VALIDATE_RETURN(stream.read(keyType)); return (keyType == wantedType); } @@ -82,14 +84,14 @@ namespace quint8 tag; quint32 len; - nextTag(stream, tag, len); + VALIDATE_RETURN(nextTag(stream, tag, len)); if (tag != TAG_INT) { return false; } target.resize(len); - stream.read(target); + VALIDATE_RETURN(stream.read(target)); return true; } } // namespace @@ -103,26 +105,26 @@ bool ASN1Key::parseDSA(QByteArray& ba, OpenSSHKey& key) } QByteArray p, q, g, y, x; - readInt(stream, p); - readInt(stream, q); - readInt(stream, g); - readInt(stream, y); - readInt(stream, x); + VALIDATE_RETURN(stream.read(p)); + VALIDATE_RETURN(stream.read(q)); + VALIDATE_RETURN(stream.read(g)); + VALIDATE_RETURN(stream.read(y)); + VALIDATE_RETURN(stream.read(x)); QByteArray publicData; BinaryStream publicDataStream(&publicData); - publicDataStream.writeString(p); - publicDataStream.writeString(q); - publicDataStream.writeString(g); - publicDataStream.writeString(y); + VALIDATE_RETURN(publicDataStream.writeString(p)); + VALIDATE_RETURN(publicDataStream.writeString(q)); + VALIDATE_RETURN(publicDataStream.writeString(g)); + VALIDATE_RETURN(publicDataStream.writeString(y)); QByteArray privateData; BinaryStream privateDataStream(&privateData); - privateDataStream.writeString(p); - privateDataStream.writeString(q); - privateDataStream.writeString(g); - privateDataStream.writeString(y); - privateDataStream.writeString(x); + VALIDATE_RETURN(privateDataStream.writeString(p)); + VALIDATE_RETURN(privateDataStream.writeString(q)); + VALIDATE_RETURN(privateDataStream.writeString(g)); + VALIDATE_RETURN(privateDataStream.writeString(y)); + VALIDATE_RETURN(privateDataStream.writeString(x)); key.setType("ssh-dss"); key.setPublicData(publicData); @@ -135,34 +137,32 @@ bool ASN1Key::parseRSA(QByteArray& ba, OpenSSHKey& key) { BinaryStream stream(&ba); - if (!parsePrivateHeader(stream, KEY_ZERO)) { - return false; - } + VALIDATE_RETURN(parsePrivateHeader(stream, KEY_ZERO)); QByteArray n, e, d, p, q, dp, dq, qinv; - readInt(stream, n); - readInt(stream, e); - readInt(stream, d); - readInt(stream, p); - readInt(stream, q); - readInt(stream, dp); - readInt(stream, dq); - readInt(stream, qinv); + VALIDATE_RETURN(readInt(stream, n)); + VALIDATE_RETURN(readInt(stream, e)); + VALIDATE_RETURN(readInt(stream, d)); + VALIDATE_RETURN(readInt(stream, p)); + VALIDATE_RETURN(readInt(stream, q)); + VALIDATE_RETURN(readInt(stream, dp)); + VALIDATE_RETURN(readInt(stream, dq)); + VALIDATE_RETURN(readInt(stream, qinv)); // Note: To properly calculate the key fingerprint, e and n are reversed per RFC 4253 QByteArray publicData; BinaryStream publicDataStream(&publicData); - publicDataStream.writeString(e); - publicDataStream.writeString(n); + VALIDATE_RETURN(publicDataStream.writeString(e)); + VALIDATE_RETURN(publicDataStream.writeString(n)); QByteArray privateData; BinaryStream privateDataStream(&privateData); - privateDataStream.writeString(n); - privateDataStream.writeString(e); - privateDataStream.writeString(d); - privateDataStream.writeString(qinv); - privateDataStream.writeString(p); - privateDataStream.writeString(q); + VALIDATE_RETURN(privateDataStream.writeString(n)); + VALIDATE_RETURN(privateDataStream.writeString(e)); + VALIDATE_RETURN(privateDataStream.writeString(d)); + VALIDATE_RETURN(privateDataStream.writeString(qinv)); + VALIDATE_RETURN(privateDataStream.writeString(p)); + VALIDATE_RETURN(privateDataStream.writeString(q)); key.setType("ssh-rsa"); key.setPublicData(publicData); From 66be8e8545e4a2473712c438df104a593c7722ef Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sat, 13 Dec 2025 15:30:08 +0100 Subject: [PATCH 2/2] Add maximum integer length to ASN1Key.cpp Values around 2 GB lead to abort() calls within Qt 5. --- src/sshagent/ASN1Key.cpp | 2 +- tests/TestOpenSSHKey.cpp | 12 ++++++++++++ tests/TestOpenSSHKey.h | 1 + 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/sshagent/ASN1Key.cpp b/src/sshagent/ASN1Key.cpp index 3016e6d2a..2e6fa4222 100644 --- a/src/sshagent/ASN1Key.cpp +++ b/src/sshagent/ASN1Key.cpp @@ -86,7 +86,7 @@ namespace VALIDATE_RETURN(nextTag(stream, tag, len)); - if (tag != TAG_INT) { + if (tag != TAG_INT || len > 1024 * 1024 * 10) { return false; } diff --git a/tests/TestOpenSSHKey.cpp b/tests/TestOpenSSHKey.cpp index a20b248ed..45a2ee453 100644 --- a/tests/TestOpenSSHKey.cpp +++ b/tests/TestOpenSSHKey.cpp @@ -177,6 +177,18 @@ void TestOpenSSHKey::testParseRSA() QCOMPARE(key.fingerprint(QCryptographicHash::Md5), QString("MD5:c2:26:5b:3d:62:19:56:b0:c3:67:99:7a:a6:4c:66:06")); } +void TestOpenSSHKey::testParseRSABroken() +{ + const QString keyString = QString("-----BEGIN RSA PRIVATE KEY-----\n" + "MAACAQAChH////8=\n" + "-----END RSA PRIVATE KEY-----\n"); + + const QByteArray keyData = keyString.toLatin1(); + + OpenSSHKey key; + QVERIFY(!key.parsePKCS1PEM(keyData)); +} + void TestOpenSSHKey::testParseRSACompare() { const QString oldKeyString = QString("-----BEGIN RSA PRIVATE KEY-----\n" diff --git a/tests/TestOpenSSHKey.h b/tests/TestOpenSSHKey.h index b80ff919c..2a73afb1f 100644 --- a/tests/TestOpenSSHKey.h +++ b/tests/TestOpenSSHKey.h @@ -31,6 +31,7 @@ private slots: void testParse(); void testParseDSA(); void testParseRSA(); + void testParseRSABroken(); void testParseRSACompare(); void testParseECDSA256(); void testParseECDSA384();