diff --git a/api/net/sf/briar/api/transport/TransportConstants.java b/api/net/sf/briar/api/transport/TransportConstants.java index d567504b9a9524948d45c6208867c715e5b44ead..dd1c71f9af2117e857d142aadbf27c88a843fb2b 100644 --- a/api/net/sf/briar/api/transport/TransportConstants.java +++ b/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. diff --git a/components/net/sf/briar/transport/ConnectionReaderImpl.java b/components/net/sf/briar/transport/ConnectionReaderImpl.java index 28b37389c3195576143b86c38c363a172c128209..2c07351c4a2c175459f3e511a81e4c55c5aeaed2 100644 --- a/components/net/sf/briar/transport/ConnectionReaderImpl.java +++ b/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; } } diff --git a/components/net/sf/briar/transport/ConnectionWriterImpl.java b/components/net/sf/briar/transport/ConnectionWriterImpl.java index 61359e61525fdb4708b58eba8ff9aebf2c7dede7..4f1c790769f21d1a7e8583adcb926b8f30abc3e5 100644 --- a/components/net/sf/briar/transport/ConnectionWriterImpl.java +++ b/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); diff --git a/components/net/sf/briar/transport/HeaderEncoder.java b/components/net/sf/briar/transport/HeaderEncoder.java new file mode 100644 index 0000000000000000000000000000000000000000..d8a3868b6cae317ef81bc41b9b47c547a82d31df --- /dev/null +++ b/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); + } +} diff --git a/components/net/sf/briar/transport/IvEncoder.java b/components/net/sf/briar/transport/IvEncoder.java index 8475f29d3ee6bb113733bbeeb435593733c08bfa..997db2007c79de3335582c4ac3e2113145e7e3c7 100644 --- a/components/net/sf/briar/transport/IvEncoder.java +++ b/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); diff --git a/components/net/sf/briar/transport/TagEncoder.java b/components/net/sf/briar/transport/TagEncoder.java index d74da3174c3176d85cfd94db4f0221cb16ff05f7..f442c96844c47cc215d8c00a800a0c48831ad234 100644 --- a/components/net/sf/briar/transport/TagEncoder.java +++ b/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()]; diff --git a/test/net/sf/briar/transport/ConnectionReaderImplTest.java b/test/net/sf/briar/transport/ConnectionReaderImplTest.java index 0e6a84850cc19307f5b6c0ad3d483dcff66b5186..c5a87df8efaffc4fac1a55654d53754c716e1435 100644 --- a/test/net/sf/briar/transport/ConnectionReaderImplTest.java +++ b/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 diff --git a/test/net/sf/briar/transport/ConnectionWriterImplTest.java b/test/net/sf/briar/transport/ConnectionWriterImplTest.java index 290a797290a96f10f9173add73898800bf5e2f26..7076deb49eda3933b78954edffa49e2343315e3f 100644 --- a/test/net/sf/briar/transport/ConnectionWriterImplTest.java +++ b/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); diff --git a/test/net/sf/briar/transport/TransportTest.java b/test/net/sf/briar/transport/TransportTest.java index 301ca4bd075a91062218d29a454eba526a9af545..c833b677ff86a4dbfd89f66b6233cc8614427cd2 100644 --- a/test/net/sf/briar/transport/TransportTest.java +++ b/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; } }