Skip to content

Commit 70af58d

Browse files
vladimirlagunovvladimirlagunov
authored
Wrap IllegalArgumentException thrown by Base64 decoder (#936)
* Wrap IllegalArgumentException thrown by Base64 decoder Some time ago, there had been `net.schmizz.sshj.common.Base64`. This class used to throw `IOException` in case of any problem. Although `IOException` isn't an appropriate class for indicating on parsing issues, a lot of code has been expecting `IOException` from Base64. Once, the old Base64 decoder was replaced with the one, bundled into Java 14 (see f35c2bd). Copy-paste elimination and switching to standard implementations is undoubtedly a good decision. Unfortunately, `java.util.Base64.Decoder` brought a pesky issue. It throws `IllegalArgumentException` in case of any problem. Since it is an unchecked exception, it was quite challenging to notice it. It's especially challenging because the error appears during processing malformed base64 strings. So, a lot of places in the code kept expecting `IOException`. Sudden `IllegalArgumentException` led to authentication termination in cases where everything used to work perfectly. One of such issues is already found and fixed: 03f8b22 This commit represents a work, based on revising every change made in f35c2bd. It should fix all other similar issues. * squash! Wrap IllegalArgumentException thrown by Base64 decoder Rename Base64DecodeError -> Base64DecodingException * squash! Wrap IllegalArgumentException thrown by Base64 decoder A better warning message in KnownHostMatchers * squash! Wrap IllegalArgumentException thrown by Base64 decoder A better error message in OpenSSHKeyFileUtil * squash! Wrap IllegalArgumentException thrown by Base64 decoder A better error message in OpenSSHKeyV1KeyFile * squash! Wrap IllegalArgumentException thrown by Base64 decoder Get rid of unnecessary `throws IOException` in Base64Decoder * squash! Wrap IllegalArgumentException thrown by Base64 decoder Better error messages in OpenSSHKeyFileUtil and PuTTYKeyFile
1 parent c0d1519 commit 70af58d

File tree

11 files changed

+335
-63
lines changed

11 files changed

+335
-63
lines changed

‎src/main/java/com/hierynomus/sshj/transport/verification/KnownHostMatchers.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
*/
1616
package com.hierynomus.sshj.transport.verification;
1717

18+
import net.schmizz.sshj.common.Base64DecodingException;
19+
import net.schmizz.sshj.common.Base64Decoder;
1820
import net.schmizz.sshj.common.IOUtils;
1921
import net.schmizz.sshj.common.SSHException;
2022
import net.schmizz.sshj.transport.mac.MAC;
@@ -26,9 +28,13 @@
2628
import java.util.regex.Pattern;
2729

2830
import com.hierynomus.sshj.transport.mac.Macs;
31+
import org.slf4j.Logger;
32+
import org.slf4j.LoggerFactory;
2933

3034
public class KnownHostMatchers {
3135

36+
private static final Logger log = LoggerFactory.getLogger(KnownHostMatchers.class);
37+
3238
public static HostMatcher createMatcher(String hostEntry) throws SSHException {
3339
if (hostEntry.contains(",")) {
3440
return new AnyHostMatcher(hostEntry);
@@ -80,17 +86,22 @@ private static class HashedHostMatcher implements HostMatcher {
8086

8187
@Override
8288
public boolean match(String hostname) throws IOException {
83-
return hash.equals(hashHost(hostname));
89+
try {
90+
return hash.equals(hashHost(hostname));
91+
} catch (Base64DecodingException err) {
92+
log.warn("Hostname [{}] not matched: salt decoding failed", hostname, err);
93+
return false;
94+
}
8495
}
8596

86-
private String hashHost(String host) throws IOException {
97+
private String hashHost(String host) throws IOException, Base64DecodingException {
8798
sha1.init(getSaltyBytes());
8899
return "|1|" + salt + "|" + Base64.getEncoder().encodeToString(sha1.doFinal(host.getBytes(IOUtils.UTF8)));
89100
}
90101

91-
private byte[] getSaltyBytes() {
102+
private byte[] getSaltyBytes() throws IOException, Base64DecodingException {
92103
if (saltyBytes == null) {
93-
saltyBytes = Base64.getDecoder().decode(salt);
104+
saltyBytes = Base64Decoder.decode(salt);
94105
}
95106
return saltyBytes;
96107
}

‎src/main/java/com/hierynomus/sshj/userauth/keyprovider/OpenSSHKeyFileUtil.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
*/
1616
package com.hierynomus.sshj.userauth.keyprovider;
1717

18+
import net.schmizz.sshj.common.Base64DecodingException;
19+
import net.schmizz.sshj.common.Base64Decoder;
1820
import net.schmizz.sshj.common.Buffer;
1921
import net.schmizz.sshj.common.KeyType;
2022

@@ -23,7 +25,6 @@
2325
import java.io.IOException;
2426
import java.io.Reader;
2527
import java.security.PublicKey;
26-
import java.util.Base64;
2728

2829
public class OpenSSHKeyFileUtil {
2930
private OpenSSHKeyFileUtil() {
@@ -54,16 +55,19 @@ public static ParsedPubKey initPubKey(Reader publicKey) throws IOException {
5455
if (!keydata.isEmpty()) {
5556
String[] parts = keydata.trim().split("\\s+");
5657
if (parts.length >= 2) {
58+
byte[] decodedPublicKey = Base64Decoder.decode(parts[1]);
5759
return new ParsedPubKey(
5860
KeyType.fromString(parts[0]),
59-
new Buffer.PlainBuffer(Base64.getDecoder().decode(parts[1])).readPublicKey()
61+
new Buffer.PlainBuffer(decodedPublicKey).readPublicKey()
6062
);
6163
} else {
6264
throw new IOException("Got line with only one column");
6365
}
6466
}
6567
}
6668
throw new IOException("Public key file is blank");
69+
} catch (Base64DecodingException err) {
70+
throw new IOException("Public key decoding failed", err);
6771
} finally {
6872
br.close();
6973
}

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

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,8 @@
2323
import net.i2p.crypto.eddsa.EdDSAPrivateKey;
2424
import net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable;
2525
import net.i2p.crypto.eddsa.spec.EdDSAPrivateKeySpec;
26-
import net.schmizz.sshj.common.Buffer;
26+
import net.schmizz.sshj.common.*;
2727
import net.schmizz.sshj.common.Buffer.PlainBuffer;
28-
import net.schmizz.sshj.common.ByteArrayUtils;
29-
import net.schmizz.sshj.common.IOUtils;
30-
import net.schmizz.sshj.common.KeyType;
31-
import net.schmizz.sshj.common.SSHRuntimeException;
32-
import net.schmizz.sshj.common.SecurityUtils;
3328
import net.schmizz.sshj.transport.cipher.Cipher;
3429
import net.schmizz.sshj.userauth.keyprovider.BaseFileKeyProvider;
3530
import net.schmizz.sshj.userauth.keyprovider.FileKeyProvider;
@@ -55,7 +50,6 @@
5550
import java.security.spec.ECPrivateKeySpec;
5651
import java.security.spec.RSAPrivateCrtKeySpec;
5752
import java.util.Arrays;
58-
import java.util.Base64;
5953
import java.util.HashMap;
6054
import java.util.Map;
6155

@@ -124,7 +118,7 @@ protected KeyPair readKeyPair() throws IOException {
124118
try {
125119
if (checkHeader(reader)) {
126120
final String encodedPrivateKey = readEncodedKey(reader);
127-
byte[] decodedPrivateKey = Base64.getDecoder().decode(encodedPrivateKey);
121+
byte[] decodedPrivateKey = Base64Decoder.decode(encodedPrivateKey);
128122
final PlainBuffer bufferedPrivateKey = new PlainBuffer(decodedPrivateKey);
129123
return readDecodedKeyPair(bufferedPrivateKey);
130124
} else {
@@ -133,6 +127,8 @@ protected KeyPair readKeyPair() throws IOException {
133127
}
134128
} catch (final GeneralSecurityException e) {
135129
throw new SSHRuntimeException("Read OpenSSH Version 1 Key failed", e);
130+
} catch (Base64DecodingException e) {
131+
throw new SSHRuntimeException("Private Key decoding failed", e);
136132
} finally {
137133
IOUtils.closeQuietly(reader);
138134
}

‎src/main/java/net/schmizz/sshj/common/Base64Decoder.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/*
2+
* Copyright (C)2009 - SSHJ Contributors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package net.schmizz.sshj.common;
18+
19+
import java.io.IOException;
20+
import java.util.Base64;
21+
22+
/**
23+
* <p>Wraps {@link java.util.Base64.Decoder} in order to wrap unchecked {@code IllegalArgumentException} thrown by
24+
* the default Java Base64 decoder here and there.</p>
25+
*
26+
* <p>Please use this class instead of {@link java.util.Base64.Decoder}.</p>
27+
*/
28+
public class Base64Decoder {
29+
private Base64Decoder() {
30+
}
31+
32+
public static byte[] decode(byte[] source) throws Base64DecodingException {
33+
try {
34+
return Base64.getDecoder().decode(source);
35+
} catch (IllegalArgumentException err) {
36+
throw new Base64DecodingException(err);
37+
}
38+
}
39+
40+
public static byte[] decode(String src) throws Base64DecodingException {
41+
try {
42+
return Base64.getDecoder().decode(src);
43+
} catch (IllegalArgumentException err) {
44+
throw new Base64DecodingException(err);
45+
}
46+
}
47+
}

‎src/main/java/net/schmizz/sshj/common/Base64DecodingException.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/*
2+
* Copyright (C)2009 - SSHJ Contributors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package net.schmizz.sshj.common;
18+
19+
/**
20+
* A checked wrapper for all {@link IllegalArgumentException}, thrown by {@link java.util.Base64.Decoder}.
21+
*
22+
* @see Base64Decoder
23+
*/
24+
public class Base64DecodingException extends Exception {
25+
public Base64DecodingException(IllegalArgumentException cause) {
26+
super("Failed to decode base64: " + cause.getMessage(), cause);
27+
}
28+
}

‎src/main/java/net/schmizz/sshj/transport/verification/OpenSSHKnownHosts.java‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,7 @@
1818
import com.hierynomus.sshj.common.KeyAlgorithm;
1919
import com.hierynomus.sshj.transport.verification.KnownHostMatchers;
2020
import com.hierynomus.sshj.userauth.certificate.Certificate;
21-
import net.schmizz.sshj.common.Buffer;
22-
import net.schmizz.sshj.common.IOUtils;
23-
import net.schmizz.sshj.common.KeyType;
24-
import net.schmizz.sshj.common.LoggerFactory;
25-
import net.schmizz.sshj.common.SSHException;
26-
import net.schmizz.sshj.common.SSHRuntimeException;
27-
import net.schmizz.sshj.common.SecurityUtils;
21+
import net.schmizz.sshj.common.*;
2822
import org.slf4j.Logger;
2923

3024
import java.io.BufferedOutputStream;
@@ -290,9 +284,9 @@ public KnownHostEntry parseEntry(String line)
290284
if (type != KeyType.UNKNOWN) {
291285
final String sKey = split[i++];
292286
try {
293-
byte[] keyBytes = Base64.getDecoder().decode(sKey);
287+
byte[] keyBytes = Base64Decoder.decode(sKey);
294288
key = new Buffer.PlainBuffer(keyBytes).readPublicKey();
295-
} catch (IOException | IllegalArgumentException exception) {
289+
} catch (IOException | Base64DecodingException exception) {
296290
log.warn("Error decoding Base64 key bytes", exception);
297291
return new BadHostEntry(line);
298292
}

‎src/main/java/net/schmizz/sshj/userauth/keyprovider/PuTTYKeyFile.java‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,7 @@
2222
import net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable;
2323
import net.i2p.crypto.eddsa.spec.EdDSAPrivateKeySpec;
2424
import net.i2p.crypto.eddsa.spec.EdDSAPublicKeySpec;
25-
import net.schmizz.sshj.common.Buffer;
26-
import net.schmizz.sshj.common.KeyType;
27-
import net.schmizz.sshj.common.SecurityUtils;
25+
import net.schmizz.sshj.common.*;
2826
import net.schmizz.sshj.userauth.password.PasswordUtils;
2927
import org.bouncycastle.asn1.nist.NISTNamedCurves;
3028
import org.bouncycastle.asn1.x9.X9ECParameters;
@@ -42,7 +40,6 @@
4240
import java.security.*;
4341
import java.security.spec.*;
4442
import java.util.Arrays;
45-
import java.util.Base64;
4643
import java.util.HashMap;
4744
import java.util.Map;
4845

@@ -240,29 +237,34 @@ protected void parseKeyPair() throws IOException {
240237
if (this.keyFileVersion == null) {
241238
throw new IOException("Invalid key file format: missing \"PuTTY-User-Key-File-?\" entry");
242239
}
243-
// Retrieve keys from payload
244-
publicKey = Base64.getDecoder().decode(payload.get("Public-Lines"));
245-
if (this.isEncrypted()) {
246-
final char[] passphrase;
247-
if (pwdf != null) {
248-
passphrase = pwdf.reqPassword(resource);
249-
} else {
250-
passphrase = "".toCharArray();
251-
}
252-
try {
253-
privateKey = this.decrypt(Base64.getDecoder().decode(payload.get("Private-Lines")), passphrase);
254-
Mac mac;
255-
if (this.keyFileVersion <= 2) {
256-
mac = this.prepareVerifyMacV2(passphrase);
240+
try {
241+
// Retrieve keys from payload
242+
publicKey = Base64Decoder.decode(payload.get("Public-Lines"));
243+
if (this.isEncrypted()) {
244+
final char[] passphrase;
245+
if (pwdf != null) {
246+
passphrase = pwdf.reqPassword(resource);
257247
} else {
258-
mac = this.prepareVerifyMacV3();
248+
passphrase = "".toCharArray();
249+
}
250+
try {
251+
privateKey = this.decrypt(Base64Decoder.decode(payload.get("Private-Lines")), passphrase);
252+
Mac mac;
253+
if (this.keyFileVersion <= 2) {
254+
mac = this.prepareVerifyMacV2(passphrase);
255+
} else {
256+
mac = this.prepareVerifyMacV3();
257+
}
258+
this.verify(mac);
259+
} finally {
260+
PasswordUtils.blankOut(passphrase);
259261
}
260-
this.verify(mac);
261-
} finally {
262-
PasswordUtils.blankOut(passphrase);
262+
} else {
263+
privateKey = Base64Decoder.decode(payload.get("Private-Lines"));
263264
}
264-
} else {
265-
privateKey = Base64.getDecoder().decode(payload.get("Private-Lines"));
265+
}
266+
catch (Base64DecodingException e) {
267+
throw new IOException("PuTTY key decoding failed", e);
266268
}
267269
}
268270

‎src/test/java/com/hierynomus/sshj/transport/verification/OpenSSHKnownHostsTest.java‎

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,16 @@
1515
*/
1616
package com.hierynomus.sshj.transport.verification;
1717

18-
import static org.junit.jupiter.api.Assertions.assertEquals;
19-
import static org.junit.jupiter.api.Assertions.assertFalse;
20-
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
21-
import static org.junit.jupiter.api.Assertions.assertTrue;
22-
import static org.assertj.core.api.Assertions.*;
18+
import net.schmizz.sshj.common.Buffer;
19+
import net.schmizz.sshj.common.SecurityUtils;
20+
import net.schmizz.sshj.transport.verification.OpenSSHKnownHosts;
21+
import net.schmizz.sshj.util.KeyUtil;
22+
import org.junit.jupiter.api.BeforeAll;
23+
import org.junit.jupiter.api.Test;
24+
import org.junit.jupiter.api.io.TempDir;
25+
import org.junit.jupiter.params.ParameterizedTest;
26+
import org.junit.jupiter.params.provider.Arguments;
27+
import org.junit.jupiter.params.provider.MethodSource;
2328

2429
import java.io.File;
2530
import java.io.IOException;
@@ -29,17 +34,8 @@
2934
import java.util.Base64;
3035
import java.util.stream.Stream;
3136

32-
import org.junit.jupiter.api.BeforeAll;
33-
import org.junit.jupiter.api.Test;
34-
import org.junit.jupiter.api.io.TempDir;
35-
import org.junit.jupiter.params.ParameterizedTest;
36-
import org.junit.jupiter.params.provider.Arguments;
37-
import org.junit.jupiter.params.provider.MethodSource;
38-
39-
import net.schmizz.sshj.common.Buffer;
40-
import net.schmizz.sshj.common.SecurityUtils;
41-
import net.schmizz.sshj.transport.verification.OpenSSHKnownHosts;
42-
import net.schmizz.sshj.util.KeyUtil;
37+
import static org.assertj.core.api.Assertions.assertThat;
38+
import static org.junit.jupiter.api.Assertions.*;
4339

4440
public class OpenSSHKnownHostsTest {
4541
@TempDir
@@ -118,6 +114,24 @@ public void shouldNotFailOnMalformedBase64String() throws IOException {
118114
assertThat(ohk.entries().get(0)).isInstanceOf(OpenSSHKnownHosts.BadHostEntry.class);
119115
}
120116

117+
@Test
118+
public void shouldNotFailOnMalformeSaltBase64String() throws IOException {
119+
// A record with broken base64 inside the salt part of the hash.
120+
// No matter how it could be generated, such broken strings must not cause unexpected errors.
121+
String hostName = "example.com";
122+
File knownHosts = knownHosts(
123+
"|1|2gujgGa6gJnK7wGPCX8zuGttvCMXX|Oqkbjtxd9RFxKQv6y3l3GIxLNiU= ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBGVVnyoAD5/uWiiuTSM3RuW8dEWRrqOXYobAMKHhAA6kuOBoPK+LoAYyUcN26bdMiCxg+VOaLHxPNWv5SlhbMWw=\n"
124+
);
125+
OpenSSHKnownHosts ohk = new OpenSSHKnownHosts(knownHosts);
126+
assertEquals(1, ohk.entries().size());
127+
128+
// Some random valid public key. It doesn't matter for the test if it matches the broken host key record or not.
129+
PublicKey k = new Buffer.PlainBuffer(Base64.getDecoder().decode(
130+
"AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBLTjA7hduYGmvV9smEEsIdGLdghSPD7kL8QarIIOkeXmBh+LTtT/T1K+Ot/rmXCZsP8hoUXxbvN+Tks440Ci0ck="))
131+
.readPublicKey();
132+
assertFalse(ohk.verify(hostName, 22, k));
133+
}
134+
121135
@Test
122136
public void shouldMarkBadLineAndNotFail() throws Exception {
123137
File knownHosts = knownHosts(

0 commit comments

Comments
 (0)