Mina's AbstractSessionHelper and BaseCipher are ignoring lengths returned from cipher update and deriving information from the input len rather than what has actually been processed during the update #455
Comments
|
Thank you for the detailed report. What cipher was used in your case? (Debug logging should show the key exchange proposals and the negotiation result, which should tell the chosen cipher algorithm.) I'm a little bit confused by the |
|
It was AES based. The BC LTS release supports AES pipelining on the Intel architecture which is why paying attention to what is coming back from update is so important (I mean, it was already important, but here it's really important). Yes, there should be a call to Cipher.doFinal() somewhere (on this one we couldn't find one either, but then we didn't spend a lot of time looking, it probably needs someone more familiar with the code). BaseCipher, in terms of update, is clearly wrong by inspection. From the point of view of how Java providers work, it is almost a miracle it ever worked, even with BC. |
That miracle appears to happen frequently. All the other Java SSH implementation that I checked basically do it that way (calling only update()). Not all of them are doing in-place processing, though. For CTR and CBC, this could be fixed by calling doFinal() and then re-initializing the cipher with the same key:
If GCM, then something must be wrong with buffer size calculations. For AES-GCM, current code already calls doFinal() and then re-inits the cipher. With CTR or CBC, the only way I can imagine this Off-topic: we should also improve the exception handling for such cases and make sure we include the exact algorithm/transformation name into the exception message. |
|
@tomaswolf apologies if I offended, I was not trying to get a raise out of anyone but the use of the JCE Cipher class is wrong by inspection. If the API is modified to conform to the Cipher class as documented in the JCE spec you should find that the code will happily support both hardware and software providers that are compliant with the JCE spec. And yes, the issue is that the calling code is not recognising that the cipher class is buffering input and delaying, which it is allowed to do, and which is why the return values from Cipher.update() and Cipher.doFinal() must be used. This means that the calling code thinks it is getting back data but is instead leaving the the array filled with zeros. The "miracle" may also be that both sides in a lot of cases are just decrypting to strings of zeros, I'm not sure that's the effect that's being looked for. |
|
No problem at all. I immediately came to the same conclusion as you did when I looked at BaseCipher, which is why I had written I was "confused". I do agree this needs to call Cipher.doFinal(), and I'm confused that it worked without so far, not just in Apache MINA SSHD but apparently also in a number of other Java SSH implementations. An API change in BaseCipher is not needed. Cipher usage in SSH is fairly simple; maybe BaseCipher.update() would have better been called processBlocks() or some such to avoid confusion with the Java Cipher interface. Data passed to the cipher in SSH is always a multiple of the block size, all ciphers are /NOPADDING. Encryption always encrypts a whole message. Decryption may in some modes first decrypt the first block to get the message length, then in a second call decrypt the rest of the message. The cipher is kept for the whole session until the next key exchange, which is why after a doFinal() needed to force-flush that internal buffer the re-initializion will need to use the correct IV. It'd still be useful to know which cipher exactly was used in the case originally reported. |
|
The following in BaseCipher: needs to be: The problem is the calling class is assuming it knows how much output it's getting back, something it cannot know unless it can see the return value from cipher.update(). I'd be very suspicous of the second use of inputOffset as well - while it's okay to process in place (so pass the variable input in twice), inputOffset is unlikely to represent the correct offset that any output from the cipher.update() will be written to, the output offset needs to be passed in as well (after the call above, inputOffset can be safely incremented by inputLen, outputOffset would be incremented by the return value from BaseCipher.update()). The same usage constraints are required for doFinal(). |
|
I was more thinking along the lines (for CTR and CBC; GCM already is OK) That simply guarantees that update always processes all |
|
Okay, it's not strictly correct, but assuming the protocol is never using a padded mode when it comes down this path the above should be okay (I think) - the assert should catch the use of padding if it's the case anyway (the return value of doFinal() becomes significant if padding is being added (as it is on encryption) or removed (as can happen on decryption)). It might be worth adding a comment above it to explain what it will mean if the assert fails To save someone later should the status quo change. I assume it's trying to avoid re-intializing the cipher where it's not really required? |
|
Yes. SSH uses only ciphers with no padding. SSH pads itself. It's not a general solution, but it is a perfectly valid and correct solution for the cipher uses in SSH. |
The org.apache.sshd.common.cipher.Cipher interface specifies for update(byte[] buffer, int offset, int length) that length bytes are encrypted or decrypted in-place in the given buffer, starting at the given offset. The BaseCipher implementation just called javax.crypto.Cipher.update(). That, however, may buffer blocks and not update all data right away. (For instance, AES pipelined implementations may behave that way.) Buffered blocks may be returned/updated in subsequent update() calls. To ensure that really all bytes given are updated, one needs to call doFinal(), which always returns/updates such buffered blocks. But javax.crypto.Cipher.doFinal() resets the cipher to its initial state. For use in SSH, this is not appropriate: the cipher must be reset not to the initial state but to the final state. This is done for CTR ciphers by adding the number of processed blocks to the initial IV and then using that IV for re-initialization. For CBC ciphers, the re-initialization IV must be the last encrypted block processed. Note that in CTR mode, we cannot check for IV re-use. This is not a problem in practice because in the SSH protocol key exchanges happen long before an IV can wrap around.
The org.apache.sshd.common.cipher.Cipher interface specifies for update(byte[] buffer, int offset, int length) that length bytes are encrypted or decrypted in-place in the given buffer, starting at the given offset. The BaseCipher implementation just called javax.crypto.Cipher.update(). That, however, may buffer blocks and not update all data right away. (For instance, AES pipelined implementations may behave that way.) Buffered blocks may be returned/updated in subsequent update() calls. To ensure that really all bytes given are updated, one needs to call doFinal(), which always returns/updates such buffered blocks. But javax.crypto.Cipher.doFinal() resets the cipher to its initial state. For use in SSH, this is not appropriate: the cipher must be reset not to the initial state but to the final state. This is done for CTR ciphers by adding the number of processed blocks to the initial IV and then using that IV for re-initialization. For CBC ciphers, the re-initialization IV must be the last encrypted block processed. Note that in CTR mode, we cannot check for IV re-use. This is not a problem in practice because in the SSH protocol key exchanges happen long before an IV can wrap around.
The org.apache.sshd.common.cipher.Cipher interface specifies for update(byte[] buffer, int offset, int length) that length bytes are encrypted or decrypted in-place in the given buffer, starting at the given offset. The BaseCipher implementation just called javax.crypto.Cipher.update(). That, however, may buffer blocks and not update all data right away. (For instance, AES pipelined implementations may behave that way.) Buffered blocks may be returned/updated in subsequent update() calls. To ensure that really all bytes given are updated, one needs to call doFinal(), which always returns/updates such buffered blocks. But javax.crypto.Cipher.doFinal() resets the cipher to its initial state. For use in SSH, this is not appropriate: the cipher must be reset not to the initial state but to the final state. This is done for CTR ciphers by adding the number of processed blocks to the initial IV and then using that IV for re-initialization. For CBC ciphers, the re-initialization IV must be the last encrypted block processed. Note that in CTR mode, we cannot check for IV re-use. This is not a problem in practice because in the SSH protocol key exchanges happen long before an IV can wrap around.
|
Hello @tomaswolf |
|
@adelel1 : could you please try your application with the 2.13.0-SNAPSHOT versions available in the Apache Snapshots Repository? It would be good to have some confirmation that the code changes made for this issue do indeed resolve the problem. |
|
Hi @tomaswolf |
Version
2.7.0
Bug description
I found that Apache mina 2.7.0+ bouncycastle 2.73.3 + java8 + linux brings javax.crypto.ShortBufferException, so originally I created my question as the bouncycastle issue bcgit/bc-java#1562.
Bouncycastle DevTeam replied that this is a bug in Apache mina 2.7.0 and gave the detailed explanation, so I'm here.
I quote the detailed explanation below:
Title: Mina's AbstractSessionHelper and BaseCipher are ignoring lengths returned from cipher update and deriving information from the input len rather than what has actually been processed during the update.
Issue:
The issue is the return values from the cipher.update and cipher.doFinal are meaningful and cannot be ignored,
particularly in the case, such as with the LTS native layer, where underlying provider maybe buffering output
for speed and efficiency reasons.
It is pure coincidence that the code works with the regular BC provider. It is simply because it does not buffer,
except in very rare cases, that the error has not shown up.
Please raise this issue with the Apache project so they can fix it, we are more than happy to talk to them
if they need any further assistance.
References:
In AbstractSessionHelper:
org.apache.sshd.common.session.helpers.AbstractSession#encryptOutgoingBuffer
v2.7.0: Line 1324
HEAD: Line 1547
Link:
mina-sshd/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
Line 1547 in b040109
blockCount is derived from input rather than what was actually processed because outCipher.update( ... ) returns void.
In org.apache.sshd.common.cipher.BaseCipher#update
v2.7.0: Line 123
HEAD: Line 121
Link:
mina-sshd/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCipher.java
Line 121 in b040109
Ignores the return value from update and returns void.
Actual behavior
I get javax.crypto.ShortBufferException when use Apache mina 2.7.0+ bouncycastle 2.73.3 + java8 + linux + ED25519 ssh key.
Expected behavior
No javax.crypto.ShortBufferException.
Relevant log output
[INFO] | +- org.eclipse.jgit:org.eclipse.jgit.http.server:jar:5.13.1.202206130422-r:compile
[INFO] | +- org.eclipse.jgit:org.eclipse.jgit.ssh.apache:jar:5.13.1.202206130422-r:compile
[INFO] | | +- org.apache.sshd:sshd-osgi:jar:2.7.0:compile
[INFO] | | +- org.apache.sshd:sshd-sftp:jar:2.7.0:compile
[INFO] | | | - org.apache.sshd:sshd-core:jar:2.7.0:compile
[INFO] | | | - org.apache.sshd:sshd-common:jar:2.7.0:compile
[INFO] | +- org.bouncycastle:bcpkix-lts8on:jar:2.73.3:provided
[INFO] | | - org.bouncycastle:bcutil-lts8on:jar:2.73.3:provided (version selected from constraint [2.73.3,2.74.0))
[INFO] | | - org.bouncycastle:bcprov-lts8on:jar:2.73.3:provided (version selected from constraint [2.73.3,2.74.0))
The text was updated successfully, but these errors were encountered: