Skip to content

Commit

Permalink
Explicitly reconstruct the public key using private's key curve.
Browse files Browse the repository at this point in the history 
Make the test failed with modified public key.

Change-Id: I638ba24ca03c162d20a5dce07938f6a4c1fcfbe8
ORIGINAL_AUTHOR=Quan Nguyen <[email protected]>
GitOrigin-RevId: c68cb8eb63ffee64f02d86ed0715c64bb200f5c1
  • Loading branch information
@cryptosubtlety @thaidn
cryptosubtlety authored and thaidn committed Oct 26, 2017
1 parent 5d68cda commit 89146ca
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 31 deletions.
43 changes: 26 additions & 17 deletions java/src/main/java/com/google/crypto/tink/subtle/EllipticCurves.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.NoSuchAlgorithmException;
import java.security.PublicKey;
import java.security.interfaces.ECPrivateKey;
import java.security.interfaces.ECPublicKey;
import java.security.spec.ECFieldFp;
Expand Down Expand Up @@ -93,11 +94,11 @@ public static ECParameterSpec getNistP521Params() {
/**
* Checks that a point is on a given elliptic curve.
*
* <p>><b>Warning:</b> Please use {@link #validatePublicKey} if you want to validate a public
* key to avoid invalid curve attacks or small subgroup attacks in ECDH.
* <p>><b>Warning:</b> Please use {@link #validatePublicKey} if you want to validate a public key
* to avoid invalid curve attacks or small subgroup attacks in ECDH.
*
* <p>This method implements the partial public key validation routine from Section 5.6.2.6 of
* <a href="http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf">NIST SP
* <p>This method implements the partial public key validation routine from Section 5.6.2.6 of <a
* href="http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf">NIST SP
* 800-56A</a>. A partial public key validation is sufficient for curves with cofactor 1. See
* Section B.3 of http://www.nsa.gov/ia/_files/SuiteB_Implementer_G-113808.pdf.
*
Expand All @@ -109,8 +110,7 @@ public static ECParameterSpec getNistP521Params() {
* @param ec the elliptic curve. This must be a curve over a prime order field.
* @throws GeneralSecurityException if the field is binary or if the point is not on the curve.
*/
static void checkPointOnCurve(ECPoint point, EllipticCurve ec)
throws GeneralSecurityException {
static void checkPointOnCurve(ECPoint point, EllipticCurve ec) throws GeneralSecurityException {
BigInteger p = getModulus(ec);
BigInteger x = point.getAffineX();
BigInteger y = point.getAffineY();
Expand All @@ -133,8 +133,8 @@ static void checkPointOnCurve(ECPoint point, EllipticCurve ec)
}

/**
* Checks that the public key's params is the same as the private key's params, and
* the public key is a valid point on the private key's curve.
* Checks that the public key's params is the same as the private key's params, and the public key
* is a valid point on the private key's curve.
*/
public static void validatePublicKey(ECPublicKey publicKey, ECPrivateKey privateKey)
throws GeneralSecurityException {
Expand Down Expand Up @@ -458,17 +458,19 @@ public static ECParameterSpec getCurveSpec(CurveType curve) throws NoSuchAlgorit
* Returns an {@link ECPublicKey} from {@code publicKey} that is a public key in point format
* {@code pointFormat} on {@code curve}.
*/
public static ECPublicKey getEcPublicKey(CurveType curve, PointFormatType pointFormat,
final byte[] publicKey) throws GeneralSecurityException {
public static ECPublicKey getEcPublicKey(
CurveType curve, PointFormatType pointFormat, final byte[] publicKey)
throws GeneralSecurityException {
return getEcPublicKey(getCurveSpec(curve), pointFormat, publicKey);
}

/**
* Returns an {@link ECPublicKey} from {@code publicKey} that is a public key in point format
* {@code pointFormat} on {@code curve}.
*/
public static ECPublicKey getEcPublicKey(ECParameterSpec spec, PointFormatType pointFormat,
final byte[] publicKey) throws GeneralSecurityException {
public static ECPublicKey getEcPublicKey(
ECParameterSpec spec, PointFormatType pointFormat, final byte[] publicKey)
throws GeneralSecurityException {
ECPoint point = ecPointDecode(spec.getCurve(), pointFormat, publicKey);
ECPublicKeySpec pubSpec = new ECPublicKeySpec(point, spec);
KeyFactory kf = EngineFactory.KEY_FACTORY.getInstance("EC");
Expand All @@ -494,8 +496,7 @@ public static ECPublicKey getEcPublicKey(CurveType curve, final byte[] x, final
public static ECPrivateKey getEcPrivateKey(final byte[] pkcs8PrivateKey)
throws GeneralSecurityException {
KeyFactory kf = EngineFactory.KEY_FACTORY.getInstance("EC");
return (ECPrivateKey)
kf.generatePrivate(new PKCS8EncodedKeySpec(pkcs8PrivateKey));
return (ECPrivateKey) kf.generatePrivate(new PKCS8EncodedKeySpec(pkcs8PrivateKey));
}

/** Returns an {@code ECPrivateKey} from {@code curve} type and {@code keyValue}. */
Expand All @@ -521,8 +522,8 @@ public static KeyPair generateKeyPair(ECParameterSpec spec) throws GeneralSecuri
}

/**
* Checks that the shared secret is on the curve of the private key, to prevent
* arithmetic errors or fault attacks.
* Checks that the shared secret is on the curve of the private key, to prevent arithmetic errors
* or fault attacks.
*/
private static void validateSharedSecret(byte[] secret, ECPrivateKey privateKey)
throws GeneralSecurityException {
Expand All @@ -539,9 +540,17 @@ private static void validateSharedSecret(byte[] secret, ECPrivateKey privateKey)
public static byte[] computeSharedSecret(ECPrivateKey myPrivateKey, ECPublicKey peerPublicKey)
throws GeneralSecurityException {
validatePublicKey(peerPublicKey, myPrivateKey);
// Explicitly reconstruct the peer public key using private key's spec. There are 2 reasons:
// - The peer public key's spec is under attacker's control and should be ignored.
// - In case we have a bug in validatePublicKey, this will protect us from attack.
ECParameterSpec privSpec = myPrivateKey.getParams();
EllipticCurve privCurve = privSpec.getCurve();
ECPublicKeySpec reconstructedPubSpec = new ECPublicKeySpec(peerPublicKey.getW(), privSpec);
KeyFactory kf = KeyFactory.getInstance("EC");
PublicKey reconstructedPubKey = kf.generatePublic(reconstructedPubSpec);
KeyAgreement ka = EngineFactory.KEY_AGREEMENT.getInstance("ECDH");
ka.init(myPrivateKey);
ka.doPhase(peerPublicKey, true /* lastPhase */);
ka.doPhase(reconstructedPubKey, true /* lastPhase */);
byte[] secret = ka.generateSecret();
validateSharedSecret(secret, myPrivateKey);
return secret;
Expand Down
18 changes: 4 additions & 14 deletions java/src/test/java/com/google/crypto/tink/subtle/EllipticCurvesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -892,17 +892,12 @@ public void testComputeSharedSecret_ModifiedPublicKeys_shouldFail() throws Excep
keyGen.initialize(EllipticCurves.getNistP256Params());
ECPrivateKey priv = (ECPrivateKey) keyGen.generateKeyPair().getPrivate();
KeyFactory kf = EngineFactory.KEY_FACTORY.getInstance("EC");
ECPublicKey validKey = (ECPublicKey) kf.generatePublic(EC_VALID_PUBLIC_KEY.getSpec());
String expected = Hex.encode(EllipticCurves.computeSharedSecret(priv, validKey));
for (EcPublicKeyTestVector test : EC_MODIFIED_PUBLIC_KEYS) {
try {
X509EncodedKeySpec spec = test.getX509EncodedKeySpec();
ECPublicKey modifiedKey = (ECPublicKey) kf.generatePublic(spec);
String shared = Hex.encode(EllipticCurves.computeSharedSecret(priv, modifiedKey));
// The implementation did not notice that the public key was modified.
// This is not nice, but at the moment we only fail the test if the
// modification was essential for computing the shared secret.
assertEquals("test:" + test.comment, expected, shared);
EllipticCurves.computeSharedSecret(priv, modifiedKey);
fail("Modified public key shouldn't be accepted");
} catch (GeneralSecurityException ex) {
// OK, since the public keys have been modified.
System.out.println(
Expand All @@ -920,8 +915,6 @@ public void testComputeSharedSecret_ModifiedPublicKeySpec_shouldFail() throws Ex
keyGen.initialize(EllipticCurves.getNistP256Params());
ECPrivateKey priv = (ECPrivateKey) keyGen.generateKeyPair().getPrivate();
KeyFactory kf = EngineFactory.KEY_FACTORY.getInstance("EC");
ECPublicKey validKey = (ECPublicKey) kf.generatePublic(EC_VALID_PUBLIC_KEY.getSpec());
String expected = Hex.encode(EllipticCurves.computeSharedSecret(priv, validKey));
for (EcPublicKeyTestVector test : EC_MODIFIED_PUBLIC_KEYS) {
ECPublicKeySpec spec = test.getSpec();
if (spec == null) {
Expand All @@ -931,11 +924,8 @@ public void testComputeSharedSecret_ModifiedPublicKeySpec_shouldFail() throws Ex
}
try {
ECPublicKey modifiedKey = (ECPublicKey) kf.generatePublic(spec);
String shared = Hex.encode(EllipticCurves.computeSharedSecret(priv, modifiedKey));
// The implementation did not notice that the public key was modified.
// This is not nice, but at the moment we only fail the test if the
// modification was essential for computing the shared secret.
assertEquals("test:" + test.comment, expected, shared);
EllipticCurves.computeSharedSecret(priv, modifiedKey);
fail("Modified publicKey shouldn't be accepted");
} catch (GeneralSecurityException ex) {
// OK, since the public keys have been modified.
System.out.println(
Expand Down

0 comments on commit 89146ca

Please sign in to comment.