fix(739) SignatureECDSAN destroying private key #740

ikucuze · 2025-01-07T11:39:14Z

fix for bug 739

ikucuze · 2025-01-07T11:40:00Z

fixes: #739

norrisjeremy · 2025-01-07T11:43:25Z

No, I very much oppose this.
We are not going to stop zero'ing private key data.

ikucuze · 2025-01-07T12:15:47Z

PR updated to always perform a copy of the array instead of not 0-ing it

norrisjeremy

This seems acceptable to me by best limiting any potential impact.

norrisjeremy · 2025-01-07T13:09:43Z

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

@@ -351,7 +351,8 @@ public byte[] getSignature(byte[] data) {
              .asSubclass(SignatureECDSA.class);
      SignatureECDSA ecdsa = c.getDeclaredConstructor().newInstance();
      ecdsa.init();
-      ecdsa.setPrvKey(prv_array);
+      // issue 730: prv_array could be destroyed by ecdsa signing process if its first bit is 1


Can you fix this to correctly point to issue 739 and not issue 730 (which is unrelated).

Also, I would prefer if we included a full link to https://github.com/mwiede/jsch/issues/739 in the comment text, so that future readers don't have to figure out what issue 739 means.

Sorry. don't know how I did that

made me realize 2 things:
This copy of the private key has to be cleaned... as we don't know if ecdsa signing will clean it...
So I need Util.bzero to be changed to public.
Would you prefer then:

byte[] keyCopy = Arrays.copyOf(prv_array, prv_array.length); ecdsa.setPrvKey(keyCopy); Util.bzero(keyCopy);

I also need to change the caller of setPubKey with the same fix :(

I pushed a new secured fixed regarding cleaning the key copy.
The resulting code starts to get ugly because of that unneeded copy...

Please revert those changes and simply constrain this change to making a copy of prv_array that is passed to ecdsa.setPrvKey().

The whole point of what I am asking you to do is to make as minimally invasive changes as possible to fix the issue you reported.
That way we reduce the chance of introducing any sort of regression or any sort of new vulnerability as much as possible.
The JSch codebase is very old and extremely fragile, and we try to minimize risk as much as possible.

made me realize 2 things: This copy of the private key has to be cleaned... as we don't know if ecdsa signing will clean it... So I need Util.bzero to be changed to public. Would you prefer then:

byte[] keyCopy = Arrays.copyOf(prv_array, prv_array.length); ecdsa.setPrvKey(keyCopy); Util.bzero(keyCopy);

I also need to change the caller of setPubKey with the same fix :(

There is zero reason to zero the public key, as that is not sensitive data.

ok, just pushed a fix only for private key, adding a cleaning to the copy for security purpose

Please revert the unnecessary changes to src/main/java/com/jcraft/jsch/jce/Util.java.

norrisjeremy · 2025-01-07T13:20:11Z

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

@@ -351,7 +351,8 @@ public byte[] getSignature(byte[] data) {
              .asSubclass(SignatureECDSA.class);
      SignatureECDSA ecdsa = c.getDeclaredConstructor().newInstance();
      ecdsa.init();
-      ecdsa.setPrvKey(prv_array);
+      // issue 739: prv_array could be destroyed by ecdsa signing process if its first bit is 1


Can you change this to point at https://github.com/mwiede/jsch/issues/739 instead of issue 739?
That way future readers don't have to figure out how to find what issue 739 means.

norrisjeremy

Please revert all these changes back to the minimally invasive change you had originally, which was to simply to call escda.setPrvKey(Arrays.copyOf(prv_array, prv_array.length));.

The whole point of this is to minimize risk as much as possible (both for any regressions as well as any newly introduced vulnerabilities) whilst fixing the issue you originally reported, which is that you were unable to create multiple sessions from a single JSch object instance.

norrisjeremy · 2025-01-07T14:17:06Z

src/main/java/com/jcraft/jsch/jce/Util.java

@@ -26,8 +26,8 @@

 package com.jcraft.jsch.jce;

-class Util {
-  static void bzero(byte[] foo) {
+public class Util {


There is no reason for this class to be made public.
Please revert this change.

needed to clean the copy

This change is unneeded though to accomplish what you have proposed.
src/main/java/com/jcraft/jsch/KeyPairECDSA.java will be utilizing the class src/main/java/com/jcraft/jsch/Util.java, not src/main/java/com/jcraft/jsch/jce/Util.java.
Therefore there is no need for the changes to make src/main/java/com/jcraft/jsch/jce/Util.java public.

Right, I was unaware of this duplicate code... fixed.

ikucuze · 2025-01-07T14:27:56Z

escda.setPrvKey(Arrays.copyOf(prv_array, prv_array.length));.

This is not acceptable as the copy array might stay in memory without cleaning in case of a positive number, thus setPrvKey won't clean it.
You fought for keeping the cleaning in SignatureECDSAN even if the priv_key is a parameter and not under its responsability. I think the responsability of the cleaning is in the hand of the code originating the data. Thus KeyPairECDSA is now reponsible to clean this copy, and for me was also was responsible for cleaning the original prv_array (and it does that correctly in its dispose() ).

norrisjeremy · 2025-01-07T14:31:52Z

escda.setPrvKey(Arrays.copyOf(prv_array, prv_array.length));.

This is not acceptable as the copy array might stay in memory without cleaning in case of a positive number, thus setPrvKey won't clean it. You fought for keeping the cleaning in SignatureECDSAN even if the priv_key is a parameter and not under its responsability. I think the responsability of the cleaning is in the hand of the code originating the data. Thus KeyPairECDSA is now reponsible to clean this copy, and for me was also was responsible for cleaning the original prv_array (and it does that correctly in its dispose() ).

I'll agree to keeping the introduction of Util.bzero() of the prv_array copy.

ikucuze · 2025-01-07T14:35:20Z

I'll agree to keeping the introduction of Util.bzero() of the prv_array copy.

Ok, pushed that. Copy of public key do not get cleaned.
Copy of private key is done, by making Util public.
pushed that now

norrisjeremy

Please execute mvn formatter:format so that your changes are properly formatted and allow the Maven build to succeed.

sonarqubecloud · 2025-01-07T14:43:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

norrisjeremy

I am good with these changes now.

ikucuze mentioned this pull request Jan 7, 2025

SignatureECDSAN destroys private key #739

Open

ikucuze force-pushed the master branch from b9b6137 to d0549e7 Compare January 7, 2025 12:14

norrisjeremy approved these changes Jan 7, 2025

View reviewed changes

norrisjeremy suggested changes Jan 7, 2025

View reviewed changes

ikucuze force-pushed the master branch from d0549e7 to ba66084 Compare January 7, 2025 13:17

norrisjeremy reviewed Jan 7, 2025

View reviewed changes

ikucuze force-pushed the master branch 3 times, most recently from c8324cc to 25a0496 Compare January 7, 2025 13:42

norrisjeremy suggested changes Jan 7, 2025

View reviewed changes

ikucuze force-pushed the master branch 2 times, most recently from 1d013ef to dd77694 Compare January 7, 2025 14:17

norrisjeremy suggested changes Jan 7, 2025

View reviewed changes

ikucuze force-pushed the master branch from dd77694 to 949a8db Compare January 7, 2025 14:33

ikucuze force-pushed the master branch from 949a8db to 9d2dfad Compare January 7, 2025 14:37

norrisjeremy suggested changes Jan 7, 2025

View reviewed changes

fix(739) SignatureECDSAN destroying private key

6156ad3

ikucuze force-pushed the master branch from 9d2dfad to 6156ad3 Compare January 7, 2025 14:43

norrisjeremy approved these changes Jan 7, 2025

View reviewed changes

mwiede merged commit b7e92bf into mwiede:master Jan 8, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(739) SignatureECDSAN destroying private key #740

fix(739) SignatureECDSAN destroying private key #740

ikucuze commented Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy commented Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy left a comment

norrisjeremy Jan 7, 2025

norrisjeremy Jan 7, 2025

ikucuze Jan 7, 2025

ikucuze Jan 7, 2025 •

edited

Loading

ikucuze Jan 7, 2025

norrisjeremy Jan 7, 2025

norrisjeremy Jan 7, 2025

ikucuze Jan 7, 2025

norrisjeremy Jan 7, 2025

norrisjeremy Jan 7, 2025

ikucuze Jan 7, 2025

norrisjeremy left a comment

norrisjeremy Jan 7, 2025

ikucuze Jan 7, 2025

norrisjeremy Jan 7, 2025

ikucuze Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy commented Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy left a comment

sonarqubecloud bot commented Jan 7, 2025

norrisjeremy left a comment

fix(739) SignatureECDSAN destroying private key #740

fix(739) SignatureECDSAN destroying private key #740

Conversation

ikucuze commented Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy commented Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikucuze Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norrisjeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikucuze commented Jan 7, 2025

norrisjeremy commented Jan 7, 2025

ikucuze commented Jan 7, 2025

norrisjeremy left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 7, 2025

Quality Gate passed

norrisjeremy left a comment

Choose a reason for hiding this comment

ikucuze Jan 7, 2025 •

edited

Loading