OpenSSHKey: correctly parse aes-256-cbc/ctr keys #1682

tycho · 2018-03-08T05:10:07Z

Description

The code for parsing OpenSSH private key files is broken for aes-256-cbc and aes-256-ctr encrypted keys in PEM format.

Motivation and context

... to correctly load encrypted SSH keys.

How has this been tested?

Not fully tested yet.

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ I have added tests to cover my changes.

tycho · 2018-03-08T05:16:51Z

There's some work to be done to fix the tests, working on that now.

tycho · 2018-03-08T05:55:36Z

I've updated the tests to include an AES-256-CBC and AES-256-CTR key, as emitted by openssl rsa -aes-256-cbc and openssl rsa -aes-256-ctr. However, they fail to decrypt properly with KeePassXC for reasons unknown to me. OpenSSL doesn't have any trouble opening/decrypting them, but KeePassXC is doing something funny:

$ ninja testopensshkey && tests/testopensshkey
[11/11] Linking CXX executable tests/testopensshkey
********* Start testing of TestOpenSSHKey *********
Config: Using QtTest library 5.10.1, Qt 5.10.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 7.3.0)
PASS   : TestOpenSSHKey::initTestCase()
PASS   : TestOpenSSHKey::testParse()
PASS   : TestOpenSSHKey::testParseDSA()
PASS   : TestOpenSSHKey::testParseRSA()
PASS   : TestOpenSSHKey::testDecryptAES128CBC()
PASS   : TestOpenSSHKey::testDecryptAES256CBC()
FAIL!  : TestOpenSSHKey::testDecryptPemAES256CBC() 'key.openPrivateKey("correctpassphrase")' returned FALSE. ()
   Loc: [../tests/TestOpenSSHKey.cpp(260)]
PASS   : TestOpenSSHKey::testDecryptAES256CTR()
FAIL!  : TestOpenSSHKey::testDecryptPemAES256CTR() 'key.openPrivateKey("correctpassphrase")' returned FALSE. ()
   Loc: [../tests/TestOpenSSHKey.cpp(342)]
PASS   : TestOpenSSHKey::cleanupTestCase()
Totals: 8 passed, 2 failed, 0 skipped, 0 blacklisted, 782ms
********* Finished testing of TestOpenSSHKey *********

tycho · 2018-03-08T19:09:46Z

If anyone can tell me why it's not able to decrypt those two encrypted RSA keys in the two new tests, that'd be great. OpenSSL doesn't have any trouble with them, and I'm not sure what's going wrong in KeePassXC.

tycho · 2018-03-09T00:01:02Z

Found the problem. It was a key size issue. We were using a 16-byte key and not filling out the full 32-byte width required for AES256.

hifi · 2018-03-09T05:32:05Z

src/sshagent/OpenSSHKey.cpp

-        QByteArray keyData = hash.result();
+        QByteArray keyData;
+        QByteArray mdBuf;
+        mdBuf.resize(0);


Freshly constructed QByteArray is zero length so this would be superfluous.

True enough. I'll drop that.

hifi · 2018-03-09T05:38:57Z

tests/TestOpenSSHKey.cpp

@@ -90,39 +90,39 @@ void TestOpenSSHKey::testParseDSA()
    QCOMPARE(key.fingerprint(), QString("SHA256:tbbNuLN1hja8JNASDTlLOZQsbTlJDzJlz/oAGK3sX18"));
 }

-void TestOpenSSHKey::testDecryptAES128CBC()
+void TestOpenSSHKey::testDecryptRSAAES128CBC()


This is nitpicking on something I started but maybe this naming scheme doesn't work any more. I'm thinking of using the KDF rather than "RSA" or "PEM". So "MD5" in this case and "BCrypt" for encrypted OpenSSH keys. Not really important anyway.

The naming scheme made me cringe a bit as I was writing it. But there is a distinction to be made between RSA/OpenSSH key files.

hifi · 2018-03-09T16:19:41Z

