Include the frame number in the header.

This ensures the frame number is covered by the MAC, cleanly
separating encryption from authentication (previously we depended on
the encryption layer to garble frames if they were reordered).

api/net/sf/briar/api/transport/TransportConstants.java

@@ -7,6 +7,9 @@ public interface TransportConstants {
 	 */
 	static final int MAX_FRAME_LENGTH = 65536; // 2^16, 64 KiB
 
+	/** The length of the frame header in bytes. */
+	static final int FRAME_HEADER_LENGTH = 8;
+
 	/**
 	 * The length in bytes of the pseudo-random tag that uniquely identifies a
 	 * connection.

components/net/sf/briar/transport/ConnectionReaderImpl.java

@@ -1,5 +1,6 @@
 package net.sf.briar.transport;
 
+import static net.sf.briar.api.transport.TransportConstants.FRAME_HEADER_LENGTH;
 import static net.sf.briar.api.transport.TransportConstants.MAX_FRAME_LENGTH;
 import static net.sf.briar.util.ByteUtils.MAX_32_BIT_UNSIGNED;
 
@@ -11,11 +12,10 @@ import java.security.InvalidKeyException;
 import java.util.Arrays;
 
 import javax.crypto.Mac;
-import net.sf.briar.api.crypto.ErasableKey;
 
 import net.sf.briar.api.FormatException;
+import net.sf.briar.api.crypto.ErasableKey;
 import net.sf.briar.api.transport.ConnectionReader;
-import net.sf.briar.util.ByteUtils;
 
 class ConnectionReaderImpl extends FilterInputStream
 implements ConnectionReader {
@@ -41,8 +41,9 @@ implements ConnectionReader {
 			throw new IllegalArgumentException(e);
 		}
 		macKey.erase();
-		maxPayloadLength = MAX_FRAME_LENGTH - 4 - mac.getMacLength();
-		header = new byte[4];
+		maxPayloadLength =
+			MAX_FRAME_LENGTH - FRAME_HEADER_LENGTH - mac.getMacLength();
+		header = new byte[FRAME_HEADER_LENGTH];
 		payload = new byte[maxPayloadLength];
 		footer = new byte[mac.getMacLength()];
 	}
@@ -81,7 +82,6 @@ implements ConnectionReader {
 		assert betweenFrames;
 		// Don't allow more than 2^32 frames to be read
 		if(frame > MAX_32_BIT_UNSIGNED) throw new IllegalStateException();
-		frame++;
 		// Read the header
 		int offset = 0;
 		while(offset < header.length) {
@@ -91,13 +91,12 @@ implements ConnectionReader {
 		}
 		if(offset == 0) return false; // EOF between frames
 		if(offset < header.length) throw new EOFException(); // Unexpected EOF
-		mac.update(header);
-		// Check that the payload and padding lengths are legal
-		payloadLen = ByteUtils.readUint16(header, 0);
-		int paddingLen = ByteUtils.readUint16(header, 2);
-		if(payloadLen + paddingLen == 0) throw new FormatException();
-		if(payloadLen + paddingLen > maxPayloadLength)
+		// Check that the frame number is correct and the length is legal
+		if(!HeaderEncoder.validateHeader(header, frame, maxPayloadLength))
 			throw new FormatException();
+		payloadLen = HeaderEncoder.getPayloadLength(header);
+		int paddingLen = HeaderEncoder.getPaddingLength(header);
+		mac.update(header);
 		// Read the payload
 		offset = 0;
 		while(offset < payloadLen) {
@@ -124,6 +123,7 @@ implements ConnectionReader {
 		decrypter.readMac(footer);
 		if(!Arrays.equals(expectedMac, footer)) throw new FormatException();
 		betweenFrames = false;
+		frame++;
 		return true;
 	}
 }

components/net/sf/briar/transport/ConnectionWriterImpl.java

@@ -1,5 +1,6 @@
 package net.sf.briar.transport;
 
+import static net.sf.briar.api.transport.TransportConstants.FRAME_HEADER_LENGTH;
 import static net.sf.briar.api.transport.TransportConstants.MAX_FRAME_LENGTH;
 import static net.sf.briar.util.ByteUtils.MAX_32_BIT_UNSIGNED;
 
@@ -10,14 +11,15 @@ import java.io.OutputStream;
 import java.security.InvalidKeyException;
 
 import javax.crypto.Mac;
-import net.sf.briar.api.crypto.ErasableKey;
 
+import net.sf.briar.api.crypto.ErasableKey;
 import net.sf.briar.api.transport.ConnectionWriter;
-import net.sf.briar.util.ByteUtils;
 
 /**
  * A ConnectionWriter that buffers its input and writes a frame whenever there
  * is a full-size frame to write or the flush() method is called.
+ * <p>
+ * This class is not thread-safe.
  */
 class ConnectionWriterImpl extends FilterOutputStream
 implements ConnectionWriter {
@@ -42,9 +44,10 @@ implements ConnectionWriter {
 			throw new IllegalArgumentException(badKey);
 		}
 		macKey.erase();
-		maxPayloadLength = MAX_FRAME_LENGTH - 4 - mac.getMacLength();
+		maxPayloadLength =
+			MAX_FRAME_LENGTH - FRAME_HEADER_LENGTH - mac.getMacLength();
 		buf = new ByteArrayOutputStream(maxPayloadLength);
-		header = new byte[4];
+		header = new byte[FRAME_HEADER_LENGTH];
 	}
 
 	public OutputStream getOutputStream() {
@@ -95,7 +98,7 @@ implements ConnectionWriter {
 		if(frame > MAX_32_BIT_UNSIGNED) throw new IllegalStateException();
 		byte[] payload = buf.toByteArray();
 		if(payload.length > maxPayloadLength) throw new IllegalStateException();
-		ByteUtils.writeUint16(payload.length, header, 0);
+		HeaderEncoder.encodeHeader(header, frame, payload.length, 0);
 		out.write(header);
 		mac.update(header);
 		out.write(payload);

components/net/sf/briar/transport/HeaderEncoder.java

@@ -0,0 +1,45 @@
+package net.sf.briar.transport;
+
+import net.sf.briar.api.transport.TransportConstants;
+import net.sf.briar.util.ByteUtils;
+
+class HeaderEncoder {
+
+	static void encodeHeader(byte[] header, long frame, int payload,
+			int padding) {
+		if(header.length < TransportConstants.FRAME_HEADER_LENGTH)
+			throw new IllegalArgumentException();
+		if(frame < 0 || frame > ByteUtils.MAX_32_BIT_UNSIGNED)
+			throw new IllegalArgumentException();
+		if(payload < 0 || payload > ByteUtils.MAX_16_BIT_UNSIGNED)
+			throw new IllegalArgumentException();
+		if(padding < 0 || padding > ByteUtils.MAX_16_BIT_UNSIGNED)
+			throw new IllegalArgumentException();
+		ByteUtils.writeUint32(frame, header, 0);
+		ByteUtils.writeUint16(payload, header, 4);
+		ByteUtils.writeUint16(padding, header, 6);
+	}
+
+	static boolean validateHeader(byte[] header, long frame, int max) {
+		if(header.length < TransportConstants.FRAME_HEADER_LENGTH)
+			return false;
+		if(ByteUtils.readUint32(header, 0) != frame) return false;
+		int payload = ByteUtils.readUint16(header, 4);
+		int padding = ByteUtils.readUint16(header, 6);
+		if(payload + padding == 0) return false;
+		if(payload + padding > max) return false;
+		return true;
+	}
+
+	static int getPayloadLength(byte[] header) {
+		if(header.length < TransportConstants.FRAME_HEADER_LENGTH)
+			throw new IllegalArgumentException();
+		return ByteUtils.readUint16(header, 4);
+	}
+
+	static int getPaddingLength(byte[] header) {
+		if(header.length < TransportConstants.FRAME_HEADER_LENGTH)
+			throw new IllegalArgumentException();
+		return ByteUtils.readUint16(header, 6);
+	}
+}

components/net/sf/briar/transport/IvEncoder.java

@@ -5,7 +5,7 @@ import net.sf.briar.util.ByteUtils;
 class IvEncoder {
 
 	static byte[] encodeIv(long frame, int blockSize) {
-		if(frame > ByteUtils.MAX_32_BIT_UNSIGNED)
+		if(frame < 0 || frame > ByteUtils.MAX_32_BIT_UNSIGNED)
 			throw new IllegalArgumentException();
 		byte[] iv = new byte[blockSize];
 		updateIv(iv, frame);

components/net/sf/briar/transport/TagEncoder.java

@@ -12,7 +12,7 @@ import net.sf.briar.util.ByteUtils;
 class TagEncoder {
 
 	static byte[] encodeTag(long frame, Cipher tagCipher, ErasableKey tagKey) {
-		if(frame > ByteUtils.MAX_32_BIT_UNSIGNED)
+		if(frame < 0 || frame > ByteUtils.MAX_32_BIT_UNSIGNED)
 			throw new IllegalArgumentException();
 		// The plaintext is blank
 		byte[] plaintext = new byte[TransportConstants.TAG_LENGTH];
@@ -32,6 +32,8 @@ class TagEncoder {
 
 	static boolean validateTag(byte[] tag, long frame, Cipher tagCipher,
 			ErasableKey tagKey) {
+		if(frame < 0 || frame > ByteUtils.MAX_32_BIT_UNSIGNED)
+			throw new IllegalArgumentException();
 		if(tag.length != TransportConstants.TAG_LENGTH) return false;
 		// Encode the frame number as a uint32 at the end of the IV
 		byte[] iv = new byte[tagCipher.getBlockSize()];

test/net/sf/briar/transport/ConnectionReaderImplTest.java

@@ -1,5 +1,6 @@
 package net.sf.briar.transport;
 
+import static net.sf.briar.api.transport.TransportConstants.FRAME_HEADER_LENGTH;
 import static net.sf.briar.api.transport.TransportConstants.MAX_FRAME_LENGTH;
 import static org.junit.Assert.assertArrayEquals;
 
@@ -21,12 +22,13 @@ public class ConnectionReaderImplTest extends TransportTest {
 	@Test
 	public void testLengthZero() throws Exception {
 		int payloadLength = 0;
-		byte[] frame = new byte[headerLength + payloadLength + macLength];
-		writeHeader(frame, payloadLength, 0);
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		// Calculate the MAC
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + payloadLength);
-		mac.doFinal(frame, headerLength + payloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Read the frame
 		ByteArrayInputStream in = new ByteArrayInputStream(frame);
 		ConnectionDecrypter d = new NullConnectionDecrypter(in);
@@ -40,12 +42,13 @@ public class ConnectionReaderImplTest extends TransportTest {
 	@Test
 	public void testLengthOne() throws Exception {
 		int payloadLength = 1;
-		byte[] frame = new byte[headerLength + payloadLength + macLength];
-		writeHeader(frame, payloadLength, 0);
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		// Calculate the MAC
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + payloadLength);
-		mac.doFinal(frame, headerLength + payloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Read the frame
 		ByteArrayInputStream in = new ByteArrayInputStream(frame);
 		ConnectionDecrypter d = new NullConnectionDecrypter(in);
@@ -59,15 +62,15 @@ public class ConnectionReaderImplTest extends TransportTest {
 	public void testMaxLength() throws Exception {
 		// First frame: max payload length
 		byte[] frame = new byte[MAX_FRAME_LENGTH];
-		writeHeader(frame, maxPayloadLength, 0);
+		HeaderEncoder.encodeHeader(frame, 0, maxPayloadLength, 0);
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + maxPayloadLength);
-		mac.doFinal(frame, headerLength + maxPayloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + maxPayloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + maxPayloadLength);
 		// Second frame: max payload length plus one
 		byte[] frame1 = new byte[MAX_FRAME_LENGTH + 1];
-		writeHeader(frame1, maxPayloadLength + 1, 0);
-		mac.update(frame1, 0, headerLength + maxPayloadLength + 1);
-		mac.doFinal(frame1, headerLength + maxPayloadLength + 1);
+		HeaderEncoder.encodeHeader(frame1, 1, maxPayloadLength + 1, 0);
+		mac.update(frame1, 0, FRAME_HEADER_LENGTH + maxPayloadLength + 1);
+		mac.doFinal(frame1, FRAME_HEADER_LENGTH + maxPayloadLength + 1);
 		// Concatenate the frames
 		ByteArrayOutputStream out = new ByteArrayOutputStream();
 		out.write(frame);
@@ -91,16 +94,17 @@ public class ConnectionReaderImplTest extends TransportTest {
 		int paddingLength = 10;
 		// First frame: max payload length, including padding
 		byte[] frame = new byte[MAX_FRAME_LENGTH];
-		writeHeader(frame, maxPayloadLength - paddingLength, paddingLength);
+		HeaderEncoder.encodeHeader(frame, 0, maxPayloadLength - paddingLength,
+				paddingLength);
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + maxPayloadLength);
-		mac.doFinal(frame, headerLength + maxPayloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + maxPayloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + maxPayloadLength);
 		// Second frame: max payload length plus one, including padding
 		byte[] frame1 = new byte[MAX_FRAME_LENGTH + 1];
-		writeHeader(frame1, maxPayloadLength + 1 - paddingLength,
-				paddingLength);
-		mac.update(frame1, 0, headerLength + maxPayloadLength + 1);
-		mac.doFinal(frame1, headerLength + maxPayloadLength + 1);
+		HeaderEncoder.encodeHeader(frame1, 1,
+				maxPayloadLength + 1 - paddingLength, paddingLength);
+		mac.update(frame1, 0, FRAME_HEADER_LENGTH + maxPayloadLength + 1);
+		mac.doFinal(frame1, FRAME_HEADER_LENGTH + maxPayloadLength + 1);
 		// Concatenate the frames
 		ByteArrayOutputStream out = new ByteArrayOutputStream();
 		out.write(frame);
@@ -122,16 +126,20 @@ public class ConnectionReaderImplTest extends TransportTest {
 	@Test
 	public void testMultipleFrames() throws Exception {
 		// First frame: 123-byte payload
-		byte[] frame = new byte[headerLength + 123 + mac.getMacLength()];
-		writeHeader(frame, 123, 0);
+		int payloadLength = 123;
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + 123);
-		mac.doFinal(frame, headerLength + 123);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Second frame: 1234-byte payload
-		byte[] frame1 = new byte[headerLength + 1234 + mac.getMacLength()];
-		writeHeader(frame1, 1234, 0);
-		mac.update(frame1, 0, headerLength + 1234);
-		mac.doFinal(frame1, headerLength + 1234);
+		int payloadLength1 = 1234;
+		byte[] frame1 = new byte[FRAME_HEADER_LENGTH + payloadLength1
+		                         + macLength];
+		HeaderEncoder.encodeHeader(frame1, 1, payloadLength1, 0);
+		mac.update(frame1, 0, FRAME_HEADER_LENGTH + payloadLength1);
+		mac.doFinal(frame1, FRAME_HEADER_LENGTH + payloadLength1);
 		// Concatenate the frames
 		ByteArrayOutputStream out = new ByteArrayOutputStream();
 		out.write(frame);
@@ -140,23 +148,24 @@ public class ConnectionReaderImplTest extends TransportTest {
 		ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
 		ConnectionDecrypter d = new NullConnectionDecrypter(in);
 		ConnectionReader r = new ConnectionReaderImpl(d, mac, macKey);
-		byte[] read = new byte[123];
+		byte[] read = new byte[payloadLength];
 		TestUtils.readFully(r.getInputStream(), read);
-		assertArrayEquals(new byte[123], read);
-		byte[] read1 = new byte[1234];
+		assertArrayEquals(new byte[payloadLength], read);
+		byte[] read1 = new byte[payloadLength1];
 		TestUtils.readFully(r.getInputStream(), read1);
-		assertArrayEquals(new byte[1234], read1);
+		assertArrayEquals(new byte[payloadLength1], read1);
 	}
 
 	@Test
 	public void testCorruptPayload() throws Exception {
 		int payloadLength = 8;
-		byte[] frame = new byte[headerLength + payloadLength + macLength];
-		writeHeader(frame, payloadLength, 0);
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		// Calculate the MAC
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + payloadLength);
-		mac.doFinal(frame, headerLength + payloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Modify the payload
 		frame[12] ^= 1;
 		// Try to read the frame - not a single byte should be read
@@ -172,12 +181,13 @@ public class ConnectionReaderImplTest extends TransportTest {
 	@Test
 	public void testCorruptMac() throws Exception {
 		int payloadLength = 8;
-		byte[] frame = new byte[headerLength + payloadLength + macLength];
-		writeHeader(frame, payloadLength, 0);
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		// Calculate the MAC
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + payloadLength);
-		mac.doFinal(frame, headerLength + payloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Modify the MAC
 		frame[17] ^= 1;
 		// Try to read the frame - not a single byte should be read

test/net/sf/briar/transport/ConnectionWriterImplTest.java

@@ -1,5 +1,6 @@
 package net.sf.briar.transport;
 
+import static net.sf.briar.api.transport.TransportConstants.FRAME_HEADER_LENGTH;
 import static net.sf.briar.api.transport.TransportConstants.MAX_FRAME_LENGTH;
 import static org.junit.Assert.assertArrayEquals;
 
@@ -30,12 +31,13 @@ public class ConnectionWriterImplTest extends TransportTest {
 	@Test
 	public void testSingleByteFrame() throws Exception {
 		int payloadLength = 1;
-		byte[] frame = new byte[headerLength + payloadLength + macLength];
-		writeHeader(frame, payloadLength, 0);
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		// Calculate the MAC
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + payloadLength);
-		mac.doFinal(frame, headerLength + payloadLength);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Check that the ConnectionWriter gets the same results
 		ByteArrayOutputStream out = new ByteArrayOutputStream();
 		ConnectionEncrypter e = new NullConnectionEncrypter(out);
@@ -76,16 +78,20 @@ public class ConnectionWriterImplTest extends TransportTest {
 	@Test
 	public void testMultipleFrames() throws Exception {
 		// First frame: 123-byte payload
-		byte[] frame = new byte[headerLength + 123 + macLength];
-		writeHeader(frame, 123, 0);
+		int payloadLength = 123;
+		byte[] frame = new byte[FRAME_HEADER_LENGTH + payloadLength
+		                        + macLength];
+		HeaderEncoder.encodeHeader(frame, 0, payloadLength, 0);
 		mac.init(macKey);
-		mac.update(frame, 0, headerLength + 123);
-		mac.doFinal(frame, headerLength + 123);
+		mac.update(frame, 0, FRAME_HEADER_LENGTH + payloadLength);
+		mac.doFinal(frame, FRAME_HEADER_LENGTH + payloadLength);
 		// Second frame: 1234-byte payload
-		byte[] frame1 = new byte[headerLength + 1234 + macLength];
-		writeHeader(frame1, 1234, 0);
-		mac.update(frame1, 0, headerLength + 1234);
-		mac.doFinal(frame1, headerLength + 1234);
+		int payloadLength1 = 1234;
+		byte[] frame1 = new byte[FRAME_HEADER_LENGTH + payloadLength1
+		                         + macLength];
+		HeaderEncoder.encodeHeader(frame1, 1, payloadLength1, 0);
+		mac.update(frame1, 0, FRAME_HEADER_LENGTH + 1234);
+		mac.doFinal(frame1, FRAME_HEADER_LENGTH + 1234);
 		// Concatenate the frames
 		ByteArrayOutputStream out = new ByteArrayOutputStream();
 		out.write(frame);

test/net/sf/briar/transport/TransportTest.java

@@ -1,14 +1,14 @@
 package net.sf.briar.transport;
 
+import static net.sf.briar.api.transport.TransportConstants.FRAME_HEADER_LENGTH;
 import static net.sf.briar.api.transport.TransportConstants.MAX_FRAME_LENGTH;
 
 import javax.crypto.Mac;
-import net.sf.briar.api.crypto.ErasableKey;
 
 import junit.framework.TestCase;
 import net.sf.briar.api.crypto.CryptoComponent;
+import net.sf.briar.api.crypto.ErasableKey;
 import net.sf.briar.crypto.CryptoModule;
-import net.sf.briar.util.ByteUtils;
 
 import com.google.inject.Guice;
 import com.google.inject.Injector;
@@ -17,7 +17,7 @@ public abstract class TransportTest extends TestCase {
 
 	protected final Mac mac;
 	protected final ErasableKey macKey;
-	protected final int headerLength = 4, macLength, maxPayloadLength;
+	protected final int macLength, maxPayloadLength;
 
 	public TransportTest() throws Exception {
 		super();
@@ -26,11 +26,6 @@ public abstract class TransportTest extends TestCase {
 		mac = crypto.getMac();
 		macKey = crypto.generateTestKey();
 		macLength = mac.getMacLength();
-		maxPayloadLength = MAX_FRAME_LENGTH - headerLength - macLength;
-	}
-
-	static void writeHeader(byte[] b, int payload, int padding) {
-		ByteUtils.writeUint16(payload, b, 0);
-		ByteUtils.writeUint16(padding, b, 2);
+		maxPayloadLength = MAX_FRAME_LENGTH - FRAME_HEADER_LENGTH - macLength;
 	}
 }