(taken from jsch-users): Possible deadlock in KnownHosts

[kimmerin]

Hi,

I've remembered that there was an issue reported on jsch-users where I've answered with a patch. I don't have the original mail in my archive on my current computer but the internet doesn't forget.

I've checked the current source and it seems that a part of my fix got included into the source base but not all, especially the removal of the synchronization that lead to the deadlock in the mentioned report above.

Also KnownHost contains a lot of System.out and System.err.println calls in case of errors that should be changed to logger.log-calls in my eyes.

[norrisjeremy]

I'm not sure I see how this deadlock could still be occurring, at least as described in the original JSch-users thread: the KnownHosts class no longer contains any synchronized methods.

Are you still able to reproduce this problem with current release?

[kimmerin]

the KnownHosts class no longer contains any synchronized methods.

Um...

jsch/src/main/java/com/jcraft/jsch/KnownHosts.java

Lines 551 to 558 in 7e05876

	synchronized(macsha1){
	macsha1.init(salt);
	byte[] foo=Util.str2byte(_host);
	macsha1.update(foo, 0, foo.length);
	byte[] bar=new byte[macsha1.getBlockSize()];
	macsha1.doFinal(bar, 0);
	return Util.array_equals(hash, bar);
	}

jsch/src/main/java/com/jcraft/jsch/KnownHosts.java

Lines 582 to 588 in 7e05876

	synchronized(macsha1){
	macsha1.init(salt);
	byte[] foo=Util.str2byte(host);
	macsha1.update(foo, 0, foo.length);
	hash=new byte[macsha1.getBlockSize()];
	macsha1.doFinal(hash, 0);
	}

That's what I see in trunk right at this moment.

Are you still able to reproduce this problem with current release?

The problem was reported by somebody else and I just provided a solution by looking very closely at the deadlock-message in that mail and its stacktraces. We've had occasional deadlocks as well "out there" and after this fix never heard of one again (but I might remember wrong and this was fixed by me at some other place back then).

As mentioned before in another issue I'm currently doing a lot of customization of the original sources when a new release is available (a lot of effort I try to reduce by contributing the useful stuff to this project - the logging issue being one of them). So if I do testing with SSH I always use a version without above synchronized blocks.

Above synchronized blocks are completely unnecessary. hash() get's only called during instantiation so that you don't have the situation that hash() and isMatched get called in parallel. The blocks are a relic of the time when there was a VM-wide hash-instance.

[norrisjeremy]

Neither of those are synchronizing on the KnownHosts object, they are synchronizing on the HmacSHA1 object.

The original deadlock as you linked from JSch-Users archive is caused by a deadlock between KnownHost object and KnownHosts.pool object, that was caused because:

Thread 1: locks the KnownHosts object (hkr) in Session.checkHost(), then attempts to lock the pool object whilst calling KnownHosts.check().

Thread 2: locks the pool object in KnownHosts.getHostKey(), then attempts to to lock the KnownHosts object because KnownHosts.getHMACSHA1() used to be a synchronized method.

So you end up with Thread 1 & Thread 2 waiting on for locks owned by each other.

However, current version of KnownHosts in this fork, no longer has KnownHosts.getHMACSHA1() as a synchronized method.

Therefore, if you look at how the deadlock originally occurred in that old release of JSch, you will see it can no longer happen, because the call by Thread 2 to KnownHosts.getHMACSHA1() will no longer attempt to lock the KnownHosts object:

Thread 1: locks the KnownHosts object (hkr) in Session.checkHost(), then attempts to lock the pool object whilst calling KnownHosts.check().

Thread 2: locks the pool object in KnownHosts.getHostKey(), then completes its call to KnownHosts.getHMACSHA1() successfully and will eventually drop the lock on the pool object when KnownHosts.getHostKey() returns to the caller, at which time Thread 1 will gain the lock on the pool object and complete it's call to KnownHosts.check().

[norrisjeremy]

Btw, I'm pretty sure this was noted as being fixed in the old 0.1.54 release as documented here:

jsch/ChangeLog

Line 31 in 7e05876

- bugfix: fixed a deadlock bug in KnownHosts#getHostKey().

[norrisjeremy]

Btw, I do agree that cleaning up the calls to System.out.println() & System.err.println() in the KnownHosts class is an excellent idea.

Also, it probably would be an improvement to stop calling getHMACSHA1() from the isMatched() & hash() methods, and simply use the hmacsha1 property of the object itself.

