Skip to content

Commit 18e86b6

Browse files
committed
Improve array bounds checks in CipherState implementations
Thanks to Pietro Oliva for identifying these issues.
1 parent a8dce06 commit 18e86b6

File tree

5 files changed

+35
-30
lines changed

5 files changed

+35
-30
lines changed

Diff for: .gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@ target
33
.metadata
44
.project
55
doc
6+
*.class

Diff for: src/main/java/com/southernstorm/noise/protocol/AESGCMFallbackCipherState.java

+10-10
Original file line numberDiff line numberDiff line change
@@ -185,10 +185,11 @@ public int encryptWithAd(byte[] ad, byte[] plaintext, int plaintextOffset,
185185
byte[] ciphertext, int ciphertextOffset, int length)
186186
throws ShortBufferException {
187187
int space;
188-
if (ciphertextOffset > ciphertext.length)
189-
space = 0;
190-
else
191-
space = ciphertext.length - ciphertextOffset;
188+
if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
189+
throw new IllegalArgumentException();
190+
if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
191+
throw new IllegalArgumentException();
192+
space = ciphertext.length - ciphertextOffset;
192193
if (!haskey) {
193194
// The key is not set yet - return the plaintext as-is.
194195
if (length > space)
@@ -214,16 +215,15 @@ public int decryptWithAd(byte[] ad, byte[] ciphertext,
214215
int ciphertextOffset, byte[] plaintext, int plaintextOffset,
215216
int length) throws ShortBufferException, BadPaddingException {
216217
int space;
217-
if (ciphertextOffset > ciphertext.length)
218-
space = 0;
218+
if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
219+
throw new IllegalArgumentException();
219220
else
220221
space = ciphertext.length - ciphertextOffset;
221222
if (length > space)
222223
throw new ShortBufferException();
223-
if (plaintextOffset > plaintext.length)
224-
space = 0;
225-
else
226-
space = plaintext.length - plaintextOffset;
224+
if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
225+
throw new IllegalArgumentException();
226+
space = plaintext.length - plaintextOffset;
227227
if (!haskey) {
228228
// The key is not set yet - return the ciphertext as-is.
229229
if (length > space)

Diff for: src/main/java/com/southernstorm/noise/protocol/AESGCMOnCtrCipherState.java

+10-10
Original file line numberDiff line numberDiff line change
@@ -218,10 +218,11 @@ public int encryptWithAd(byte[] ad, byte[] plaintext, int plaintextOffset,
218218
byte[] ciphertext, int ciphertextOffset, int length)
219219
throws ShortBufferException {
220220
int space;
221-
if (ciphertextOffset > ciphertext.length)
222-
space = 0;
223-
else
224-
space = ciphertext.length - ciphertextOffset;
221+
if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
222+
throw new IllegalArgumentException();
223+
if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
224+
throw new IllegalArgumentException();
225+
space = ciphertext.length - ciphertextOffset;
225226
if (keySpec == null) {
226227
// The key is not set yet - return the plaintext as-is.
227228
if (length > space)
@@ -262,16 +263,15 @@ public int decryptWithAd(byte[] ad, byte[] ciphertext,
262263
int ciphertextOffset, byte[] plaintext, int plaintextOffset,
263264
int length) throws ShortBufferException, BadPaddingException {
264265
int space;
265-
if (ciphertextOffset > ciphertext.length)
266-
space = 0;
266+
if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
267+
throw new IllegalArgumentException();
267268
else
268269
space = ciphertext.length - ciphertextOffset;
269270
if (length > space)
270271
throw new ShortBufferException();
271-
if (plaintextOffset > plaintext.length)
272-
space = 0;
273-
else
274-
space = plaintext.length - plaintextOffset;
272+
if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
273+
throw new IllegalArgumentException();
274+
space = plaintext.length - plaintextOffset;
275275
if (keySpec == null) {
276276
// The key is not set yet - return the ciphertext as-is.
277277
if (length > space)

Diff for: src/main/java/com/southernstorm/noise/protocol/ChaChaPolyCipherState.java

+10-10
Original file line numberDiff line numberDiff line change
@@ -214,10 +214,11 @@ private void encrypt(byte[] plaintext, int plaintextOffset,
214214
public int encryptWithAd(byte[] ad, byte[] plaintext, int plaintextOffset,
215215
byte[] ciphertext, int ciphertextOffset, int length) throws ShortBufferException {
216216
int space;
217-
if (ciphertextOffset > ciphertext.length)
218-
space = 0;
219-
else
220-
space = ciphertext.length - ciphertextOffset;
217+
if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
218+
throw new IllegalArgumentException();
219+
if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
220+
throw new IllegalArgumentException();
221+
space = ciphertext.length - ciphertextOffset;
221222
if (!haskey) {
222223
// The key is not set yet - return the plaintext as-is.
223224
if (length > space)
@@ -241,16 +242,15 @@ public int decryptWithAd(byte[] ad, byte[] ciphertext,
241242
int ciphertextOffset, byte[] plaintext, int plaintextOffset,
242243
int length) throws ShortBufferException, BadPaddingException {
243244
int space;
244-
if (ciphertextOffset > ciphertext.length)
245-
space = 0;
245+
if (ciphertextOffset < 0 || ciphertextOffset > ciphertext.length)
246+
throw new IllegalArgumentException();
246247
else
247248
space = ciphertext.length - ciphertextOffset;
248249
if (length > space)
249250
throw new ShortBufferException();
250-
if (plaintextOffset > plaintext.length)
251-
space = 0;
252-
else
253-
space = plaintext.length - plaintextOffset;
251+
if (length < 0 || plaintextOffset < 0 || plaintextOffset > plaintext.length)
252+
throw new IllegalArgumentException();
253+
space = plaintext.length - plaintextOffset;
254254
if (!haskey) {
255255
// The key is not set yet - return the ciphertext as-is.
256256
if (length > space)

Diff for: src/main/java/com/southernstorm/noise/protocol/CipherState.java

+4
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,8 @@ public interface CipherState extends Destroyable {
100100
*
101101
* @throws IllegalStateException The nonce has wrapped around.
102102
*
103+
* @throws IllegalArgumentException One of the parameters is out of range.
104+
*
103105
* The plaintext and ciphertext buffers can be the same for in-place
104106
* encryption. In that case, plaintextOffset must be identical to
105107
* ciphertextOffset.
@@ -130,6 +132,8 @@ public interface CipherState extends Destroyable {
130132
*
131133
* @throws IllegalStateException The nonce has wrapped around.
132134
*
135+
* @throws IllegalArgumentException One of the parameters is out of range.
136+
*
133137
* The plaintext and ciphertext buffers can be the same for in-place
134138
* decryption. In that case, ciphertextOffset must be identical to
135139
* plaintextOffset.

0 commit comments

Comments
 (0)