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

Remove use of BouncyCastle for EC key generation and point (de)compression #129

Merged
merged 3 commits into from
Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
### Maintenance
* Add support for standard test vectors via `testVectorZip` system property.
* No longer require use of BouncyCastle with RSA `JceMasterKey`s
* No longer use BouncyCastle for Elliptic Curve key generation and point compression/decompression

## 1.6.0 -- 2019-05-31

Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
package com.amazonaws.encryptionsdk.internal;

import java.security.GeneralSecurityException;
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.PublicKey;

import org.bouncycastle.crypto.params.ECDomainParameters;
import org.bouncycastle.crypto.params.ECPublicKeyParameters;
import org.bouncycastle.jcajce.provider.asymmetric.ec.BCECPublicKey;
import org.bouncycastle.jce.ECNamedCurveTable;
import org.bouncycastle.jce.interfaces.ECPublicKey;
import org.bouncycastle.jce.provider.BouncyCastleProvider;
import org.bouncycastle.jce.spec.ECNamedCurveParameterSpec;
import org.bouncycastle.math.ec.ECPoint;
import java.math.BigInteger;
import java.security.*;
import java.security.interfaces.ECPublicKey;
import java.security.spec.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

No import stars please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and updated my IDE

import java.util.Arrays;

import com.amazonaws.encryptionsdk.CryptoAlgorithm;

import static com.amazonaws.encryptionsdk.internal.BouncyCastleConfiguration.INTERNAL_BOUNCY_CASTLE_PROVIDER;
import static com.amazonaws.encryptionsdk.internal.Utils.bigIntegerToByteArray;
import static com.amazonaws.encryptionsdk.internal.Utils.encodeBase64String;
import static org.apache.commons.lang3.Validate.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto on import star.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

import static java.math.BigInteger.ONE;
import static java.math.BigInteger.ZERO;

/**
* Provides a consistent interface across various trailing signature algorithms.
Expand All @@ -36,15 +32,36 @@ private TrailingSignatureAlgorithm() {
public abstract String serializePublicKey(PublicKey key);
public abstract KeyPair generateKey() throws GeneralSecurityException;

/** Standards for Efficient Cryptography over a prime field **/
private static final String SEC_PRIME_FIELD_PREFIX = "secp";