In fact, you can see that it's even possible to cause NPE's, because getHMACSHA1() can return null, but the calls to it in isMatched() & hash() both directly use the return value without confirming it isn't null first.

[kimmerin]

As said, Here[TM] I'm testing with a customized version of JSch that contain the complete patch as shared on the mail list back then but I assume that you're right with your analysis that having an hash-instance per HashedHostKey-instance is sufficient to prevent occurences.

I'll take your other answer as an invitation for implementation ;-) so I'll have a look into it.

[kimmerin]

@norrisjeremy I think I've found a bug unrelated by this issue but that came up while I've put the class under test. In the instantiation of HashedHostkey there is a check in order to decide if the hostname is a hashed host text or not:

jsch/src/main/java/com/jcraft/jsch/KnownHosts.java

Lines 534 to 539 in 7e05876

	if(salt.length!=20 || // block size of hmac-sha1
	hash.length!=20){
	salt=null;
	hash=null;
	return;
	}

This worked well at a time where SHA-1 was the hashing algorithm. Now the hashing algorithm can be configured, so the length of the hash-text can vary. When actually testing this, the testcase fails (as expected):

kh.hmacsha1 = new HMACSHA256();
hhk = kh.new HashedHostKey("@somemarker", "host.example.com", HostKey.ED448, dsaKeyBytes, "some commänt");
[...]
hhk.hash();
assertEquals("|1|AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=|mie6rcAf1aPGk6d+HxnkpvO4HaOAH/Y6YWegs+Xog/s=", hhk.getHost(), "host mismatch");
assertTrue(hhk.isHashed(), "key should report itself hashed");

hhk = kh.new HashedHostKey(hhk.getHost(), HostKey.ED448, dsaKeyBytes);
assertEquals("|1|AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=|mie6rcAf1aPGk6d+HxnkpvO4HaOAH/Y6YWegs+Xog/s=", hhk.getHost(), "host mismatch");
assertTrue(hhk.isHashed(), "key should report itself hashed");

org.opentest4j.AssertionFailedError: key should report itself hashed ==> expected: but was:
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
at com.jcraft.jsch.KnownHostsTest.testHashedKeyCreation(KnownHostsTest.java:535)

I can do the fix and change the subject of this issue accordingly so that the PR stays "on topic"

[norrisjeremy]

I'm confused, why are you using HMACSHA256 for host key hashing?

[kimmerin]

JSch.setConfig("hmac-sha1") allows you to set a different HMAC-class than the default one (HMACSHA1). That's what I'm testing here and instead of calling JSch.setConfig("hmac-sha1", HMACSHA256.class.getName()) I'm setting an instance of that class directly.

That way the effect can be shown. If it's forbidden to use a non-SHA-1-hashing algorithm, setting one via JSch.setConfig should lead to an IllegalArgumentException (doesn't happen, that's another test I've already written) or hashing algorithms with different blocksizes than 20 should be supported at the code location I've quoted.

[norrisjeremy]

I think you've misunderstood the purpose of JSch.setConfig("hmac-sha1", ...).
It's not to allow a user to pick a different HMAC algorithm, like HMAC-SHA256: it's to let a user direct JSch to use a customized implementation of HMAC-SHA1, like with a different crypto provider.

There's no need to make any changes here: if a user incorrectly execute JSch.setConfig("hmac-sha1", HMACSHA256.class.getName()), lots of things will break, not just this.

[kimmerin]

Misunderstanding is the wrong word. I've looked at the source, checked what's possible and wrote tests around that.

Edit: I assumed that non-SHA-256-hashes are supported because hash and isMatched both take hash.getBlockSize into account rather than using 20 as a fixed value.

As I said: If the use of non-SHA-1-hashes shouldn't be allowed, trying to set one should lead to an exception. Otherwise - looking at KnownHosts - you will corrupt your known-hosts-file e.g. as soon as you do a remove-call, that leads to a sync where the wrong hashes are written to the file.

[norrisjeremy]

Again, there is no need to do anything here: if you incorrectly execute JSch.setConfig("hmac-sha1", ...) to point at a MAC implementation that is not actually implementing HMAC-SHA1, you've broken alot more than the known_hosts file.
For example, MAC negotiation of hmac-sha1 would utterly break and your SSH session would not function.

