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.

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

8311902: Concurrency regression in the PBKDF2 key impl of SunJCE provider #14859

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you also want to revert the changes made to getPassword().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think the change which moved the registering of the Cleaner outside the finally block in the constructor is not correct, as the passwd is no longer zero-ed out if the code after that throws an Exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my reading of the code. the cleaner is only used when the PBKDF2 key constructor succeeds. If an exception occurred, then the passwd cleanup is handled by the if (key == null) condition in the finally block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, took another closer look at the code and you are right. So, never mind this comment.

Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@
package com.sun.crypto.provider;

import java.io.ObjectStreamException;
import java.lang.ref.Reference;
import java.lang.ref.Cleaner;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
@@ -205,7 +206,12 @@ public boolean equals(Object obj) {
}

public byte[] getEncoded() {
return key.clone();
try {
return key.clone();
} finally {
// prevent this from being cleaned for the above block
Reference.reachabilityFence(this);
}
}

public String getAlgorithm() {
@@ -221,7 +227,12 @@ public void clear() {
}

public char[] getPassword() {
return passwd.clone();
try {
return passwd.clone();
} finally {
// prevent this from being cleaned for the above block
Reference.reachabilityFence(this);
}
}

public byte[] getSalt() {
@@ -237,30 +248,45 @@ public String getFormat() {
* Objects that are equal will also have the same hashcode.
*/
public int hashCode() {
int retval = 0;
for (int i = 1; i < this.key.length; i++) {
retval += this.key[i] * i;
try {
int retval = 0;
for (int i = 1; i < this.key.length; i++) {
retval += this.key[i] * i;
}
return (retval ^= getAlgorithm().toLowerCase
(Locale.ENGLISH).hashCode());
} finally {
// prevent this from being cleaned for the above block
Reference.reachabilityFence(this);
}
return(retval ^= getAlgorithm().toLowerCase(Locale.ENGLISH).hashCode());
}

public boolean equals(Object obj) {
if (obj == this)
return true;
try {
if (obj == this) {
return true;
}

if (!(obj instanceof SecretKey))
return false;
if (!(obj instanceof SecretKey)) {
return false;
}

SecretKey that = (SecretKey) obj;
SecretKey that = (SecretKey) obj;

if (!(that.getAlgorithm().equalsIgnoreCase(getAlgorithm())))
return false;
if (!(that.getFormat().equalsIgnoreCase("RAW")))
return false;
byte[] thatEncoded = that.getEncoded();
boolean ret = MessageDigest.isEqual(key, thatEncoded);
Arrays.fill(thatEncoded, (byte)0x00);
return ret;
if (!(that.getAlgorithm().equalsIgnoreCase(getAlgorithm()))) {
return false;
}
if (!(that.getFormat().equalsIgnoreCase("RAW"))) {
return false;
}
byte[] thatEncoded = that.getEncoded();
boolean ret = MessageDigest.isEqual(key, thatEncoded);
Arrays.fill(thatEncoded, (byte)0x00);
return ret;
} finally {
// prevent this from being cleaned for the above block
Reference.reachabilityFence(this);
}
}

/**
@@ -273,7 +299,12 @@ public boolean equals(Object obj) {
*/
@java.io.Serial
private Object writeReplace() throws ObjectStreamException {
return new KeyRep(KeyRep.Type.SECRET, getAlgorithm(),
getFormat(), key);
try {
return new KeyRep(KeyRep.Type.SECRET, getAlgorithm(),
getFormat(), key);
} finally {
// prevent this from being cleaned for the above block
Reference.reachabilityFence(this);
}
}
}