fixed findings from code review

[deploy]

Author: overheadhunter

CHANGELOG.md

@@ -8,32 +8,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased](https://github.com/cryptomator/siv-mode/compare/1.6.1...HEAD)
 
 ### Added
-- new lowlevel API:
-   * `new SivEngine(key).encrypt(plaintext, associatedData...)`
-   * `new SivEngine(key).decrypt(plaintext, associatedData...)`
+- new low-level API:
+  * `new SivEngine(key).encrypt(plaintext, associatedData...)`
+  * `new SivEngine(key).decrypt(plaintext, associatedData...)`
 - implement JCA `Cipher` SPI:
-    ```
-    Cipher siv = Cipher.getInstance("AES/SIV/NoPadding");
-    siv.init(Cipher.ENCRYPT_MODE, key);
-    siv.updateAAD(aad1);
-    siv.updateAAD(aad2);
-    byte[] ciphertext = siv.doFinal(plaintext);
-    ```
-  
+  ```java
+  Cipher siv = Cipher.getInstance("AES/SIV/NoPadding");
+  siv.init(Cipher.ENCRYPT_MODE, key);
+  siv.updateAAD(aad1);
+  siv.updateAAD(aad2);
+  byte[] ciphertext = siv.doFinal(plaintext);
+  ```
+
 ### Changed
 - remove dependencies on BouncyCastle and Jetbrains Annotations
 - simplify build by removing `maven-shade-plugin`
 - update test dependencies
 - update build plugins
 
 ### Deprecated
-- old lowlevel API:
-   * `new SivMode().encrypt(key, plaintext, associatedData...)`
-   * `new SivMode().encrypt(ctrKey, macKey, plaintext, associatedData...)`
-   * `new SivMode().decrypt(key, ciphertext, associatedData...)`
-   * `new SivMode().decrypt(ctrKey, macKey, ciphertext, associatedData...)`
-  
-
+- old low-level API:
+  * `new SivMode().encrypt(key, plaintext, associatedData...)`
+  * `new SivMode().encrypt(ctrKey, macKey, plaintext, associatedData...)`
+  * `new SivMode().decrypt(key, ciphertext, associatedData...)`
+  * `new SivMode().decrypt(ctrKey, macKey, ciphertext, associatedData...)`
 
 ## [1.6.1](https://github.com/cryptomator/siv-mode/compare/1.6.0...1.6.1)
 

README.md

@@ -11,7 +11,6 @@
 - No dependencies
 - Passes official RFC 5297 test vectors
 - Constant time authentication
-- Thread-safe
 - [Fast](https://github.com/cryptomator/siv-mode/issues/15)
 - Requires JDK 8+ or Android API Level 24+ (since version 1.4.0)
 

src/main/java/org/cryptomator/siv/CMac.java

@@ -58,11 +58,14 @@ protected void engineInit(Key key, AlgorithmParameterSpec params) throws Invalid
 		try {
 			// L = AES_encrypt(K, const_Zero)
 			encryptBlock(cipher, L, L);
-			this.k1 = dbl(L).clone();
-			this.k2 = dbl(L).clone();
+			this.k1 = dbl(L.clone());
+			this.k2 = dbl(k1.clone());
 		} finally {
 			Arrays.fill(L, (byte) 0);
 		}
+
+		// reset state
+		engineReset();
 	}
 
 	@Override

src/main/java/org/cryptomator/siv/SivCipher.java

@@ -123,34 +123,54 @@ protected int engineUpdate(byte[] input, int inputOffset, int inputLen, byte[] o
 
 	@Override
 	protected byte[] engineDoFinal(byte[] input, int inputOffset, int inputLen) throws IllegalBlockSizeException, BadPaddingException {
-		byte[] output = new byte[engineGetOutputSize(inputBuffer.length + inputLen)];
+		int outputSize = engineGetOutputSize(inputBuffer.length + inputLen);
+		if (outputSize < 0) {
+			throw new IllegalBlockSizeException("Ciphertext too short (must be at least 16 bytes including SIV tag)");
+		}
+		byte[] output = new byte[outputSize];
 		try {
 			engineDoFinal(input, inputOffset, inputLen, output, 0);
 		} catch (ShortBufferException e) {
+			// outputSize was calculated before, so this should never happen
 			throw new IllegalStateException(e);
 		}
 		return output;
 	}
 
 	@Override
 	protected int engineDoFinal(byte[] input, int inputOffset, int inputLen, byte[] output, int outputOffset) throws ShortBufferException, IllegalBlockSizeException, BadPaddingException {
+		int outputSize = engineGetOutputSize(inputBuffer.length + inputLen);
+		if (outputSize < 0) {
+			throw new IllegalBlockSizeException("Ciphertext too short (must be at least 16 bytes including SIV tag)");
+		}
 		int availableSpace = output.length - outputOffset;
-		if (availableSpace < engineGetOutputSize(inputBuffer.length + inputLen)) {
+		if (availableSpace < outputSize) {
 			throw new ShortBufferException();
 		}
 		engineUpdate(input, inputOffset, inputLen);
 
+		int resultLength;
 		SivEngine siv = new SivEngine(this.key);
 		byte[][] aad = this.aad.toArray(new byte[this.aad.size()][]);
 		if (this.opmode == Cipher.ENCRYPT_MODE || this.opmode == Cipher.WRAP_MODE) {
-			return siv.encrypt(inputBuffer, output, outputOffset, aad);
+			resultLength = siv.encrypt(inputBuffer, output, outputOffset, aad);
 		} else if (this.opmode == Cipher.DECRYPT_MODE || this.opmode == Cipher.UNWRAP_MODE) {
 			// for security reasons we can't write into output directly before checking integrity:
-			byte[] plaintext = siv.decrypt(inputBuffer, aad);
-			System.arraycopy(plaintext, 0, output, outputOffset, plaintext.length);
-			return plaintext.length;
+			byte[] plaintext = new byte[0];
+			try {
+				plaintext = siv.decrypt(inputBuffer, aad);
+				System.arraycopy(plaintext, 0, output, outputOffset, plaintext.length);
+				resultLength = plaintext.length;
+			} finally {
+				Arrays.fill(plaintext, (byte) 0x00);
+			}
 		} else {
 			throw new IllegalStateException("Invalid opmode " + this.opmode);
 		}
+
+		// reset internal state:
+		this.inputBuffer = EMPTY;
+		this.aad.clear();
+		return resultLength;
 	}
 }

src/main/java/org/cryptomator/siv/SivEngine.java

@@ -113,8 +113,8 @@ public int encrypt(byte[] plaintext, byte[] output, int outputOffset, byte[]...
 		}
 		byte[] iv = s2v(plaintext, associatedData);
 		assert iv.length == IV_LENGTH;
-		System.arraycopy(iv, 0, output, 0, IV_LENGTH);
-		return IV_LENGTH + computeCtr(plaintext, 0, plaintext.length, iv, 0, IV_LENGTH, output, IV_LENGTH);
+		System.arraycopy(iv, 0, output, outputOffset, IV_LENGTH);
+		return IV_LENGTH + computeCtr(plaintext, 0, plaintext.length, iv, 0, IV_LENGTH, output, outputOffset + IV_LENGTH);
 	}
 
 	/**
@@ -213,7 +213,7 @@ byte[] s2v(byte[] plaintext, byte[]... associatedData) throws IllegalArgumentExc
 			return cmac.doFinal(end);
 		} else {
 			// T = dbl(D) xor pad(Sn)
-			byte[] t = xor(dbl(d), pad(plaintext));
+			byte[] t = xor(dbl(d), pad(plaintext, 16));
 			return cmac.doFinal(t);
 		}
 	}

src/main/java/org/cryptomator/siv/Utils.java

@@ -12,8 +12,8 @@ public class Utils {
 	private static final byte DOUBLING_CONST = (byte) 0x87;
 
 	// First bit 1, following bits 0.
-	static byte[] pad(byte[] in) {
-		final byte[] result = Arrays.copyOf(in, 16);
+	static byte[] pad(byte[] in, int desiredLength) {
+		final byte[] result = Arrays.copyOf(in, desiredLength);
 		result[in.length] = (byte) 0x80;
 		return result;
 	}

src/test/java/org/cryptomator/siv/CMacTest.java

@@ -1,11 +1,15 @@
 package org.cryptomator.siv;
 
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.converter.ArgumentConversionException;
 import org.junit.jupiter.params.converter.ConvertWith;
 import org.junit.jupiter.params.converter.SimpleArgumentConverter;
 import org.junit.jupiter.params.provider.CsvSource;
 
+import java.nio.charset.StandardCharsets;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 class CMacTest {
@@ -22,19 +26,26 @@ protected Object convert(Object source, Class<?> targetType) throws ArgumentConv
 				throw new ArgumentConversionException("Source must be a String");
 			}
 			String hex = (String) source;
-			if (hex.isEmpty()) {
-				return new byte[0];
-			}
-			int len = hex.length();
-			byte[] data = new byte[len / 2];
-			for (int i = 0; i < len; i += 2) {
-				data[i / 2] = (byte) ((Character.digit(hex.charAt(i), 16) << 4)
-						+ Character.digit(hex.charAt(i + 1), 16));
-			}
-			return data;
+			return hexToBytes(hex);
 		}
 	}
 
+	/**
+	 * Convert hex string to byte array
+	 */
+	private static byte[] hexToBytes(String hex) {
+		if (hex.isEmpty()) {
+			return new byte[0];
+		}
+		int len = hex.length();
+		byte[] data = new byte[len / 2];
+		for (int i = 0; i < len; i += 2) {
+			data[i / 2] = (byte) ((Character.digit(hex.charAt(i), 16) << 4)
+					+ Character.digit(hex.charAt(i + 1), 16));
+		}
+		return data;
+	}
+
 	/**
 	 * Convert byte array to hex string
 	 */
@@ -84,4 +95,19 @@ void testNistVectors(String testName,
 		assertEquals(expectedHex, bytesToHex(result), testName + " failed");
 	}
 
+	@Test
+	public void testReset() {
+		byte[] key = hexToBytes("2b7e151628aed2a6abf7158809cf4f3c");
+		byte[] incorrectInput = "oops, never ment to update with this data!".getBytes(StandardCharsets.UTF_8);
+		byte[] correctInput = hexToBytes("6bc1bee22e409f96e93d7e117393172a");
+
+		CMac cmac = CMac.create(key);
+		cmac.engineUpdate(incorrectInput, 0, incorrectInput.length);
+		cmac.engineReset();
+		cmac.engineUpdate(correctInput, 0, correctInput.length);
+		byte[] tag = cmac.engineDoFinal();
+
+		assertEquals("070a16b46b4d4144f79bdd9dd04a287c", bytesToHex(tag));
+	}
+
 }