Also, I'm not really sure how you would propose to to implement any sort of runtime detection of what a user configures JSch.getConfig("hmac-sha1') as JSch uses reflection to instantiate the class and it can be pointed at anything.

I.e. it's impossible to check that when a user executes JSch.setConfig("hmac-sha1", "my.own.HMACSHA1"), to determine that my.own.HMACSHA1 is a com.jcraft.jsch.MAC implementation that conforms to HMAC-SHA1.

[kimmerin]

I.e. it's impossible to check that when a user executes JSch.setConfig("hmac-sha1", "my.own.HMACSHA1"), to determine that my.own.HMACSHA1 is a com.jcraft.jsch.MAC implementation that conforms to HMAC-SHA1.

Why shouldn't that be possible? HMACSHA1 is a deterministic function, so given the same input you get the same result. If it differs you can complain.

[kimmerin]

a MAC implementation that is not actually implementing HMAC-SHA1, you've broken alot more than the known_hosts file.
For example, MAC negotiation of hmac-sha1 would utterly break and your SSH session would not function.

Personally I see a corrupt hosts-file much more serious than a handshake-failure. After fixing the MAC-configuration the latter starts working again while the former will still leave you with a corrupt hosts-file.

Edit: can you show me where JSch.getConfig("hmac-sha1") is used other than in KnownHosts? I've only found it to be used in KnownHosts and KnownHosts.getHMACSHA1 is only called in the context of KnownHosts as well. So how can setting hmac-sha1 to HMACSHA256 have any effect on the MAC negotiation you've mentioned? I've searched for the text occurrence of getConfig("hmac-sha1") to also get Session.getConfig-calls.

[norrisjeremy]

a MAC implementation that is not actually implementing HMAC-SHA1, you've broken alot more than the known_hosts file.
For example, MAC negotiation of hmac-sha1 would utterly break and your SSH session would not function.

Personally I see a corrupt hosts-file much more serious than a handshake-failure. After fixing the MAC-configuration the latter starts working again while the former will still leave you with a corrupt hosts-file.

Edit: can you show me where JSch.getConfig("hmac-sha1") is used other than in KnownHosts? I've only found it to be used in KnownHosts and KnownHosts.getHMACSHA1 is only called in the context of KnownHosts as well. So how can setting hmac-sha1 to HMACSHA256 have any effect on the MAC negotiation you've mentioned? I've searched for the text occurrence of getConfig("hmac-sha1") to also get Session.getConfig-calls.

https://github.com/mwiede/jsch/blob/master/src/main/java/com/jcraft/jsch/Session.java#L1497
https://github.com/mwiede/jsch/blob/master/src/main/java/com/jcraft/jsch/Session.java#L1526
https://github.com/mwiede/jsch/blob/master/src/main/java/com/jcraft/jsch/Session.java#L2905

[kimmerin]

https://github.com/mwiede/jsch/blob/master/src/main/java/com/jcraft/jsch/Session.java#L1497

That explains why I haven't found anything ;-)

I'll remove the test with HMACSHA256 then but I still like to add a "is-this-SHA-1" check to prevent the corrupt-known-hosts-file-situation from happening. Is that OK?

[norrisjeremy]

https://github.com/mwiede/jsch/blob/master/src/main/java/com/jcraft/jsch/Session.java#L1497

That explains why I haven't found anything ;-)

I'll remove the test with HMACSHA256 then but I still like to add a "is-this-SHA-1" check to prevent the corrupt-known-hosts-file-situation from happening. Is that OK?

I would rather not, as it would simply degrade performance. If a user incorrectly sets hmac-sha1 to point at something that isn't implementing HMAC-SHA1, then too bad.

[kimmerin]

I would rather not, as it would simply degrade performance.

Loading the known-hosts-file isn't part of a performance-critical operation. The read-operation of the file should take magnitudes longer than a single hash-operation with given input. I'm not talking about all occurrences where the SHA-1-class is used (e.g. during MAC negotiations).

[norrisjeremy]

I would rather not, as it would simply degrade performance.

Loading the known-hosts-file isn't part of a performance-critical operation. The read-operation of the file should take magnitudes longer than a single hash-operation with given input. I'm not talking about all occurrences where the SHA-1-class is used (e.g. during MAC negotiations).

I strongly disagree with adding any sort of runtime testing of MAC operations. We simply need to trust that users know what they are doing and if they do the wrong thing, well things can get corrupted.