CVE-2020-25021: Out-of-bounds Read (CWE-125)
value: An issue was discovered in Noise-Java through 2020-08-27. ChaChaPolyCipherState.encryptWithAd() allows out-of-bounds access.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2020-09-04T04:15Z
Repository: https://github.com/rweather/noise-java
Language: Java
Files changed: .gitignore, src/main/java/com/southernstorm/noise/protocol/AESGCMFallbackCipherState.java, src/main/java/com/southernstorm/noise/protocol/AESGCMOnCtrCipherState.java, src/main/java/com/southernstorm/noise/protocol/ChaChaPolyCipherState.java, src/main/java/com/southernstorm/noise/protocol/CipherState.java
Commit date: 2020-08-29 07:59:27 +1000
Commit message: Improve array bounds checks in CipherState implementations

Thanks to Pietro Oliva for identifying these issues.

Diff: diff --git a/.gitignore b/.gitignore
index 7f3fdcd..9081256 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,6 @@
 target
 .classpath
 .metadata
 .project
 doc
+*.class
diff --git a/src/main/java/com/southernstorm/noise/protocol/AESGCMFallbackCipherState.java b/src/main/java/com/southernstorm/noise/protocol/AESGCMFallbackCipherState.java
index 7cd2b74..f627be3 100644
--- a/src/main/java/com/southernstorm/noise/protocol/AESGCMFallbackCipherState.java
+++ b/src/main/java/com/southernstorm/noise/protocol/AESGCMFallbackCipherState.java
@@ -183,14 +183,15 @@ class AESGCMFallbackCipherState implements CipherState {
 	@Override
 	public int encryptWithAd(byte[] ad, byte[] plaintext, int plaintextOffset,
 			byte[] ciphertext, int ciphertextOffset, int length)
 			throws ShortBufferException {
 		int space;
-		if (ciphertextOffset > ciphertext.length)
-			space = 0;
-		else
-			space = ciphertext.length - ciphertextOffset;
+		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
+			throw new IllegalArgumentException();
+		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
+			throw new IllegalArgumentException();
+		space = ciphertext.length - ciphertextOffset;
 		if (!haskey) {
 			// The key is not set yet - return the plaintext as-is.
 			if (length > space)
 				throw new ShortBufferException();
 			if (plaintext != ciphertext || plaintextOffset != ciphertextOffset)
@@ -212,20 +213,19 @@ class AESGCMFallbackCipherState implements CipherState {
 	@Override
 	public int decryptWithAd(byte[] ad, byte[] ciphertext,
 			int ciphertextOffset, byte[] plaintext, int plaintextOffset,
 			int length) throws ShortBufferException, BadPaddingException {
 		int space;
-		if (ciphertextOffset > ciphertext.length)
-			space = 0;
+		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
+			throw new IllegalArgumentException();
 		else
 			space = ciphertext.length - ciphertextOffset;
 		if (length > space)
 			throw new ShortBufferException();
-		if (plaintextOffset > plaintext.length)
-			space = 0;
-		else
-			space = plaintext.length - plaintextOffset;
+		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
+			throw new IllegalArgumentException();
+		space = plaintext.length - plaintextOffset;
 		if (!haskey) {
 			// The key is not set yet - return the ciphertext as-is.
 			if (length > space)
 				throw new ShortBufferException();
 			if (plaintext != ciphertext || plaintextOffset != ciphertextOffset)
diff --git a/src/main/java/com/southernstorm/noise/protocol/AESGCMOnCtrCipherState.java b/src/main/java/com/southernstorm/noise/protocol/AESGCMOnCtrCipherState.java
index deada67..df40945 100644
--- a/src/main/java/com/southernstorm/noise/protocol/AESGCMOnCtrCipherState.java
+++ b/src/main/java/com/southernstorm/noise/protocol/AESGCMOnCtrCipherState.java
@@ -216,14 +216,15 @@ class AESGCMOnCtrCipherState implements CipherState {
 	@Override
 	public int encryptWithAd(byte[] ad, byte[] plaintext, int plaintextOffset,
 			byte[] ciphertext, int ciphertextOffset, int length)
 			throws ShortBufferException {
 		int space;
-		if (ciphertextOffset > ciphertext.length)
-			space = 0;
-		else
-			space = ciphertext.length - ciphertextOffset;
+		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
+			throw new IllegalArgumentException();
+		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
+			throw new IllegalArgumentException();
+		space = ciphertext.length - ciphertextOffset;
 		if (keySpec == null) {
 			// The key is not set yet - return the plaintext as-is.
 			if (length > space)
 				throw new ShortBufferException();
 			if (plaintext != ciphertext || plaintextOffset != ciphertextOffset)
@@ -260,20 +261,19 @@ class AESGCMOnCtrCipherState implements CipherState {
 	@Override
 	public int decryptWithAd(byte[] ad, byte[] ciphertext,
 			int ciphertextOffset, byte[] plaintext, int plaintextOffset,
 			int length) throws ShortBufferException, BadPaddingException {
 		int space;
-		if (ciphertextOffset > ciphertext.length)
-			space = 0;
+		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
+			throw new IllegalArgumentException();
 		else
 			space = ciphertext.length - ciphertextOffset;
 		if (length > space)
 			throw new ShortBufferException();
-		if (plaintextOffset > plaintext.length)
-			space = 0;
-		else
-			space = plaintext.length - plaintextOffset;
+		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
+			throw new IllegalArgumentException();
+		space = plaintext.length - plaintextOffset;
 		if (keySpec == null) {
 			// The key is not set yet - return the ciphertext as-is.
 			if (length > space)
 				throw new ShortBufferException();
 			if (plaintext != ciphertext || plaintextOffset != ciphertextOffset)
diff --git a/src/main/java/com/southernstorm/noise/protocol/ChaChaPolyCipherState.java b/src/main/java/com/southernstorm/noise/protocol/ChaChaPolyCipherState.java
index 25b1a4c..40ce2bc 100644
--- a/src/main/java/com/southernstorm/noise/protocol/ChaChaPolyCipherState.java
+++ b/src/main/java/com/southernstorm/noise/protocol/ChaChaPolyCipherState.java
@@ -212,14 +212,15 @@ class ChaChaPolyCipherState implements CipherState {
 
 	@Override
 	public int encryptWithAd(byte[] ad, byte[] plaintext, int plaintextOffset,
 			byte[] ciphertext, int ciphertextOffset, int length) throws ShortBufferException {
 		int space;
-		if (ciphertextOffset > ciphertext.length)
-			space = 0;
-		else
-			space = ciphertext.length - ciphertextOffset;
+		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
+			throw new IllegalArgumentException();
+		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
+			throw new IllegalArgumentException();
+		space = ciphertext.length - ciphertextOffset;
 		if (!haskey) {
 			// The key is not set yet - return the plaintext as-is.
 			if (length > space)
 				throw new ShortBufferException();
 			if (plaintext != ciphertext || plaintextOffset != ciphertextOffset)
@@ -239,20 +240,19 @@ class ChaChaPolyCipherState implements CipherState {
 	@Override
 	public int decryptWithAd(byte[] ad, byte[] ciphertext,
 			int ciphertextOffset, byte[] plaintext, int plaintextOffset,
 			int length) throws ShortBufferException, BadPaddingException {
 		int space;
-		if (ciphertextOffset > ciphertext.length)
-			space = 0;
+		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
+			throw new IllegalArgumentException();
 		else
 			space = ciphertext.length - ciphertextOffset;
 		if (length > space)
 			throw new ShortBufferException();
-		if (plaintextOffset > plaintext.length)
-			space = 0;
-		else
-			space = plaintext.length - plaintextOffset;
+		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
+			throw new IllegalArgumentException();
+		space = plaintext.length - plaintextOffset;
 		if (!haskey) {
 			// The key is not set yet - return the ciphertext as-is.
 			if (length > space)
 				throw new ShortBufferException();
 			if (plaintext != ciphertext || plaintextOffset != ciphertextOffset)
diff --git a/src/main/java/com/southernstorm/noise/protocol/CipherState.java b/src/main/java/com/southernstorm/noise/protocol/CipherState.java
index 3168139..74f808b 100644
--- a/src/main/java/com/southernstorm/noise/protocol/CipherState.java
+++ b/src/main/java/com/southernstorm/noise/protocol/CipherState.java
@@ -98,10 +98,12 @@ public interface CipherState extends Destroyable {
 	 * @throws ShortBufferException The ciphertext buffer does not have
 	 * enough space to hold the ciphertext plus MAC.
 	 * 
 	 * @throws IllegalStateException The nonce has wrapped around.
 	 * 
+	 * @throws IllegalArgumentException One of the parameters is out of range.
+	 *
 	 * The plaintext and ciphertext buffers can be the same for in-place
 	 * encryption.  In that case, plaintextOffset must be identical to
 	 * ciphertextOffset.
 	 * 
 	 * There must be enough space in the ciphertext buffer to accomodate
@@ -128,10 +130,12 @@ public interface CipherState extends Destroyable {
 	 * 
 	 * @throws BadPaddingException The MAC value failed to verify.
 	 * 
 	 * @throws IllegalStateException The nonce has wrapped around.
 	 * 
+	 * @throws IllegalArgumentException One of the parameters is out of range.
+	 *
 	 * The plaintext and ciphertext buffers can be the same for in-place
 	 * decryption.  In that case, ciphertextOffset must be identical to
 	 * plaintextOffset.
 	 */
 	int decryptWithAd(byte[] ad, byte[] ciphertext, int ciphertextOffset, byte[] plaintext, int plaintextOffset, int length) throws ShortBufferException, BadPaddingException;

Vulnerable code: 		if (ciphertextOffset > ciphertext.length)
			space = 0;
		else
			space = ciphertext.length - ciphertextOffset;
		if (ciphertextOffset > ciphertext.length)
			space = 0;
		if (plaintextOffset > plaintext.length)
			space = 0;
		else
			space = plaintext.length - plaintextOffset;
		if (ciphertextOffset > ciphertext.length)
			space = 0;
		else
			space = ciphertext.length - ciphertextOffset;
		if (ciphertextOffset > ciphertext.length)
			space = 0;
		if (plaintextOffset > plaintext.length)
			space = 0;
		else
			space = plaintext.length - plaintextOffset;
		if (ciphertextOffset > ciphertext.length)
			space = 0;
		else
			space = ciphertext.length - ciphertextOffset;
		if (ciphertextOffset > ciphertext.length)
			space = 0;
		if (plaintextOffset > plaintext.length)
			space = 0;
		else
			space = plaintext.length - plaintextOffset;
Fixed code: *.class
		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
			throw new IllegalArgumentException();
		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
			throw new IllegalArgumentException();
		space = ciphertext.length - ciphertextOffset;
		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
			throw new IllegalArgumentException();
		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
			throw new IllegalArgumentException();
		space = plaintext.length - plaintextOffset;
		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
			throw new IllegalArgumentException();
		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
			throw new IllegalArgumentException();
		space = ciphertext.length - ciphertextOffset;
		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
			throw new IllegalArgumentException();
		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
			throw new IllegalArgumentException();
		space = plaintext.length - plaintextOffset;
		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
			throw new IllegalArgumentException();
		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
			throw new IllegalArgumentException();
		space = ciphertext.length - ciphertextOffset;
		if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
			throw new IllegalArgumentException();
		if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
			throw new IllegalArgumentException();
		space = plaintext.length - plaintextOffset;
	 * @throws IllegalArgumentException One of the parameters is out of range.
	 *
	 * @throws IllegalArgumentException One of the parameters is out of range.
	 *