Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bug with AEAD ciphers when compression is used. #15

Merged
merged 12 commits into from
Oct 15, 2020
Merged

Fix bug with AEAD ciphers when compression is used. #15

merged 12 commits into from
Oct 15, 2020

Conversation

norrisjeremy
Copy link
Contributor

The inflater code expects the end of buffer to not include the AEAD tag.

I updated the integration tests to always test every cipher & mac both with and without compression enabled to hopefully catch issues like this in the future.

@norrisjeremy
Copy link
Contributor Author

I'm not sure what caused this failure with just ea4283a applied earlier:

Error:  Tests run: 76, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 116.501 s <<< FAILURE! - in com.jcraft.jsch.AlgorithmsIT
Error:  testCiphers{String, String}[7]  Time elapsed: 1.061 s  <<< ERROR!
com.jcraft.jsch.JSchException: Session.connect: java.io.IOException: End of IO Stream Read
	at com.jcraft.jsch.AlgorithmsIT.doSftp(AlgorithmsIT.java:391)
	at com.jcraft.jsch.AlgorithmsIT.testCiphers(AlgorithmsIT.java:244)

It seems to have gone away after I pushed 1393e81 to try and get more details...

@mwiede
Copy link
Owner

mwiede commented Oct 13, 2020

I also had test failures with my latest changes. Some flakiness, because when re-running the maven job without any code changes, it went through. Now it failed again after re-running. We need to investigate.

@norrisjeremy
Copy link
Contributor Author

Yes, I've reproduced the intermittent failures locally and I'm attempting to track them down.
I believe that there is an issue in curve25519-sha2 that may be the cause.

@mwiede
Copy link
Owner

mwiede commented Oct 13, 2020 via email •
edited
Loading

@norrisjeremy
Add signature verify logging for ECDSA and include the specific RSA a…
4ba6c43 
…lgorithm in verify logging.
@norrisjeremy
Increase logging verbosity to help troubleshoot failures.
a151ebd
@norrisjeremy
Print logging info during failures to help troubleshoot failures.
e9df50b
@norrisjeremy
Add version for maven-javadoc-plugin to eliminate build warning.
02183db
@norrisjeremy
Use SSH_MSG_KEX_ECDH_REPLY constant instead of raw value.
c76d43c
@norrisjeremy
Attempt to fix intermittent issues with curve25519-sha2.
2d9681c 
The Q_C value was not properly zero-extended to the full 32-byte length
before being inserted into the SSH_MSG_KEX_ECDH_INIT packet, as well as
for use in the hash H computation.

Additionally, stop trying to clear sign bit of Q values: this appears to
be unnecessary (or is handled by the JCE X25519 implementation).

Finally, validate the length of Q_S sent by the server in order to
comply with section 3 of RFC 8731.
@norrisjeremy
Copy link
Contributor Author

Ok, I apologize for the overall size of this PR now: but I think I've finally tracked down the source of the intermittent test failures.

It appears that the curve25519-sha2 code I originally submitted earlier this year wasn't correctly handling encoding of the Q_C values.

I've run the tests in a continuous loop locally for a couple hours this afternoon and didn't see any failures.
I'm also continuing to run them in a non-stop loop for another few hours this evening to see how well things turn out.

@norrisjeremy
Copy link
Contributor Author

Ok, I let the tests run in a loop for almost 3 hours locally ( mvn verify -DskipITs=false 100 times).
There were only 4 failures, none of which appeared to be related to any crypto/SSH protocol issues:

  • Two instances of org.testcontainers.containers.ContainerLaunchException: Container startup failed.
  • One instance of com.jcraft.jsch.JSchException: channel is not opened.
  • One instance of com.jcraft.jsch.JSchException: Session.connect: java.net.SocketTimeoutException: Read timed out.

I don't believe any of these are caused by latent bugs in the new crypto code, so I believe the issues with the AES-GCM & X25519 implementations may finally be solved with the changes contained in this PR.

@norrisjeremy
Copy link
Contributor Author

FYI, on a separate note: I now have an implementation of ssh-ed25519 that can be added.
I'll submit a new PR after we get this PR wrapped up.

@mwiede mwiede merged commit 1416308 into mwiede:master Oct 15, 2020
@norrisjeremy norrisjeremy deleted the fix_gcm branch October 15, 2020 13:28
@norrisjeremy norrisjeremy mentioned this pull request Aug 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@norrisjeremy @mwiede