Skip to content

use SecureRandom for digest auth cnonce#2220

Open
jmestwa-coder wants to merge 1 commit into
AsyncHttpClient:mainfrom
jmestwa-coder:digest-cnonce-securerandom
Open

use SecureRandom for digest auth cnonce#2220
jmestwa-coder wants to merge 1 commit into
AsyncHttpClient:mainfrom
jmestwa-coder:digest-cnonce-securerandom

Conversation

@jmestwa-coder

Copy link
Copy Markdown
Contributor

The digest cnonce was drawn from a non-cryptographic PRNG:

  • Realm.Builder.newCnonce seeds the cnonce from ThreadLocalRandom, whose output is predictable
  • RFC 7616 section 3.3 needs the cnonce unpredictable so it guards against chosen-plaintext and precomputation attacks on the credentials
  • NtlmEngine and ScramEngine in this repo already use SecureRandom for their nonces

Switched the cnonce source to SecureRandom to match.

public static class Builder {

// cnonce must be unpredictable (RFC 7616 section 3.3), like the NTLM and SCRAM nonces
private static final SecureRandom CNONCE_RANDOM = new SecureRandom();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't share one static SecureRandom; use a ThreadLocal. newCnonce runs on Netty event-loop threads (digest header build + 401/407 interceptors), and SecureRandom#nextBytes locks internally, so a single shared instance makes all event loops queue on one lock. ThreadLocalRandom had no such lock. ScramEngine already handles this the right way:

private static final ThreadLocal<SecureRandom> CNONCE_RANDOM = ThreadLocal.withInitial(SecureRandom::new);

and in newCnonce:

CNONCE_RANDOM.get().nextBytes(b);

byte[] b = new byte[8];
ThreadLocalRandom.current().nextBytes(b);
CNONCE_RANDOM.nextBytes(b);
byte[] full = md.digest(b);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the md.digest(b) step is now pointless. The 8 bytes are already cryptographically random, so hashing them and trimming back to 8 bytes adds nothing. The method can just be:

private void newCnonce() {
    byte[] b = new byte[8];
    CNONCE_RANDOM.get().nextBytes(b);
    cnonce = toHexString(b);
}

Same 16-hex-char cnonce. This is safe because pooledMessageDigest already resets the digest before build() gets it, so nothing downstream depends on the digest(b) call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants