Rewrote EC key encoding and added fuzzing tests to track down bug #33.

briar-core/src/net/sf/briar/crypto/Sec1KeyParser.java

@@ -28,7 +28,7 @@ class Sec1KeyParser implements KeyParser {
 		this.params = params;
 		this.modulus = modulus;
 		this.keyBits = keyBits;
-		bytesPerInt = (int) Math.ceil(keyBits / 8.0);
+		bytesPerInt = keyBits + 7 / 8;
 		publicKeyBytes = 1 + 2 * bytesPerInt;
 		privateKeyBytes = bytesPerInt;
 	}
@@ -47,7 +47,7 @@ class Sec1KeyParser implements KeyParser {
 		if(x.compareTo(modulus) >= 0) throw new GeneralSecurityException();
 		// The y co-ordinate must be >= 0 and < q
 		byte[] yBytes = new byte[bytesPerInt];
-		System.arraycopy(encodedKey, bytesPerInt + 1, yBytes, 0, bytesPerInt);
+		System.arraycopy(encodedKey, 1 + bytesPerInt, yBytes, 0, bytesPerInt);
 		BigInteger y = new BigInteger(1, yBytes); // Positive signum
 		if(y.compareTo(modulus) >= 0) throw new GeneralSecurityException();
 		// Verify that y^2 == x^3 + ax + b (mod p)

briar-core/src/net/sf/briar/crypto/Sec1PrivateKey.java

@@ -1,7 +1,5 @@
 package net.sf.briar.crypto;
 
-import java.math.BigInteger;
-
 import net.sf.briar.api.crypto.PrivateKey;
 
 import org.spongycastle.crypto.params.ECPrivateKeyParameters;
@@ -9,20 +7,17 @@ import org.spongycastle.crypto.params.ECPrivateKeyParameters;
 class Sec1PrivateKey implements PrivateKey {
 
 	private final ECPrivateKeyParameters key;
-	private final int privateKeyBytes;
+	private final int bytesPerInt;
 
 	Sec1PrivateKey(ECPrivateKeyParameters key, int keyBits) {
 		this.key = key;
-		privateKeyBytes = (int) Math.ceil(keyBits / 8.0);
+		bytesPerInt = keyBits + 7 / 8;
 	}
 
 	public byte[] getEncoded() {
-		byte[] encodedKey = new byte[privateKeyBytes];
+		byte[] encodedKey = new byte[bytesPerInt];
-		BigInteger d = key.getD();
+		byte[] d = key.getD().toByteArray();
-		// Copy up to privateKeyBytes bytes into exactly privateKeyBytes bytes
+		Sec1Utils.convertToFixedLength(d, encodedKey, bytesPerInt, 0);
-		byte[] dBytes = d.toByteArray();
-		for(int i = 0; i < dBytes.length && i < privateKeyBytes; i++)
-			encodedKey[privateKeyBytes - 1 - i] = dBytes[dBytes.length - 1 - i];
 		return encodedKey;
 	}
 

briar-core/src/net/sf/briar/crypto/Sec1PublicKey.java

@@ -1,7 +1,5 @@
 package net.sf.briar.crypto;
 
-import java.math.BigInteger;
-
 import net.sf.briar.api.crypto.PublicKey;
 
 import org.spongycastle.crypto.params.ECPublicKeyParameters;
@@ -18,22 +16,18 @@ class Sec1PublicKey implements PublicKey {
 
 	Sec1PublicKey(ECPublicKeyParameters key, int keyBits) {
 		this.key = key;
-		bytesPerInt = (int) Math.ceil(keyBits / 8.0);
+		bytesPerInt = keyBits + 7 / 8;
 		publicKeyBytes = 1 + 2 * bytesPerInt;
 	}
 
 	public byte[] getEncoded() {
 		byte[] encodedKey = new byte[publicKeyBytes];
 		encodedKey[0] = 4;
-		BigInteger x = key.getQ().getX().toBigInteger();
+		byte[] x = key.getQ().getX().toBigInteger().toByteArray();
-		BigInteger y = key.getQ().getY().toBigInteger();
+		Sec1Utils.convertToFixedLength(x, encodedKey, bytesPerInt, 1);
-		// Copy up to bytesPerInt bytes into exactly bytesPerInt bytes
+		byte[] y = key.getQ().getY().toBigInteger().toByteArray();
-		byte[] xBytes = x.toByteArray();
+		Sec1Utils.convertToFixedLength(y, encodedKey, bytesPerInt,
-		for(int i = 0; i < xBytes.length && i < bytesPerInt; i++)
+				1 + bytesPerInt);
-			encodedKey[bytesPerInt - i] = xBytes[xBytes.length - 1 - i];
-		byte[] yBytes = y.toByteArray();
-		for(int i = 0; i < yBytes.length && i < bytesPerInt; i++)
-			encodedKey[2 * bytesPerInt - i] = yBytes[yBytes.length - 1 - i];
 		return encodedKey;
 	}
 

briar-core/src/net/sf/briar/crypto/Sec1Utils.java

@@ -0,0 +1,15 @@
+package net.sf.briar.crypto;
+
+class Sec1Utils {
+
+	static void convertToFixedLength(byte[] src, byte[] dest, int destLen,
+			int destOff) {
+		if(src.length < destLen) {
+			destOff += destLen - src.length;
+			System.arraycopy(src, 0, dest, destOff, src.length);
+		} else {
+			int srcOff = src.length - destLen;
+			System.arraycopy(src, srcOff, dest, destOff, destLen);
+		}
+	}
+}

briar-tests/src/net/sf/briar/crypto/KeyEncodingAndParsingTest.java

@@ -1,6 +1,10 @@
 package net.sf.briar.crypto;
 
 import static org.junit.Assert.assertArrayEquals;
+
+import java.security.GeneralSecurityException;
+import java.util.Random;
+
 import net.sf.briar.BriarTestCase;
 import net.sf.briar.api.crypto.KeyPair;
 import net.sf.briar.api.crypto.KeyParser;
@@ -47,6 +51,35 @@ public class KeyEncodingAndParsingTest extends BriarTestCase {
 		assertArrayEquals(secret, secret1);
 	}
 
+	@Test
+	public void testAgreementKeyParserByFuzzing() throws Exception {
+		KeyParser parser = crypto.getAgreementKeyParser();
+		// Generate a key pair to get the proper public key length
+		KeyPair p = crypto.generateAgreementKeyPair();
+		int pubLength = p.getPublic().getEncoded().length;
+		int privLength = p.getPrivate().getEncoded().length;
+		// Parse some random byte arrays - expect GeneralSecurityException
+		Random random = new Random();
+		byte[] pubFuzz = new byte[pubLength];
+		byte[] privFuzz = new byte[privLength];
+		for(int i = 0; i < 1000; i++) {
+			random.nextBytes(pubFuzz);
+			try {
+				parser.parsePublicKey(pubFuzz);
+			} catch(GeneralSecurityException expected) {
+			} catch(Exception e) {
+				fail();
+			}
+			random.nextBytes(privFuzz);
+			try {
+				parser.parsePrivateKey(privFuzz);
+			} catch(GeneralSecurityException expected) {
+			} catch(Exception e) {
+				fail();
+			}
+		}
+	}
+
 	@Test
 	public void testSignaturePublicKeyEncodingAndParsing() throws Exception {
 		KeyParser parser = crypto.getSignatureKeyParser();
@@ -80,4 +113,33 @@ public class KeyEncodingAndParsingTest extends BriarTestCase {
 		byte[] secret1 = crypto.deriveSharedSecret(bPriv, aPair.getPublic());
 		assertArrayEquals(secret, secret1);
 	}
+
+	@Test
+	public void testSignatureKeyParserByFuzzing() throws Exception {
+		KeyParser parser = crypto.getSignatureKeyParser();
+		// Generate a key pair to get the proper public key length
+		KeyPair p = crypto.generateSignatureKeyPair();
+		int pubLength = p.getPublic().getEncoded().length;
+		int privLength = p.getPrivate().getEncoded().length;
+		// Parse some random byte arrays - expect GeneralSecurityException
+		Random random = new Random();
+		byte[] pubFuzz = new byte[pubLength];
+		byte[] privFuzz = new byte[privLength];
+		for(int i = 0; i < 1000; i++) {
+			random.nextBytes(pubFuzz);
+			try {
+				parser.parsePublicKey(pubFuzz);
+			} catch(GeneralSecurityException expected) {
+			} catch(Exception e) {
+				fail();
+			}
+			random.nextBytes(privFuzz);
+			try {
+				parser.parsePrivateKey(privFuzz);
+			} catch(GeneralSecurityException expected) {
+			} catch(Exception e) {
+				fail();
+			}
+		}
+	}
 }