Skip to content
This repository has been archived by the owner on Apr 23, 2019. It is now read-only.

Fixes memory leak when decrypting scuttlebutt incoming messages #209

Merged
merged 5 commits into from
Apr 7, 2019
Merged

Fixes memory leak when decrypting scuttlebutt incoming messages #209

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
774f6d8
Fixes memory leak when decrypting scuttlebutt incoming messages - lib…
Happy0 Apr 5, 2019
1e58b1c
Fix formatting with
Happy0 Apr 5, 2019
c8f879f
Improve error handling on sodium error on decrypting incoming scuttle…
Happy0 Apr 5, 2019
4d4fd15
Remove blank line.
Happy0 Apr 5, 2019
6f92b79
Implement 'destroyable' rather than 'auto closable' for the SecretBox…
Happy0 Apr 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion crypto/src/main/java/net/consensys/cava/crypto/sodium/SecretBox.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public byte[] bytesArray() {
/**
* A SecretBox nonce.
*/
public static final class Nonce {
public static final class Nonce implements AutoCloseable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with making a Nonce "auto closeable". It's not a resource, so it feels odd to treat it like one. We could certainly make it implement Destroyable, just like the secret keys.

However, whatever we do, it must be applied consistently across all cava-sodium types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cleishm - I see where you're coming from. I've pushed a new commit to this PR to implement Destroyable instead.

There's currently a couple of other classes (below) that implement AutoCloseable that aren't 'resources' (like sockets) but on searching, it looks like the majority implement Destroyable like you suggested so I'll follow that convention.

public final class AES256GCM implements AutoCloseable {

public final class Box implements AutoCloseable {

Perhaps these can be made to implement 'destroyable' also in a separate PR instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a different flow. You open a secret box and close it - you don't destroy it. It's more the pointers of the keys and nonces generated along the way we need to clean up.

final Allocated value;

private Nonce(Pointer ptr, int length) {
Expand Down Expand Up @@ -230,6 +230,10 @@ public static Nonce fromBytes(byte[] bytes) {
return Sodium.dup(bytes, Nonce::new);
}

public void destroy() {
this.value.destroy();
}

/**
* Obtain the length of the nonce in bytes (24).
*
Expand Down Expand Up @@ -312,6 +316,11 @@ public Bytes bytes() {
public byte[] bytesArray() {
return value.bytesArray();
}

@Override
public void close() {
destroy();
}
}

/**
Expand Down
48 changes: 29 additions & 19 deletions ...shake/src/main/java/net/consensys/cava/scuttlebutt/handshake/SecureScuttlebuttStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import net.consensys.cava.bytes.MutableBytes;
import net.consensys.cava.crypto.sodium.SHA256Hash;
import net.consensys.cava.crypto.sodium.SecretBox;
import net.consensys.cava.crypto.sodium.SodiumException;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -100,29 +101,38 @@ private Bytes decrypt(Bytes message, SecretBox.Key key, MutableBytes nonce, bool
}

private Bytes decryptMessage(Bytes message, SecretBox.Key key, MutableBytes nonce) {
if (message.size() < 34) {
return null;
}
MutableBytes snapshotNonce = nonce.mutableCopy();
SecretBox.Nonce headerNonce = SecretBox.Nonce.fromBytes(snapshotNonce);
SecretBox.Nonce bodyNonce = SecretBox.Nonce.fromBytes(snapshotNonce.increment());
Bytes decryptedHeader = SecretBox.decrypt(message.slice(0, 34), key, headerNonce);

if (decryptedHeader == null) {
throw new StreamException("Failed to decrypt message header");
}
try {
try (SecretBox.Nonce headerNonce = SecretBox.Nonce.fromBytes(snapshotNonce);
SecretBox.Nonce bodyNonce = SecretBox.Nonce.fromBytes(snapshotNonce.increment());) {
if (message.size() < 34) {
return null;
}

int bodySize = ((decryptedHeader.get(0) & 0xFF) << 8) + (decryptedHeader.get(1) & 0xFF);
if (message.size() < bodySize + 34) {
return null;
}
Bytes body = message.slice(34, bodySize);
Bytes decryptedBody = SecretBox.decrypt(Bytes.concatenate(decryptedHeader.slice(2), body), key, bodyNonce);
if (decryptedBody == null) {
throw new StreamException("Failed to decrypt message");
Bytes decryptedHeader = SecretBox.decrypt(message.slice(0, 34), key, headerNonce);

if (decryptedHeader == null) {
throw new StreamException("Failed to decrypt message header");
}

int bodySize = ((decryptedHeader.get(0) & 0xFF) << 8) + (decryptedHeader.get(1) & 0xFF);
if (message.size() < bodySize + 34) {
return null;
}
Bytes body = message.slice(34, bodySize);
Bytes decryptedBody = SecretBox.decrypt(Bytes.concatenate(decryptedHeader.slice(2), body), key, bodyNonce);
if (decryptedBody == null) {
throw new StreamException("Failed to decrypt message");
}
nonce.increment().increment();

return decryptedBody;

}
} catch (SodiumException | OutOfMemoryError ex) {
throw new StreamException(ex);
}
nonce.increment().increment();
return decryptedBody;
}

private Bytes encrypt(Bytes message, SecretBox.Key clientToServerKey, MutableBytes clientToServerNonce) {
Expand Down
5 changes: 5 additions & 0 deletions ...utt-handshake/src/main/java/net/consensys/cava/scuttlebutt/handshake/StreamException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,9 @@ public final class StreamException extends RuntimeException {
StreamException(String message) {
super(message);
}

StreamException(Throwable ex) {
super(ex);
}

}