src/test/java/org/cryptomator/siv/SivCipherTest.java

@@ -78,7 +78,7 @@ public void testEngineInitWithValidKeySize(int keysize) {
 	@ValueSource(ints = {16, 24, 1337})
 	public void testEngineInitWithInvalidKeySize(int keysize) {
 		SecretKeySpec key = new SecretKeySpec(new byte[keysize], "AES");
-		Assertions.assertThrows(InvalidKeyException.class,() -> cipher.engineInit(Cipher.ENCRYPT_MODE, key, null));
+		Assertions.assertThrows(InvalidKeyException.class, () -> cipher.engineInit(Cipher.ENCRYPT_MODE, key, null));
 	}
 
 	@Test
@@ -132,4 +132,19 @@ public void testWrapAndUnwrap() throws InvalidKeyException, ShortBufferException
 		// compare:
 		Assertions.assertEquals("helloworld!", new String(unwrapped, StandardCharsets.UTF_8));
 	}
+
+	@Test
+	public void testReuseCipher() throws InvalidKeyException, IllegalBlockSizeException, BadPaddingException {
+		byte[] plaintext = "helloworld".getBytes(StandardCharsets.UTF_8);
+		cipher.engineInit(Cipher.ENCRYPT_MODE, key, null);
+		byte[] encrypted1 = cipher.engineDoFinal(plaintext, 0, plaintext.length);
+		byte[] encrypted2 = cipher.engineDoFinal(plaintext, 0, plaintext.length);
+		cipher.engineInit(Cipher.DECRYPT_MODE, key, null);
+		byte[] decrypted1 = cipher.engineDoFinal(encrypted1, 0, encrypted1.length);
+		byte[] decrypted2 = cipher.engineDoFinal(encrypted2, 0, encrypted2.length);
+
+		Assertions.assertArrayEquals(encrypted1, encrypted2);
+		Assertions.assertArrayEquals(plaintext, decrypted1);
+		Assertions.assertArrayEquals(plaintext, decrypted2);
+	}
 }

src/test/java/org/cryptomator/siv/SivEngineTest.java

@@ -169,6 +169,14 @@ public void testSivEncrypt() {
 			Assertions.assertArrayEquals(ciphertext, result);
 		}
 
+		@Test
+		public void testSivEncryptWithOffset() throws ShortBufferException {
+			final byte[] result = new byte[ciphertext.length + 10];
+			int encrypted = new SivEngine(key).encrypt(plaintext, result, 10, ad);
+			Assertions.assertEquals(ciphertext.length, encrypted);
+			Assertions.assertArrayEquals(ciphertext, Arrays.copyOfRange(result, 10, result.length));
+		}
+
 		@Test
 		public void testSivDecrypt() throws AEADBadTagException, IllegalBlockSizeException {
 			final byte[] result = new SivEngine(key).decrypt(ciphertext, ad);