You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2022/12/12 18:21:36 UTC
[commons-crypto] branch master updated: [CRYPTO-160] Package-private class JavaCryptoRandom extends Random but should not
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-crypto.git
The following commit(s) were added to refs/heads/master by this push:
new a484db6 [CRYPTO-160] Package-private class JavaCryptoRandom extends Random but should not
a484db6 is described below
commit a484db66f6e4b4c7748a7a07e3def44853ad659b
Author: Gary Gregory <ga...@gmail.com>
AuthorDate: Mon Dec 12 13:21:31 2022 -0500
[CRYPTO-160] Package-private class JavaCryptoRandom extends Random but
should not
---
src/changes/changes.xml | 1 +
.../commons/crypto/jna/OpenSslJnaCryptoRandom.java | 42 +-------------
.../commons/crypto/random/JavaCryptoRandom.java | 67 +++++++++-------------
.../commons/crypto/random/OpenSslCryptoRandom.java | 39 +------------
.../commons/crypto/random/OsCryptoRandom.java | 23 +-------
.../crypto/random/JavaCryptoRandomTest.java | 21 +------
6 files changed, 31 insertions(+), 162 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 0a4d7ad..fb108fd 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -73,6 +73,7 @@
<action type="fix" dev="ggregory" due-to="Gary Gregory">Fix build on Java 17.</action>
<action issue="CRYPTO-155" type="fix" due-to="Arturo Bernal">Minor improvement #115, #125.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">PositionedCryptoInputStream does not close its CryptoCipher instances.</action>
+ <action issue="CRYPTO-160" type="fix" dev="ggregory" due-to="Gary Gregory">Package-private class JavaCryptoRandom extends Random but should not.</action>
<!-- ADD -->
<action type="fix" dev="ggregory" due-to="Gary Gregory, Dependabot">Add github/codeql-action 2 #159.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory, Dependabot">Add AES utility class.</action>
diff --git a/src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java b/src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java
index 3f4cdbb..b839e1d 100644
--- a/src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java
+++ b/src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java
@@ -21,10 +21,8 @@ import java.nio.ByteBuffer;
import java.security.GeneralSecurityException;
import java.security.NoSuchAlgorithmException;
import java.util.Properties;
-import java.util.Random;
import org.apache.commons.crypto.random.CryptoRandom;
-import org.apache.commons.crypto.utils.Utils;
import com.sun.jna.NativeLong;
import com.sun.jna.ptr.PointerByReference;
@@ -46,9 +44,8 @@ import com.sun.jna.ptr.PointerByReference;
* @see <a href="http://en.wikipedia.org/wiki/RdRand">
* http://en.wikipedia.org/wiki/RdRand</a>
*/
-class OpenSslJnaCryptoRandom extends Random implements CryptoRandom {
+class OpenSslJnaCryptoRandom implements CryptoRandom {
- private static final long serialVersionUID = -7128193502768749585L;
private final boolean rdrandEnabled;
private final transient PointerByReference rdrandEngine;
@@ -117,43 +114,6 @@ class OpenSslJnaCryptoRandom extends Random implements CryptoRandom {
}
}
- /**
- * Overrides {@link OpenSslJnaCryptoRandom}. For {@link OpenSslJnaCryptoRandom},
- * we don't need to set seed.
- *
- * @param seed the initial seed, ignored.
- */
- @SuppressWarnings("sync-override") // Empty implementation.
- @Override
- public void setSeed(final long seed) {
- // Self-seeding.
- }
-
- /**
- * Overrides Random#next(). Generates an integer containing the
- * user-specified number of random bits(right justified, with leading
- * zeros).
- *
- * @param numBits number of random bits to be generated, where 0
- * {@literal <=} {@code numBits} {@literal <=} 32.
- * @return int an {@code int} containing the user-specified number of
- * random bits (right justified, with leading zeros).
- */
- @Override
- final protected int next(final int numBits) {
- Utils.checkArgument(numBits >= 0 && numBits <= 32);
- final int numBytes = (numBits + 7) / 8;
- final byte[] b = new byte[numBytes];
- int next = 0;
-
- nextBytes(b);
- for (int i = 0; i < numBytes; i++) {
- next = (next << 8) + (b[i] & 0xFF);
- }
-
- return next >>> (numBytes * 8 - numBits);
- }
-
/**
* Overrides {@link java.lang.AutoCloseable#close()}. Closes OpenSSL context
* if native enabled.
diff --git a/src/main/java/org/apache/commons/crypto/random/JavaCryptoRandom.java b/src/main/java/org/apache/commons/crypto/random/JavaCryptoRandom.java
index 627532e..47a0f69 100644
--- a/src/main/java/org/apache/commons/crypto/random/JavaCryptoRandom.java
+++ b/src/main/java/org/apache/commons/crypto/random/JavaCryptoRandom.java
@@ -1,26 +1,25 @@
- /*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
+/*
+* Licensed to the Apache Software Foundation (ASF) under one
+* or more contributor license agreements. See the NOTICE file
+* distributed with this work for additional information
+* regarding copyright ownership. The ASF licenses this file
+* to you under the Apache License, Version 2.0 (the
+* "License"); you may not use this file except in compliance
+* with the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
package org.apache.commons.crypto.random;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Properties;
-import java.util.Random;
import org.apache.commons.crypto.utils.Utils;
@@ -31,22 +30,15 @@ import org.apache.commons.crypto.utils.Utils;
* CryptoRandomFactory.RandomProvider
* </p>
*/
-class JavaCryptoRandom extends Random implements CryptoRandom {
-
- /**
- * Generated serialVersionUID.
- */
- private static final long serialVersionUID = 5517475898166660050L;
+class JavaCryptoRandom implements CryptoRandom {
private final SecureRandom instance;
/**
* Constructs a {@link JavaCryptoRandom}.
*
- * @param properties the configuration properties.
- * Uses the key {@link CryptoRandomFactory#JAVA_ALGORITHM_KEY}
- * to get the name of the algorithm, with a default of
- * {@link CryptoRandomFactory#JAVA_ALGORITHM_DEFAULT}
+ * @param properties the configuration properties. Uses the key {@link CryptoRandomFactory#JAVA_ALGORITHM_KEY} to get the name of the algorithm, with a
+ * default of {@link CryptoRandomFactory#JAVA_ALGORITHM_DEFAULT}
*/
public JavaCryptoRandom(final Properties properties) {
SecureRandom tmp;
@@ -59,8 +51,7 @@ class JavaCryptoRandom extends Random implements CryptoRandom {
}
/**
- * Overrides {@link java.lang.AutoCloseable#close()}. For
- * {@link JavaCryptoRandom}, we don't need to recycle resource.
+ * Overrides {@link java.lang.AutoCloseable#close()}. For {@link JavaCryptoRandom}, we don't need to recycle resource.
*/
@Override
public void close() {
@@ -68,9 +59,8 @@ class JavaCryptoRandom extends Random implements CryptoRandom {
}
/**
- * Overrides {@link CryptoRandom#nextBytes(byte[])}. Generates random bytes
- * and places them into a user-supplied byte array. The number of random
- * bytes produced is equal to the length of the byte array.
+ * Overrides {@link CryptoRandom#nextBytes(byte[])}. Generates random bytes and places them into a user-supplied byte array. The number of random bytes
+ * produced is equal to the length of the byte array.
*
* @param bytes the array to be filled in with random bytes.
*/
@@ -80,16 +70,11 @@ class JavaCryptoRandom extends Random implements CryptoRandom {
}
/**
- * Overrides Random#next(). Generates an integer containing the
- * user-specified number of random bits(right justified, with leading
- * zeros).
+ * Overrides Random#next(). Generates an integer containing the user-specified number of random bits(right justified, with leading zeros).
*
- * @param numBits number of random bits to be generated, where 0
- * {@literal <=} {@code numBits} {@literal <=} 32.
- * @return int an {@code int} containing the user-specified number of
- * random bits (right justified, with leading zeros).
+ * @param numBits number of random bits to be generated, where 0 {@literal <=} {@code numBits} {@literal <=} 32.
+ * @return int an {@code int} containing the user-specified number of random bits (right justified, with leading zeros).
*/
- @Override
protected int next(final int numBits) {
Utils.checkArgument(numBits >= 0 && numBits <= 32);
// Can't simply invoke instance.next(bits) here, because that is package protected.
diff --git a/src/main/java/org/apache/commons/crypto/random/OpenSslCryptoRandom.java b/src/main/java/org/apache/commons/crypto/random/OpenSslCryptoRandom.java
index 67a9b3b..591fd31 100644
--- a/src/main/java/org/apache/commons/crypto/random/OpenSslCryptoRandom.java
+++ b/src/main/java/org/apache/commons/crypto/random/OpenSslCryptoRandom.java
@@ -22,7 +22,6 @@ import java.util.Properties;
import java.util.Random;
import org.apache.commons.crypto.Crypto;
-import org.apache.commons.crypto.utils.Utils;
/**
* <p>
@@ -41,8 +40,7 @@ import org.apache.commons.crypto.utils.Utils;
* @see <a href="http://en.wikipedia.org/wiki/RdRand">
* http://en.wikipedia.org/wiki/RdRand</a>
*/
-class OpenSslCryptoRandom extends Random implements CryptoRandom {
- private static final long serialVersionUID = -7828193502768789584L;
+class OpenSslCryptoRandom implements CryptoRandom {
private static final boolean nativeEnabled;
@@ -112,31 +110,6 @@ class OpenSslCryptoRandom extends Random implements CryptoRandom {
// noop
}
- /**
- * Overrides Random#next(). Generates an integer containing the
- * user-specified number of random bits(right justified, with leading
- * zeros).
- *
- * @param numBits number of random bits to be generated, where 0
- * {@literal <=} {@code numBits} {@literal <=} 32.
- * @return int an {@code int} containing the user-specified number of
- * random bits (right justified, with leading zeros).
- */
- @Override
- protected final int next(final int numBits) {
- Utils.checkArgument(numBits >= 0 && numBits <= 32);
- final int numBytes = (numBits + 7) / 8;
- final byte[] b = new byte[numBytes];
- int next = 0;
-
- nextBytes(b);
- for (int i = 0; i < numBytes; i++) {
- next = (next << 8) + (b[i] & 0xFF);
- }
-
- return next >>> (numBytes * 8 - numBits);
- }
-
/**
* Generates a user-specified number of random bytes. It's thread-safe.
* Overrides {@link Random}.
@@ -152,14 +125,4 @@ class OpenSslCryptoRandom extends Random implements CryptoRandom {
}
}
- /**
- * Overrides {@link Random} (which is synchronized).
- * We don't need to set seed for this class.
- *
- * @param seed the initial seed.
- */
- @Override
- public synchronized void setSeed(final long seed) {
- // Self-seeding.
- }
}
diff --git a/src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java b/src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
index 5e145cf..f5b06d3 100644
--- a/src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
+++ b/src/main/java/org/apache/commons/crypto/random/OsCryptoRandom.java
@@ -21,7 +21,6 @@ import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.util.Properties;
-import java.util.Random;
import org.apache.commons.crypto.utils.IoUtils;
@@ -29,9 +28,7 @@ import org.apache.commons.crypto.utils.IoUtils;
* A Random implementation that uses random bytes sourced from the operating
* system.
*/
-class OsCryptoRandom extends Random implements CryptoRandom {
-
- private static final long serialVersionUID = 6391500337172057900L;
+class OsCryptoRandom implements CryptoRandom {
private static final int RESERVOIR_LENGTH = 8192;
@@ -107,24 +104,6 @@ class OsCryptoRandom extends Random implements CryptoRandom {
}
}
- /**
- * Overrides Random#next(). Generates the next pseudorandom number.
- * Subclasses should override this, as this is used by all other methods.
- *
- * @param nbits random bits.
- * @return the next pseudorandom value from this random number generator's
- * sequence.
- */
- @Override
- synchronized protected int next(final int nbits) {
- fillReservoir(4);
- int n = 0;
- for (int i = 0; i < 4; i++) {
- n = (n << 8) | (reservoir[pos++] & 0xff);
- }
- return n & (0xffffffff >> (32 - nbits));
- }
-
/**
* Overrides {@link java.lang.AutoCloseable#close()}. Closes the OS stream.
*/
diff --git a/src/test/java/org/apache/commons/crypto/random/JavaCryptoRandomTest.java b/src/test/java/org/apache/commons/crypto/random/JavaCryptoRandomTest.java
index e4eb5bd..6ad26d4 100644
--- a/src/test/java/org/apache/commons/crypto/random/JavaCryptoRandomTest.java
+++ b/src/test/java/org/apache/commons/crypto/random/JavaCryptoRandomTest.java
@@ -17,17 +17,13 @@
*/
package org.apache.commons.crypto.random;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.security.GeneralSecurityException;
import java.util.Properties;
-import java.util.Random;
-
-import org.junit.jupiter.api.Test;
public class JavaCryptoRandomTest extends AbstractRandomTest {
+
@Override
public CryptoRandom getCryptoRandom() throws GeneralSecurityException {
final Properties props = new Properties();
@@ -37,19 +33,4 @@ public class JavaCryptoRandomTest extends AbstractRandomTest {
return random;
}
- @Test
- public void testNextIntIsntActuallyRandomNextInt() throws Exception {
- final CryptoRandom cr = getCryptoRandom();
- final Random r = (Random) cr;
- final long seed = 1654421930011L; // System.getCurrentMillis() on 2022-June-05, 11:39
- final Random otherRandom = new Random(seed);
- final Random otherRandom2 = new Random();
- otherRandom2.setSeed(seed);
- r.setSeed(seed);
- final long l1 = r.nextLong();
- final long l2 = otherRandom.nextLong();
- final long l3 = otherRandom2.nextLong();
- assertEquals(l2, l3);
- assertNotEquals(l1, l2);
- }
}