private static final class ECDSASignatureAlgorithm extends TrailingSignatureAlgorithm {
private final ECNamedCurveParameterSpec ecSpec;
private final ECGenParameterSpec ecSpec;
private final ECParameterSpec ecParameterSpec;
private final String messageDigestAlgorithm;
private final String hashAndSignAlgorithm;
private static final String ELLIPTIC_CURVE_ALGORITHM = "EC";
/* Constants used by SEC-1 v2 point compression and decompression algorithms */
private static final BigInteger TWO = BigInteger.valueOf(2);
private static final BigInteger THREE = BigInteger.valueOf(3);
private static final BigInteger FOUR = BigInteger.valueOf(4);

private ECDSASignatureAlgorithm(ECGenParameterSpec ecSpec, String messageDigestAlgorithm) {
if(!ecSpec.getName().startsWith(SEC_PRIME_FIELD_PREFIX)) {
throw new IllegalStateException("Non-prime curves are not supported at this time");
}

private ECDSASignatureAlgorithm(ECNamedCurveParameterSpec ecSpec, String messageDigestAlgorithm) {
this.ecSpec = ecSpec;
this.messageDigestAlgorithm = messageDigestAlgorithm;
this.hashAndSignAlgorithm = messageDigestAlgorithm + "withECDSA";

try {
final AlgorithmParameters parameters = AlgorithmParameters.getInstance(ELLIPTIC_CURVE_ALGORITHM);
parameters.init(ecSpec);
this.ecParameterSpec = parameters.getParameterSpec(ECParameterSpec.class);
} catch (NoSuchAlgorithmException | InvalidParameterSpecException e) {
throw new IllegalStateException("Invalid algorithm", e);
}
}

@Override
Expand All @@ -66,37 +83,99 @@ public String getRawSignatureAlgorithm() {
return hashAndSignAlgorithm;
}

/**
* Decodes a compressed elliptic curve point as described in SEC-1 v2 section 2.3.4
* @see <a href="http://www.secg.org/sec1-v2.pdf">http://www.secg.org/sec1-v2.pdf</a>
* @param keyString The serialized and compressed public key
* @return The PublicKey
*/
@Override
public PublicKey deserializePublicKey(String keyString) {
final ECPoint q = ecSpec.getCurve().decodePoint(Utils.decodeBase64String(keyString));

ECPublicKeyParameters keyParams = new ECPublicKeyParameters(
q,
new ECDomainParameters(ecSpec.getCurve(), ecSpec.getG(), ecSpec.getN(), ecSpec.getH())
);

return new BCECPublicKey("EC", keyParams, ecSpec, BouncyCastleProvider.CONFIGURATION);
notNull(keyString, "keyString is required");

final byte[] decodedKey = Utils.decodeBase64String(keyString);
final BigInteger x = new BigInteger(1, Arrays.copyOfRange(decodedKey, 1, decodedKey.length));

final byte compressedY = decodedKey[0];
final BigInteger yOrder;

if(compressedY == TWO.byteValue()) {
yOrder = ZERO;
} else if (compressedY == THREE.byteValue()) {
yOrder = ONE;
} else {
throw new IllegalArgumentException("Compressed y value was invalid");
}

final BigInteger p = ((ECFieldFp)ecParameterSpec.getCurve().getField()).getP();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and throughout: Please have a space between the closing parenthesis of the cast and the following token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

final BigInteger a = ecParameterSpec.getCurve().getA();
final BigInteger b = ecParameterSpec.getCurve().getB();

final BigInteger alpha = x.modPow(THREE, p)
SalusaSecondus marked this conversation as resolved.
Show resolved Hide resolved
.add(a.multiply(x).mod(p))
.add(b)
.mod(p);

final BigInteger beta;
if(p.mod(FOUR).equals(THREE)) {
beta = alpha.modPow(p.add(ONE).divide(FOUR), p);
} else {
throw new IllegalArgumentException("Curve not supported at this time");
}

final BigInteger y = beta.mod(TWO).equals(yOrder) ? beta : p.subtract(beta);

//Validate that Y is a root of Y^2 to prevent invalid point attacks
if(!alpha.equals(y.modPow(TWO, p))) {
throw new IllegalArgumentException("Y was invalid");
}

try {
return KeyFactory.getInstance(ELLIPTIC_CURVE_ALGORITHM).generatePublic(
new ECPublicKeySpec(new ECPoint(x, y), ecParameterSpec));
} catch (InvalidKeySpecException | NoSuchAlgorithmException e) {
throw new IllegalStateException("Invalid algorithm", e);
}
}

/**
* Encodes a compressed elliptic curve point as described in SEC-1 v2 section 2.3.3
* @see <a href="http://www.secg.org/sec1-v2.pdf">http://www.secg.org/sec1-v2.pdf</a>
* @param key The Elliptic Curve public key to compress and serialize
* @return The serialized and compressed public key
*/
@Override
public String serializePublicKey(PublicKey key) {
return Utils.encodeBase64String(((ECPublicKey)key).getQ().getEncoded(true));
notNull(key, "key is required");
isInstanceOf(ECPublicKey.class, key, "key must be an instance of ECPublicKey");

final BigInteger x = ((ECPublicKey)key).getW().getAffineX();
final BigInteger y = ((ECPublicKey)key).getW().getAffineY();
final BigInteger compressedY = y.mod(TWO).equals(ZERO) ? TWO : THREE;

final byte[] xBytes = bigIntegerToByteArray(x,
ecParameterSpec.getCurve().getField().getFieldSize() / Byte.SIZE);

final byte[] compressedKey = new byte[xBytes.length + 1];
System.arraycopy(xBytes, 0, compressedKey, 1, xBytes.length);
compressedKey[0] = compressedY.byteValue();

return encodeBase64String(compressedKey);
}

@Override
public KeyPair generateKey() throws GeneralSecurityException {
// We use BouncyCastle for this so that we can easily serialize the compressed point.
KeyPairGenerator keyGen = KeyPairGenerator.getInstance("EC", INTERNAL_BOUNCY_CASTLE_PROVIDER);
KeyPairGenerator keyGen = KeyPairGenerator.getInstance(ELLIPTIC_CURVE_ALGORITHM);
keyGen.initialize(ecSpec, Utils.getSecureRandom());

return keyGen.generateKeyPair();
}
}

private static final ECDSASignatureAlgorithm SHA256_ECDSA_P256
= new ECDSASignatureAlgorithm(ECNamedCurveTable.getParameterSpec("secp256r1"), "SHA256");
= new ECDSASignatureAlgorithm(new ECGenParameterSpec(SEC_PRIME_FIELD_PREFIX + "256r1"), "SHA256");
private static final ECDSASignatureAlgorithm SHA384_ECDSA_P384
= new ECDSASignatureAlgorithm(ECNamedCurveTable.getParameterSpec("secp384r1"), "SHA384");
= new ECDSASignatureAlgorithm(new ECGenParameterSpec(SEC_PRIME_FIELD_PREFIX + "384r1"), "SHA384");

public static TrailingSignatureAlgorithm forCryptoAlgorithm(CryptoAlgorithm algorithm) {
switch (algorithm) {
Expand Down
27 changes: 27 additions & 0 deletions src/main/java/com/amazonaws/encryptionsdk/internal/Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.amazonaws.encryptionsdk.internal;

import java.io.Serializable;
import java.math.BigInteger;
import java.nio.Buffer;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -279,4 +280,30 @@ public static byte[] decodeBase64String(final String encoded) {
public static String encodeBase64String(final byte[] data) {
return Base64.toBase64String(data);
}

/**
* Removes the leading zero sign byte from the byte array representation of a BigInteger (if present)
* and left pads with zeroes to produce a byte array of the given length.
* @param bigInteger The BigInteger to convert to a byte array
* @param length The length of the byte array
* @return The byte array
*/
public static byte[] bigIntegerToByteArray(final BigInteger bigInteger, final int length) {
final byte[] result = new byte[length];
byte[] rawBytes = bigInteger.toByteArray();

//Remove sign byte if one is present
if(rawBytes[0] == 0) {
rawBytes = Arrays.copyOfRange(rawBytes, 1, rawBytes.length);
}

if(length < rawBytes.length) {
throw new IllegalArgumentException("Length must be as least as long as the BigInteger byte array " +
"without the sign byte");
}

System.arraycopy(rawBytes, 0, result, length - rawBytes.length, rawBytes.length);

return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's simplify this (and reduce unneeded allocations).

final byte[] rawBytes = bigInteger.toByteArray();
// If we're already the correct length, just return it.
if (rawBytes.length ==length) {
    return rawBytes;
}
// If we're exactly one byte too large, but we have a leading zero byte, remove it and return.
if (rawBytes.length == length + 1 && rawBytes[0] == 0) {
    return Arrays.copyOfRange(rawBytes, 1, rawBytes.length);
}

if (rawBytes.length > length) {
    throw new IllegalArgumentException("Length must be as least as long as the BigInteger byte array " +
      "without the sign byte");
}
final byte[] paddedResult = new byte[length];
System.arraycopy(rawBytes, 0, paddedResult , length - rawBytes.length, rawBytes.length);
return paddedResult;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
35 changes: 35 additions & 0 deletions src/test/java/com/amazonaws/encryptionsdk/TestUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,39 @@ public static int[] getFrameSizesToTest(final CryptoAlgorithm cryptoAlg) {
};
return frameSizeToTest;
}

/**
* Converts an array of unsigned bytes (represented as int values between 0 and 255 inclusive)
* to an array of Java primitive type byte, which are by definition signed.
* @param unsignedBytes An array on unsigned bytes
* @return An array of signed bytes
*/
public static byte[] unsignedBytesToSignedBytes(final int[] unsignedBytes) {
byte[] signedBytes = new byte[unsignedBytes.length];

for(int i= 0 ; i < unsignedBytes.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and throughout: space after if, for, and other keywords to distinguish them from function calls.

Here: Space on either side of the equals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if(unsignedBytes[i] > 255) {
throw new IllegalArgumentException("Encountered unsigned byte value > 255");
}
signedBytes[i] = (byte)(unsignedBytes[i] & 0xff);
}

return signedBytes;
}

/**
* Converts an array of Java primitive type bytes (which are by definition signed) to
* an array of unsigned bytes (represented as int values between 0 and 255 inclusive).
* @param signedBytes An array of signed bytes
* @return An array of unsigned bytes
*/
public static int[] signedBytesToUnsignedBytes(final byte[] signedBytes) {
int[] unsignedBytes = new int[signedBytes.length];

for(int i= 0 ; i < signedBytes.length; i++) {
unsignedBytes[i] = ((int)signedBytes[i]) & 0xff;
}

return unsignedBytes;
}
}
15 changes: 15 additions & 0 deletions src/test/java/com/amazonaws/encryptionsdk/UtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertTrue;

import java.math.BigInteger;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;

Expand Down Expand Up @@ -96,5 +97,19 @@ public void base64something() {
assertEquals(encoded, Utils.encodeBase64String(data));
assertArrayEquals(data, Utils.decodeBase64String(encoded));
}

@Test
public void testBigIntegerToByteArray() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also test exceptional cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests

byte[] bytes = new byte[] {23, 47, 126, -42, 34};

assertArrayEquals(new byte[]{0, 0, 0, 23, 47, 126, -42, 34},
Utils.bigIntegerToByteArray(new BigInteger(bytes), 8));

bytes = new byte[] {0, -47, 126, -42, 34};

assertArrayEquals(new byte[]{-47, 126, -42, 34},
Utils.bigIntegerToByteArray(new BigInteger(bytes), 4));

}
}

Loading