Skip to content

Commit

Permalink
GH-434: skip unknown public keys from external sources
Browse files Browse the repository at this point in the history
Change Buffer.getPublicKey() to set the read position to after the key
even if reading the key fails. This enables us to continue reading keys
from a list of keys in a buffer even when a particular key cannot be
decoded.

Change the two places where we receive lists of public keys from
external sources: from an SSH agent or via the "hostkeys-00@openssh.com"
extension. Skip and log keys that cannot be decoded.
  • Loading branch information
tomaswolf committed Jan 7, 2024
1 parent e5c48cd commit e37bcc6
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -537,11 +537,14 @@ public PublicKey getPublicKey() throws SshException {
public PublicKey getPublicKey(BufferPublicKeyParser<? extends PublicKey> parser) throws SshException {
int ow = wpos();
int len = ensureAvailable(getInt());
wpos(rpos() + len);
int afterKey = rpos() + len;
wpos(afterKey);
try {
return getRawPublicKey(parser);
} finally {
wpos(ow);
// Skip this key, even if the parser failed.
rpos(afterKey);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,25 @@ public Iterable<? extends Map.Entry<PublicKey, String>> getIdentities() throws I
List<SimpleImmutableEntry<PublicKey, String>> keys = new ArrayList<>(nbIdentities);
boolean debugEnabled = log.isDebugEnabled();
for (int i = 0; i < nbIdentities; i++) {
PublicKey key = buffer.getPublicKey();
PublicKey key = null;
String error = null;
try {
key = buffer.getPublicKey();
} catch (SshException e) {
error = e.getLocalizedMessage();
}
String comment = buffer.getString();
if (debugEnabled) {
log.debug("getIdentities() key type={}, comment={}, fingerprint={}",
KeyUtils.getKeyType(key), comment, KeyUtils.getFingerPrint(key));
if (key == null) {
if (log.isTraceEnabled()) {
log.trace("getIdentities() ignoring an unknown key type; comment={}; error={}", comment, error);
}
} else {
if (debugEnabled) {
log.debug("getIdentities() key type={}, comment={}, fingerprint={}", KeyUtils.getKeyType(key), comment,
KeyUtils.getFingerPrint(key));
}
keys.add(new SimpleImmutableEntry<>(key, comment));
}
keys.add(new SimpleImmutableEntry<>(key, comment));
}

return keys;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,12 @@ public class OpenSshHostKeysHandler extends AbstractOpenSshHostKeysHandler {

public OpenSshHostKeysHandler() {
super(REQUEST);
setIgnoreInvalidKeys(true);
}

public OpenSshHostKeysHandler(BufferPublicKeyParser<? extends PublicKey> parser) {
super(REQUEST, parser);
setIgnoreInvalidKeys(true);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.LinkedList;
import java.util.Objects;

import org.apache.sshd.common.SshException;
import org.apache.sshd.common.config.keys.KeyUtils;
import org.apache.sshd.common.session.ConnectionService;
import org.apache.sshd.common.session.Session;
Expand All @@ -39,6 +40,8 @@ public abstract class AbstractOpenSshHostKeysHandler extends AbstractConnectionS
private final String request;
private final BufferPublicKeyParser<? extends PublicKey> parser;

private boolean ignoreInvalidKeys;

protected AbstractOpenSshHostKeysHandler(String request) {
this(request, BufferPublicKeyParser.DEFAULT);
}
Expand Down Expand Up @@ -70,12 +73,22 @@ public Result process(
if (p != null) {
boolean debugEnabled = log.isDebugEnabled();
while (buffer.available() > 0) {
PublicKey key = buffer.getPublicKey(p);
if (debugEnabled) {
log.debug("process({})[{}] key type={}, fingerprint={}",
connectionService, request, KeyUtils.getKeyType(key), KeyUtils.getFingerPrint(key));
PublicKey key = null;
try {
key = buffer.getPublicKey(p);
} catch (SshException e) {
if (!isIgnoreInvalidKeys()) {
throw e;
}
if (log.isTraceEnabled()) {
log.trace("process({})[{}] received an unsupported key", connectionService, request, e);
}
}
if (key != null) {
if (debugEnabled) {
log.debug("process({})[{}] key type={}, fingerprint={}", connectionService, request,
KeyUtils.getKeyType(key), KeyUtils.getFingerPrint(key));
}
keys.add(key);
}
}
Expand All @@ -84,6 +97,14 @@ public Result process(
return handleHostKeys(connectionService.getSession(), keys, wantReply, buffer);
}

protected void setIgnoreInvalidKeys(boolean ignore) {
ignoreInvalidKeys = ignore;
}

protected boolean isIgnoreInvalidKeys() {
return ignoreInvalidKeys;
}

protected abstract Result handleHostKeys(
Session session, Collection<? extends PublicKey> keys, boolean wantReply, Buffer buffer)
throws Exception;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.sshd.common.global;

import java.security.GeneralSecurityException;
import java.security.KeyPairGenerator;
import java.security.PublicKey;
import java.util.Collection;

import org.apache.sshd.common.SshException;
import org.apache.sshd.common.config.keys.KeyUtils;
import org.apache.sshd.common.session.ConnectionService;
import org.apache.sshd.common.session.Session;
import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
import org.apache.sshd.util.test.BaseTestSupport;
import org.apache.sshd.util.test.NoIoTestCase;
import org.junit.Before;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

/**
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
@Category(NoIoTestCase.class)
@RunWith(MockitoJUnitRunner.class)
public class OpenSshHostKeysHandlerTest extends BaseTestSupport {

@Mock
private ConnectionService connectionService;

private PublicKey key;
private Buffer buffer;

public OpenSshHostKeysHandlerTest() {
super();
}

@Before
public void prepareBuffer() throws Exception {
// Create an RSA key
key = KeyPairGenerator.getInstance("RSA").generateKeyPair().getPublic();
// Serialize it twice to a buffer, but insert a fake item in between
buffer = new ByteArrayBuffer();
buffer.putPublicKey(key);
buffer.putUInt(34);
buffer.putString("unknown"); // Fake key type; 7 + 4 bytes length
buffer.putString("followed by garbage"); // 19 + 4
buffer.putPublicKey(key);
}

@Test
public void clientIgnoresUnknownKeys() throws Exception {
boolean[] handlerCalled = { false };
org.apache.sshd.client.global.OpenSshHostKeysHandler handler
= new org.apache.sshd.client.global.OpenSshHostKeysHandler() {
@Override
protected Result handleHostKeys(
Session session, Collection<? extends PublicKey> keys, boolean wantReply,
Buffer buffer) throws Exception {
handlerCalled[0] = true;
assertEquals("Unexpected number of keys", 2, keys.size());
for (PublicKey k : keys) {
assertTrue("Unexpected public key", KeyUtils.compareKeys(key, k));
}
return Result.Replied;
}
};
handler.process(connectionService, org.apache.sshd.client.global.OpenSshHostKeysHandler.REQUEST, false, buffer);
assertTrue("Handler should have been called", handlerCalled[0]);
}

@Test
public void serverThrowsOnUnknownKeys() throws Exception {
boolean[] handlerCalled = { false };
org.apache.sshd.server.global.OpenSshHostKeysHandler handler
= new org.apache.sshd.server.global.OpenSshHostKeysHandler() {
@Override
protected Result handleHostKeys(
Session session, Collection<? extends PublicKey> keys, boolean wantReply,
Buffer buffer) throws Exception {
handlerCalled[0] = true;
return Result.Replied;
}
};
SshException e = assertThrows(SshException.class, () -> handler.process(connectionService,
org.apache.sshd.server.global.OpenSshHostKeysHandler.REQUEST, false, buffer));
assertFalse("Handler should not have been called", handlerCalled[0]);
assertTrue("Expected exception cause", e.getCause() instanceof GeneralSecurityException);
}
}

0 comments on commit e37bcc6

Please sign in to comment.