CWE-331 in DigestAuthentication class

[mslowiak]

Jetty version
jetty-client-9.4.30.v20200611

Java version
Java 11

OS type/version
Windows

Description
We are using the Wiremock standalone version (the newest com.github.tomakehurst:wiremock-jre8-standalone:2.27.1) with jetty-client-9.4.30.v20200611 included.
Veracode scan complains about CWE-331 located in DigestAuthentication in line 221 which contains code:

private String newClientNonce()
{
    Random random = new Random();
    byte[] bytes = new byte[8];
    random.nextBytes(bytes);
    return toHexString(bytes);
}

[joakime]

Adding direct link to line 221 in the reported version 9.4.30 (for reference by future readers of this issue) ...

https://github.com/eclipse/jetty.project/blob/jetty-9.4.30.v20200611/jetty-client/src/main/java/org/eclipse/jetty/client/util/DigestAuthentication.java#L221

[gregw]

@mslowiak Thanks for that report. It is not actually a security issue because the nonce used in DIGEST auth is just there to prevent replay attacks and doesn't need to be strong. In fact some common implementations just use the system time! However, we will make sure that our next release (out in a week or two) will use an algorithm that wont be flagged by Veracode.

[mslowiak]

@gregw Thank you for fast check and willing to fix that :-)

[sbordet]

@gregw the PR is wrong as Random and SecureRandom are stateful and not thread safe.
There was a reason they were created on-the-fly, and that should be restored.

[joakime]

The apidoc for SecureRandom doesn't have the warning about threadsafety.

Also, the Java 9+ version of the SecureRandom javadoc says ...

Thread safety
SecureRandom objects are safe for use by multiple concurrent threads.

https://docs.oracle.com/javase/9/docs/api/java/security/SecureRandom.html#:~:text=Thread%20safety,true%22%20when%20registering%20the%20provider.

[sbordet]

Okay I learnt that Random was made thread safe (it was not).

But now it becomes a global contended lock, like the javadocs of Random say.

I still think it should be reverted to allocating rather than contending a static final lock.

[sbordet]

Plus, I think it should be benchmarked since the PR changed the use of Random with SecureRandom in many places, not only in the Digest code reported originally.

[gregw]

Benchmarking SecureRandom can be difficult because it relies on a good sources of entropy like doing actual network traffic. A micro benchmark will quickly exhaust entropy because it is not having real interactions with real random sources. Using SecureRandom for digest auth is a good match because there is network traffic associated with every use, so there is entropy provided.

Creating a temporary Random using it and then disposing of it was always a bad thing to do. Not only is it a source of needless garbage but it essentially makes your random numbers a function of (nanotime), rather than a function of (nanotime, previous requests). This makes them a lot easier to predict... at least reduces the search space.

Creating a SecureRandom to discard is even worse as it does provider searches and I believe seeding can consumes more entropy than nextBytes, so that is just a bad idea.
SecureRandom is going to need to mutually exclude at some level as it needs to access /dev/random.

If we see contention on SecureRandom then we can create a pool of threadlocal ones.. or a pool of Randoms that are regularly seeded from a shared SecureRandom.... or something similar, but I would not be surprised if the providers already do something like that under the hood.

[gregw]

@sbordet @joakime @lorban Due to me being a heavy handed idiot, I just committed f6d3984 to jetty-9.4.x head.
The intention is to make it possible to pass in the Random to be used, and if not given then to use a weak pseudo random algorithm.

The default of digest is to use SecureRandom.
The default for websocket masking and multipart is to use a weak random based on system hashcodes and nanotime.

Please have a look as see review in situ... if you don't like I'll roll back and make a PR.

[joakime]

Copying my comments on the commit into here for reference at a later date.

---

Regarding pseudo random on multipart boundary lines ...

This pseudo random isn't sufficient for client multipart boundary.

See similar discussion at firefox about this as well ...

• https://bugzilla.mozilla.org/show_bug.cgi?id=464071
• https://bugzilla.mozilla.org/show_bug.cgi?id=461204

In short, it needs to be using SecureRandom (like it was before).

The only change this file needs is to remove the static from (the left-hand) line 73

---

Regarding WebSocket client to server masking ...

According to the WebSocket Protocol Spec.
https://tools.ietf.org/html/rfc6455#section-5.3

The masking key is a 32-bit value chosen at random by the client.
When preparing a masked frame, the client MUST pick a fresh masking
key from the set of allowed 32-bit values. The masking key needs to
be unpredictable; thus, the masking key MUST be derived from a strong
source of entropy, and the masking key for a given frame MUST NOT
make it simple for a server/proxy to predict the masking key for a
subsequent frame. The unpredictability of the masking key is
essential to prevent authors of malicious applications from selecting
the bytes that appear on the wire. RFC 4086 [RFC4086] discusses what
entails a suitable source of entropy for security-sensitive
applications.

This MUST be derived from SecureRandom.

[joakime]

I added a simple jmh test for the various SecureRandom algorithms to a new branch .
Results for that commit can be found at b0f68a7#commitcomment-40690103

[gregw]

@joakime while I don't think micro benchmarks for random are going to be fair to any that wait for sufficient entropy, your results do indicate that there does not appear to be any significant problems with contention. The rate of random number production is not affected (good or bad) by thread count.

[gregw]

@joakime the multipart boundary problem is only a problem for browsers, as the state of the random number generator can be determined and then future boundaries used to track users.
I don't think the weakly unique algorithm we are using will be predictable in the same way. The state of the heap, current nano time and the thread allocated are not going to be as identifiable as a well known Random number sequence. Specifically two different clients using our code are likely to have similar hashcodes for their threads and string builders and thus neither will act as a good user ID.
Besides our client is not a browser and thus doesn't really have that tracking concern. So I'm inclined to leave as is.