Looks good to me. Could you please rebase this against release/2.3.2? Thanks.

tycho · 2018-03-09T16:28:27Z

Done!

phoerious · 2018-03-10T09:05:44Z

src/sshagent/OpenSSHKey.cpp

-        QByteArray keyData = hash.result();
+        QByteArray keyData;
+        QByteArray mdBuf;
+        for (;;) {


You should use a do/while loop instead.

That's really nitpicky.

Maybe, but it's ensuring we are keeping a consistent style. And TBH, an empty for loop header is just extremely ugly.

Changed, but I'm not sure it looks any better.

It does. Thank you very much.

varjolintu · 2018-03-10T09:20:02Z

src/sshagent/OpenSSHKey.cpp

        cipher.reset(new SymmetricCipher(SymmetricCipher::Aes256, SymmetricCipher::Cbc, SymmetricCipher::Decrypt));
-    } else if (m_cipherName == "aes256-ctr") {
+    } else if (m_cipherName == "aes256-ctr" || m_cipherName.compare("aes-256-ctr", Qt::CaseInsensitive) == 0) {


Why use comparison operator with the first one but compare()with the second one?

Could probably just make it a comparison operator, but the .compare is copying the behavior used for the aes-128-cbc above this.

With the OpenSSH key type it is presumably fixed lower case while with encrypted RSA PEMs it could be upper or lower case? I don't think I've ever seen an encrypted RSA PEM with DEK-Info parameter of aes-128-cbc in lower case, but it seems this code was already prepared for that possibility. Could just move them both to == operators.

compare is used for case-insensitive comparison. == cannot do that.

The DEK-Info cipher was normalized to lowercase as the OpenSSH key types are lowercase as well.

varjolintu · 2018-03-10T09:21:57Z

src/sshagent/OpenSSHKey.cpp

@@ -319,9 +319,9 @@ bool OpenSSHKey::openPrivateKey(const QString& passphrase)

    if (m_cipherName.compare("aes-128-cbc", Qt::CaseInsensitive) == 0) {
        cipher.reset(new SymmetricCipher(SymmetricCipher::Aes128, SymmetricCipher::Cbc, SymmetricCipher::Decrypt));
-    } else if (m_cipherName == "aes256-cbc") {
+    } else if (m_cipherName == "aes256-cbc" || m_cipherName.compare("aes-256-cbc", Qt::CaseInsensitive) == 0) {


Same == versus .compare() here.

AES-256 uses a 32-byte (256-bit) key size. This un-breaks the loader and tests added for AES-256-CBC and AES-256-CTR PEM keys. Signed-off-by: Steven Noonan <[email protected]>

Signed-off-by: Steven Noonan <[email protected]>

droidmonkey · 2018-04-05T01:19:11Z

Ping @tycho @phoerious @hifi is this ready for merge?

tycho · 2018-04-05T01:30:42Z

I believe it's been ready for some time. Just waiting on someone to sign off on it...

changes were made

- Enable high entropy ASLR on Windows [#1747] - Enhance favicon fetching [#1786] - Fix crash on Windows due to autotype [#1691] - Fix dark tray icon changing all icons [#1680] - Fix --pw-stdin not using getPassword function [#1686] - Fix placeholders being resolved in notes [#1907] - Enable auto-type start delay to be configurable [#1908] - Browser: Fix native messaging reply size [#1719] - Browser: Increase maximum buffer size [#1720] - Browser: Enhance usability and functionality [#1810, #1822, #1830, #1884, #1906] - SSH Agent: Parse aes-256-cbc/ctr keys [#1682] - SSH Agent: Enhance usability and functionality [#1677, #1679, #1681, #1787]

AES-256 uses a 32-byte (256-bit) key size. This un-breaks the loader and tests added for AES-256-CBC and AES-256-CTR PEM keys. * OpenSSHKey: correctly parse encrypted PEM AES-256-CBC/AES-256-CTR keys * OpenSSHKey: use correct key derivation for AES-256

tycho force-pushed the ssh-key-parse-fix branch from ecf3a7f to 7322148 Compare March 8, 2018 05:53

TheZ3ro requested review from hifi and a team March 8, 2018 11:13

TheZ3ro added feature: SSH agent needs work labels Mar 8, 2018

tycho force-pushed the ssh-key-parse-fix branch from 7322148 to c2d8886 Compare March 9, 2018 00:01

hifi removed the needs work label Mar 9, 2018

hifi requested changes Mar 9, 2018

View reviewed changes

tycho force-pushed the ssh-key-parse-fix branch from c2d8886 to 5e33c2c Compare March 9, 2018 05:45

hifi approved these changes Mar 9, 2018

View reviewed changes

tycho force-pushed the ssh-key-parse-fix branch from 5e33c2c to 8b7102d Compare March 9, 2018 16:27

tycho changed the base branch from develop to release/2.3.2 March 9, 2018 16:30

phoerious previously requested changes Mar 10, 2018

View reviewed changes

varjolintu reviewed Mar 10, 2018

View reviewed changes

tycho force-pushed the ssh-key-parse-fix branch 2 times, most recently from 996a5e8 to a5e607b Compare March 10, 2018 18:52

droidmonkey added this to the v2.3.2 milestone Mar 11, 2018

tycho force-pushed the ssh-key-parse-fix branch from a5e607b to e0d9918 Compare March 18, 2018 08:28

tycho added 2 commits March 25, 2018 00:08

OpenSSHKey: use correct key derivation for AES-256

ca25cd4

AES-256 uses a 32-byte (256-bit) key size. This un-breaks the loader and tests added for AES-256-CBC and AES-256-CTR PEM keys. Signed-off-by: Steven Noonan <[email protected]>

OpenSSHKey: correctly parse encrypted PEM AES-256-CBC/AES-256-CTR keys

2202f07

Signed-off-by: Steven Noonan <[email protected]>

tycho force-pushed the ssh-key-parse-fix branch from e0d9918 to 2202f07 Compare March 25, 2018 07:09

droidmonkey merged commit c21f4b5 into keepassxreboot:release/2.3.2 Apr 5, 2018

tycho deleted the ssh-key-parse-fix branch April 5, 2018 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenSSHKey: correctly parse aes-256-cbc/ctr keys #1682

OpenSSHKey: correctly parse aes-256-cbc/ctr keys #1682

tycho commented Mar 8, 2018 •

edited

Loading

tycho commented Mar 8, 2018

tycho commented Mar 8, 2018 •

edited

Loading

tycho commented Mar 8, 2018

tycho commented Mar 9, 2018

hifi Mar 9, 2018

tycho Mar 9, 2018

hifi Mar 9, 2018

tycho Mar 9, 2018

hifi commented Mar 9, 2018

tycho commented Mar 9, 2018

phoerious Mar 10, 2018

tycho Mar 10, 2018

phoerious Mar 10, 2018

tycho Mar 10, 2018

phoerious Mar 10, 2018

varjolintu Mar 10, 2018

tycho Mar 10, 2018 •

edited

Loading

phoerious Mar 10, 2018

hifi Mar 11, 2018

varjolintu Mar 10, 2018

droidmonkey commented Apr 5, 2018

tycho commented Apr 5, 2018

OpenSSHKey: correctly parse aes-256-cbc/ctr keys #1682

OpenSSHKey: correctly parse aes-256-cbc/ctr keys #1682

Conversation

tycho commented Mar 8, 2018 • edited Loading

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

tycho commented Mar 8, 2018

tycho commented Mar 8, 2018 • edited Loading

tycho commented Mar 8, 2018

tycho commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hifi commented Mar 9, 2018

tycho commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tycho Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droidmonkey commented Apr 5, 2018

tycho commented Apr 5, 2018

tycho commented Mar 8, 2018 •

edited

Loading

tycho commented Mar 8, 2018 •

edited

Loading

tycho Mar 10, 2018 •

edited

Loading