Skip to content

Remove clear key revisit TODO #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 2, 2020
Merged

Remove clear key revisit TODO #244

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/main/java/com/pusher/client/crypto/nacl/SecretBoxOpener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ a copy of this software and associated documentation files (the "Software"),

import static com.pusher.client.util.internal.Preconditions.checkArgument;
import static com.pusher.client.util.internal.Preconditions.checkNotNull;

import java.util.Arrays;
import static java.util.Arrays.fill;
Copy link
Contributor

@mdpye mdpye Apr 1, 2020

Choose a reason for hiding this comment

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

I'm not the biggest fan of static imports, but why not do the same for System.arraycopy?

Copy link
Contributor Author

@marekoid marekoid Apr 2, 2020

Choose a reason for hiding this comment

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

I've considered using static import for System.arraycopy but it feels like it's good to convey the information that it's about using quick native OS ("System") call to copy a continuous part of the memory as opposed to Java implementation iterating over all elements.

With fill the call site is as legible without Arrays. which was just the noise.

Out of curiosity why are you not a fan of static imports? You didn't seem to have problems with top level functions in Kotlin which are a very similar thing, indeed on jvm they are the same.


public class SecretBoxOpener {

Expand Down Expand Up @@ -97,13 +96,12 @@ public byte[] open(byte[] box, byte[] nonce) throws AuthenticityException {
}

public void clearKey() {
Arrays.fill(key, (byte) 0);
fill(key, (byte) 0);
if (key[0] != 0) {
// so that hopefully the optimiser won't remove the clearing code (best sensible effort)
throw new SecurityException("key not cleared correctly");
}
key = null;
// TODO: ensure implemented securely (so that the clearing code
// is not removed by compiler's optimisations)
}

// subKey = byte[32], counter = byte[16], nonce = byte[24], key = byte[32]
Expand Down