Skip to content

Commit

Permalink
Prevent memory access errors in many native calls by adding checks
Browse files Browse the repository at this point in the history 
This is not comprehensive coverage, but should take care of a number of
unguarded calls. It comes out of issue terl#81 in which a call to
`cryptoSecretStreamPush` with an incorrectly sized array lead to a VM
crash.

- Add array checkers to BaseChecker
- Simplify some existing checks by moving the `throw` into the check
  method; add reporting of which variable failed a check
- Add checkers for SecretStream
- Split PwHash.Checker.checkAll into individual check calls to reduce
  chance of positional argument errors
  • Loading branch information
@timmc
timmc committed Feb 8, 2021
1 parent a09e4db commit 6d3b5c4
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 67 deletions.
58 changes: 27 additions & 31 deletions src/main/java/com/goterl/lazycode/lazysodium/LazySodium.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ public byte[] randomBytesDeterministic(int size, byte[] seed) {

@Override
public boolean sodiumPad(IntByReference paddedBuffLen, char[] buf, int unpaddedBufLen, int blockSize, int maxBufLen) {
BaseChecker.checkArrayLength("buffer", buf, maxBufLen);
return successful(getSodium().sodium_pad(paddedBuffLen.getPointer(), buf, unpaddedBufLen, blockSize, maxBufLen));
}

Expand Down Expand Up @@ -281,9 +282,7 @@ public Key cryptoKdfKeygen() {
@Override
public Key cryptoKdfDeriveFromKey(int lengthOfSubKey, long subKeyId, String context, Key masterKey)
throws SodiumException {
if (!KeyDerivation.Checker.subKeyIsCorrect(lengthOfSubKey)) {
throw new SodiumException("Subkey is not between the correct lengths.");
}
KeyDerivation.Checker.checkSubKeyLength(lengthOfSubKey);
if (!KeyDerivation.Checker.masterKeyIsCorrect(masterKey.getAsBytes().length)) {
throw new SodiumException("Master key is not the correct length.");
}
Expand Down Expand Up @@ -405,6 +404,13 @@ public boolean cryptoPwHash(byte[] outputHash,
long opsLimit,
NativeLong memLimit,
PwHash.Alg alg) {
PwHash.Checker.checkPassword(password);
PwHash.Checker.checkSalt(salt);
PwHash.Checker.checkOpsLimit(opsLimit);
PwHash.Checker.checkMemLimit(memLimit);

BaseChecker.checkArrayLength("password bytes", password, passwordLen);

if (outputHashLen < 0 || outputHashLen > outputHash.length) {
throw new IllegalArgumentException("outputHashLen out of bounds: " + outputHashLen);
}
Expand Down Expand Up @@ -455,7 +461,10 @@ public boolean cryptoPwHashStrNeedsRehash(byte[] hash, long opsLimit, NativeLong
public String cryptoPwHash(String password, int lengthOfHash, byte[] salt, long opsLimit, NativeLong memLimit, PwHash.Alg alg)
throws SodiumException {
byte[] passwordBytes = bytes(password);
PwHash.Checker.checkAll(passwordBytes.length, salt.length, opsLimit, memLimit);
PwHash.Checker.checkPassword(passwordBytes);
PwHash.Checker.checkSalt(salt);
PwHash.Checker.checkOpsLimit(opsLimit);
PwHash.Checker.checkMemLimit(memLimit);
byte[] hash = new byte[lengthOfHash];
int res = getSodium().crypto_pwhash(hash, hash.length, passwordBytes, passwordBytes.length, salt, opsLimit, memLimit, alg.getValue());
if (res != 0) {
Expand Down Expand Up @@ -1274,19 +1283,20 @@ public KeyPair convertKeyPairEd25519ToCurve25519(KeyPair ed25519KeyPair) throws

@Override
public void cryptoSecretStreamKeygen(byte[] key) {
SecretStream.Checker.checkKey(key);
getSodium().crypto_secretstream_xchacha20poly1305_keygen(key);
}

@Override
public boolean cryptoSecretStreamInitPush(SecretStream.State state, byte[] header, byte[] key) {
SecretStream.Checker.checkHeader(header);
SecretStream.Checker.checkKey(key);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_init_push(state, header, key));
}

@Override
public boolean cryptoSecretStreamPush(SecretStream.State state, byte[] cipher, long[] cipherAddr, byte[] message, long messageLen, byte tag) {
if (messageLen < 0 || messageLen > message.length) {
throw new IllegalArgumentException("messageLen out of bounds: " + messageLen);
}
SecretStream.Checker.checkPush(message, messageLen, cipher);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_push(
state,
cipher,
Expand All @@ -1305,9 +1315,7 @@ public boolean cryptoSecretStreamPush(SecretStream.State state,
byte[] message,
long messageLen,
byte tag) {
if (messageLen < 0 || messageLen > message.length) {
throw new IllegalArgumentException("messageLen out of bounds: " + messageLen);
}
SecretStream.Checker.checkPush(message, messageLen, cipher);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_push(
state,
cipher,
Expand All @@ -1329,12 +1337,8 @@ public boolean cryptoSecretStreamPush(SecretStream.State state,
byte[] additionalData,
long additionalDataLen,
byte tag) {
if (messageLen < 0 || messageLen > message.length) {
throw new IllegalArgumentException("messageLen out of bounds: " + messageLen);
}
if (additionalDataLen < 0 || additionalDataLen > additionalData.length) {
throw new IllegalArgumentException("additionalDataLen out of bounds: " + additionalDataLen);
}
SecretStream.Checker.checkPush(message, messageLen, cipher);
BaseChecker.checkArrayLength("additional data", additionalData, additionalDataLen);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_push(
state,
cipher,
Expand All @@ -1349,6 +1353,8 @@ public boolean cryptoSecretStreamPush(SecretStream.State state,

@Override
public boolean cryptoSecretStreamInitPull(SecretStream.State state, byte[] header, byte[] key) {
SecretStream.Checker.checkHeader(header);
SecretStream.Checker.checkKey(key);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_init_pull(state, header, key));
}

Expand All @@ -1361,22 +1367,16 @@ public boolean cryptoSecretStreamPull(SecretStream.State state,
long cipherLen,
byte[] additionalData,
long additionalDataLen) {
if (cipherLen < 0 || cipherLen > cipher.length) {
throw new IllegalArgumentException("cipherLen out of bounds: " + cipherLen);
}
if (additionalDataLen < 0 || additionalDataLen > additionalData.length) {
throw new IllegalArgumentException("additionalDataLen out of bounds: " + additionalDataLen);
}
SecretStream.Checker.checkPull(cipher, cipherLen, message);
BaseChecker.checkArrayLength("additional data", additionalData, additionalDataLen);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_pull(
state, message, messageAddress, tag, cipher, cipherLen, additionalData, additionalDataLen
));
}

@Override
public boolean cryptoSecretStreamPull(SecretStream.State state, byte[] message, byte[] tag, byte[] cipher, long cipherLen) {
if (cipherLen < 0 || cipherLen > cipher.length) {
throw new IllegalArgumentException("cipherLen out of bounds: " + cipherLen);
}
SecretStream.Checker.checkPull(cipher, cipherLen, message);
return successful(getSodium().crypto_secretstream_xchacha20poly1305_pull(
state,
message,
Expand All @@ -1398,10 +1398,8 @@ public Key cryptoSecretStreamKeygen() {

@Override
public SecretStream.State cryptoSecretStreamInitPush(byte[] header, Key key) throws SodiumException {
SecretStream.Checker.checkHeader(header);
SecretStream.State state = new SecretStream.State.ByReference();
if (!SecretStream.Checker.headerCheck(header.length)) {
throw new SodiumException("Header of secret stream incorrect length.");
}
getSodium().crypto_secretstream_xchacha20poly1305_init_push(state, header, key.getAsBytes());
return state;
}
Expand Down Expand Up @@ -1430,10 +1428,8 @@ public String cryptoSecretStreamPush(SecretStream.State state, String message, b

@Override
public SecretStream.State cryptoSecretStreamInitPull(byte[] header, Key key) throws SodiumException {
SecretStream.Checker.checkHeader(header);
SecretStream.State state = new SecretStream.State.ByReference();
if (!SecretStream.Checker.headerCheck(header.length)) {
throw new SodiumException("Header of secret stream incorrect length.");
}

int res = getSodium().crypto_secretstream_xchacha20poly1305_init_pull(state, header, key.getAsBytes());

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/goterl/lazycode/lazysodium/interfaces/KeyDerivation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ public static boolean masterKeyIsCorrect(long masterKeyLen) {
return masterKeyLen == KeyDerivation.MASTER_KEY_BYTES;
}

public static boolean subKeyIsCorrect(int lengthOfSubkey) {
return isBetween(lengthOfSubkey, BYTES_MIN, BYTES_MAX);
public static void checkSubKeyLength(int subkeyLen) {
checkBetween("subkey length", subkeyLen, BYTES_MIN, BYTES_MAX);
}

public static boolean contextIsCorrect(int length) {
Expand Down
37 changes: 10 additions & 27 deletions src/main/java/com/goterl/lazycode/lazysodium/interfaces/PwHash.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,37 +70,20 @@ public interface PwHash {


class Checker extends BaseChecker {
public static boolean saltIsCorrect(long saltLen) {
return correctLen(saltLen, PwHash.SALTBYTES);
public static void checkPassword(byte[] password) {
checkBetween("password length", password.length, PwHash.PASSWD_MIN, PwHash.PASSWD_MAX);
}
public static boolean passwordIsCorrect(long len) {
return isBetween(len, PwHash.PASSWD_MIN, PwHash.PASSWD_MAX);
}
public static boolean opsLimitIsCorrect(long ops) {
return isBetween(ops, PwHash.OPSLIMIT_MIN, PwHash.OPSLIMIT_MAX);

public static void checkSalt(byte[] salt) {
checkEqual("salt length", salt.length, SALTBYTES);
}
public static boolean memLimitIsCorrect(NativeLong len) {
return isBetween(len, PwHash.MEMLIMIT_MIN, PwHash.MEMLIMIT_MAX);

public static void checkOpsLimit(long opsLimit) {
checkBetween("opsLimit", opsLimit, PwHash.OPSLIMIT_MIN, PwHash.OPSLIMIT_MAX);
}

public static boolean checkAll(long passwordBytesLen,
long saltBytesLen,
long opsLimit,
NativeLong memLimit)
throws SodiumException {
if (!PwHash.Checker.saltIsCorrect(saltBytesLen)) {
throw new SodiumException("The salt provided is not the correct length.");
}
if (!PwHash.Checker.passwordIsCorrect(passwordBytesLen)) {
throw new SodiumException("The password provided is not the correct length.");
}
if (!PwHash.Checker.opsLimitIsCorrect(opsLimit)) {
throw new SodiumException("The opsLimit provided is not the correct value.");
}
if (!PwHash.Checker.memLimitIsCorrect(memLimit)) {
throw new SodiumException("The memLimit provided is not the correct value.");
}
return true;
public static void checkMemLimit(NativeLong memLimit) {
checkBetween("memLimit", memLimit, PwHash.MEMLIMIT_MIN, PwHash.MEMLIMIT_MAX);
}
}

Expand Down
22 changes: 20 additions & 2 deletions src/main/java/com/goterl/lazycode/lazysodium/interfaces/SecretStream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,26 @@ public interface SecretStream {

class Checker extends BaseChecker {

public static boolean headerCheck(int headerSize) {
return headerSize == HEADERBYTES;
public static void checkHeader(byte[] header) {
checkEqual("secret stream header length", HEADERBYTES, header.length);
}

public static void checkKey(byte[] key) {
checkEqual("secret stream key length", KEYBYTES, key.length);
}

public static void checkPush(byte[] message, long messageLen, byte[] cipher) {
checkArrayLength("message bytes", message, messageLen);
if (cipher.length < messageLen + ABYTES) {
throw new IllegalArgumentException("Cipher array too small for messageLen + header");
}
}

public static void checkPull(byte[] cipher, long cipherLen, byte[] message) {
checkArrayLength("message bytes", cipher, cipherLen);
if (message.length < cipherLen - ABYTES) {
throw new IllegalArgumentException("Message array too small for cipherLen - header");
}
}

}
Expand Down
48 changes: 43 additions & 5 deletions src/main/java/com/goterl/lazycode/lazysodium/utils/BaseChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,55 @@

public class BaseChecker {

public static boolean isBetween(long num, long min, long max) {
return min <= num && num <= max;
public static void checkBetween(String name, long num, long min, long max) {
if (num < min) {
throw new IllegalArgumentException("Provided " + name + " is below minimum bound.");
}
if (num > max) {
throw new IllegalArgumentException("Provided " + name + " is above maximum bound.");
}
}

public static boolean isBetween(NativeLong num, NativeLong min, NativeLong max) {
long number = num.longValue();
return min.longValue() <= number && number <= max.longValue();
public static void checkBetween(String name, NativeLong num, NativeLong min, NativeLong max) {
checkBetween(name, num.longValue(), min.longValue(), max.longValue());
}

public static boolean isBetween(long num, long min, long max) {
return min <= num && num <= max;
}

public static boolean correctLen(long num, long len) {
return num == len;
}

/**
* Throw if provided value does not match an expected value.
*/
public static void checkEqual(String name, int expected, int actual) {
if (actual != expected) {
// Neither value is reported, in case this is passed sensitive
// values, even though most uses are likely for header lengths and
// similar.
throw new IllegalArgumentException(
"Provided " + name + " did not match expected value");
}
}

public static void checkArrayLength(String name, char[] array, long length) {
checkArrayLength(name, array.length, length);
}

public static void checkArrayLength(String name, byte[] array, long length) {
checkArrayLength(name, array.length, length);
}

private static void checkArrayLength(String name, int arrayLength, long length) {
if (length > arrayLength) {
throw new IllegalArgumentException("Provided " + name + " array length is larger than array");
}
if (length < 0) {
throw new IllegalArgumentException("Provided " + name + " array length is negative");
}
}

}

0 comments on commit 6d3b5c4

Please sign in to comment.