Improved malformed file handling for OpenSSH Private Keys (#898)

src/main/java/com/hierynomus/sshj/userauth/keyprovider/OpenSSHKeyV1KeyFile.java

@@ -98,19 +98,19 @@ public class OpenSSHKeyV1KeyFile extends BaseFileKeyProvider {
 
     @Override
     protected KeyPair readKeyPair() throws IOException {
-        BufferedReader reader = new BufferedReader(resource.getReader());
+        final BufferedReader reader = new BufferedReader(resource.getReader());
         try {
-            if (!checkHeader(reader)) {
+            if (checkHeader(reader)) {
-                throw new IOException("This key is not in 'openssh-key-v1' format");
+                final String encodedPrivateKey = readEncodedKey(reader);
+                byte[] decodedPrivateKey = Base64.getDecoder().decode(encodedPrivateKey);
+                final PlainBuffer bufferedPrivateKey = new PlainBuffer(decodedPrivateKey);
+                return readDecodedKeyPair(bufferedPrivateKey);
+            } else {
+                final String message = String.format("File header not found [%s%s]", BEGIN, OPENSSH_PRIVATE_KEY);
+                throw new IOException(message);
             }
-
+        } catch (final GeneralSecurityException e) {
-            String keyFile = readKeyFile(reader);
+            throw new SSHRuntimeException("Read OpenSSH Version 1 Key failed", e);
-            byte[] decode = Base64.getDecoder().decode(keyFile);
-            PlainBuffer keyBuffer = new PlainBuffer(decode);
-            return readDecodedKeyPair(keyBuffer);
-
-        } catch (GeneralSecurityException e) {
-            throw new SSHRuntimeException(e);
         } finally {
             IOUtils.closeQuietly(reader);
         }
@@ -149,7 +149,7 @@ public class OpenSSHKeyV1KeyFile extends BaseFileKeyProvider {
             logger.debug("Reading unencrypted keypair");
             return readUnencrypted(privateKeyBuffer, publicKey);
         } else {
-            logger.info("Keypair is encrypted with: " + cipherName + ", " + kdfName + ", " + Arrays.toString(kdfOptions));
+            logger.info("Keypair is encrypted with: {}, {}, {}", cipherName, kdfName, Arrays.toString(kdfOptions));
             while (true) {
                 PlainBuffer decryptionBuffer = new PlainBuffer(privateKeyBuffer);
                 PlainBuffer decrypted = decryptBuffer(decryptionBuffer, cipherName, kdfName, kdfOptions);
@@ -209,14 +209,26 @@ public class OpenSSHKeyV1KeyFile extends BaseFileKeyProvider {
         return KeyType.fromString(plainBuffer.readString()).readPubKeyFromBuffer(plainBuffer);
     }
 
-    private String readKeyFile(final BufferedReader reader) throws IOException {
+    private String readEncodedKey(final BufferedReader reader) throws IOException {
-        StringBuilder sb = new StringBuilder();
+        final StringBuilder builder = new StringBuilder();
+
+        boolean footerFound = false;
         String line = reader.readLine();
-        while (!line.startsWith(END)) {
+        while (line != null) {
-            sb.append(line);
+            if (line.startsWith(END)) {
+                footerFound = true;
+                break;
+            }
+            builder.append(line);
             line = reader.readLine();
         }
-        return sb.toString();
+
+        if (footerFound) {
+            return builder.toString();
+        } else {
+            final String message = String.format("File footer not found [%s%s]", END, OPENSSH_PRIVATE_KEY);
+            throw new IOException(message);
+        }
     }
 
     private boolean checkHeader(final BufferedReader reader) throws IOException {
@@ -244,7 +256,7 @@ public class OpenSSHKeyV1KeyFile extends BaseFileKeyProvider {
         // The private key section contains both the public key and the private key
         String keyType = keyBuffer.readString(); // string keytype
         KeyType kt = KeyType.fromString(keyType);
-        logger.info("Read key type: {}", keyType, kt);
+
         KeyPair kp;
         switch (kt) {
             case ED25519:

src/test/java/net/schmizz/sshj/keyprovider/OpenSSHKeyFileTest.java

@@ -25,7 +25,6 @@ import net.schmizz.sshj.userauth.password.PasswordFinder;
 import net.schmizz.sshj.userauth.password.PasswordUtils;
 import net.schmizz.sshj.userauth.password.Resource;
 import net.schmizz.sshj.util.KeyUtil;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 
@@ -226,6 +225,22 @@ public class OpenSSHKeyFileTest {
         assertThat(aPrivate.getAlgorithm(), equalTo("EdDSA"));
     }
 
+    @Test
+    public void shouldHandlePrivateKeyMissingHeader() {
+        OpenSSHKeyV1KeyFile keyFile = new OpenSSHKeyV1KeyFile();
+        keyFile.init(new File("src/test/resources/keyformats/pkcs8"));
+        final IOException exception = assertThrows(IOException.class, keyFile::getPrivate);
+        assertTrue(exception.getMessage().contains("header not found"));
+    }
+
+    @Test
+    public void shouldHandlePrivateKeyMissingFooter() {
+        final OpenSSHKeyV1KeyFile keyFile = new OpenSSHKeyV1KeyFile();
+        keyFile.init(new File("src/test/resources/keytypes/test_ed25519_missing_footer"));
+        final IOException exception = assertThrows(IOException.class, keyFile::getPrivate);
+        assertTrue(exception.getMessage().contains("footer not found"));
+    }
+
     @Test
     public void shouldLoadProtectedED25519PrivateKeyAes256CTR() throws IOException {
         checkOpenSSHKeyV1("src/test/resources/keytypes/ed25519_protected", "sshjtest", false);
@@ -245,7 +260,7 @@ public class OpenSSHKeyFileTest {
     }
 
     @Test
-    public void shouldFailOnIncorrectPassphraseAfterRetries() throws IOException {
+    public void shouldFailOnIncorrectPassphraseAfterRetries() {
         assertThrows(KeyDecryptionFailedException.class, () -> {
         OpenSSHKeyV1KeyFile keyFile = new OpenSSHKeyV1KeyFile();
         keyFile.init(new File("src/test/resources/keytypes/ed25519_aes256cbc.pem"), new PasswordFinder() {

src/test/resources/keytypes/test_ed25519_missing_footer

@@ -0,0 +1,6 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
+QyNTUxOQAAACAwHSYkZJATPMgvLHkxKAJ9j38Gyyq5HGoWdMcT6FiAiQAAAJDimgR84poE
+fAAAAAtzc2gtZWQyNTUxOQAAACAwHSYkZJATPMgvLHkxKAJ9j38Gyyq5HGoWdMcT6FiAiQ
+AAAECmsckQycWnfGQK6XtQpaMGODbAkMQOdJNK6XJSipB7dDAdJiRkkBM8yC8seTEoAn2P
+fwbLKrkcahZ0xxPoWICJAAAACXJvb3RAc3NoagECAwQ=