Skip to content

Commit

Permalink
Improves #5053 by giving option of secure or pseudo random
Browse files Browse the repository at this point in the history 
Allow random to be passed in and can default to a weak pseudo random.
  • Loading branch information
@gregw
gregw committed Jul 17, 2020
1 parent 8e7bfa0 commit f6d3984
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 23 deletions.
30 changes: 25 additions & 5 deletions jetty-client/src/main/java/org/eclipse/jetty/client/util/DigestAuthentication.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.atomic.AtomicInteger;

import org.eclipse.jetty.client.HttpClient;
Expand All @@ -46,19 +47,32 @@
*/
public class DigestAuthentication extends AbstractAuthentication
{
private static final SecureRandom random = new SecureRandom();
private final Random random;
private final String user;
private final String password;

/**
/** Construct a DigestAuthentication with a {@link SecureRandom} nonce.
* @param uri the URI to match for the authentication
* @param realm the realm to match for the authentication
* @param user the user that wants to authenticate
* @param password the password of the user
*/
public DigestAuthentication(URI uri, String realm, String user, String password)
{
this(uri, realm, user, password, new SecureRandom());
}

/**
* @param uri the URI to match for the authentication
* @param realm the realm to match for the authentication
* @param user the user that wants to authenticate
* @param password the password of the user
* @param random the Random generator to use for nonces, or null for a weak algorithm.
*/
public DigestAuthentication(URI uri, String realm, String user, String password, Random random)
{
super(uri, realm);
this.random = random;
this.user = user;
this.password = password;
}
Expand Down Expand Up @@ -217,9 +231,15 @@ private String nextNonceCount()

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

long pseudoRandom = System.nanoTime() ^ System.identityHashCode(new Object());
return Long.toHexString(pseudoRandom);
}

private String toHexString(byte[] bytes)
Expand Down
13 changes: 3 additions & 10 deletions jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartContentProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@
import java.io.IOException;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.Random;
import java.util.concurrent.atomic.AtomicBoolean;

import org.eclipse.jetty.client.AsyncContentProvider;
Expand Down Expand Up @@ -70,7 +68,6 @@ public class MultiPartContentProvider extends AbstractTypedContentProvider imple
private static final Logger LOG = Log.getLogger(MultiPartContentProvider.class);
private static final byte[] COLON_SPACE_BYTES = new byte[]{':', ' '};
private static final byte[] CR_LF_BYTES = new byte[]{'\r', '\n'};
private static final Random random = new SecureRandom();

private final List<Part> parts = new ArrayList<>();
private final ByteBuffer firstBoundary;
Expand Down Expand Up @@ -102,13 +99,9 @@ public MultiPartContentProvider(String boundary)
private static String makeBoundary()
{
StringBuilder builder = new StringBuilder("JettyHttpClientBoundary");
int length = builder.length();
while (builder.length() < length + 16)
{
long rnd = random.nextLong();
builder.append(Long.toString(rnd < 0 ? -rnd : rnd, 36));
}
builder.setLength(length + 16);
builder.append(Long.toString(System.identityHashCode(builder), 36));
builder.append(Long.toString(System.identityHashCode(Thread.currentThread()), 36));
builder.append(Long.toString(System.nanoTime(), 36));

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Jul 17, 2020

Contributor

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

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

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
return builder.toString();
}

Expand Down
5 changes: 1 addition & 4 deletions jetty-plus/src/main/java/org/eclipse/jetty/plus/webapp/PlusConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@

package org.eclipse.jetty.plus.webapp;

import java.security.SecureRandom;
import java.util.Random;
import javax.naming.Context;
import javax.naming.InitialContext;
import javax.naming.NameNotFoundException;
Expand All @@ -40,7 +38,6 @@
public class PlusConfiguration extends AbstractConfiguration
{
private static final Logger LOG = Log.getLogger(PlusConfiguration.class);
private static final Random __random = new SecureRandom();

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Jul 17, 2020

Contributor

You could have just changed this line to remove the static and that would have been enough.

private Integer _key;

Expand Down Expand Up @@ -101,7 +98,7 @@ protected void lockCompEnv(WebAppContext wac)
{
try (ThreadClassLoaderScope scope = new ThreadClassLoaderScope(wac.getClassLoader()))
{
_key = __random.nextInt();
_key = (int)(this.hashCode() ^ System.nanoTime());
Context context = new InitialContext();
Context compCtx = (Context)context.lookup("java:comp");
compCtx.addToEnvironment(NamingContext.LOCK_PROPERTY, _key);
Expand Down
26 changes: 22 additions & 4 deletions ...websocket-client/src/main/java/org/eclipse/jetty/websocket/client/masks/RandomMasker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

package org.eclipse.jetty.websocket.client.masks;

import java.security.SecureRandom;
import java.util.Random;

import org.eclipse.jetty.websocket.common.WebSocketFrame;
Expand All @@ -29,7 +28,7 @@ public class RandomMasker implements Masker

public RandomMasker()
{
this(new SecureRandom());
this(null);
}

public RandomMasker(Random random)
Expand All @@ -40,8 +39,27 @@ public RandomMasker(Random random)
@Override
public void setMask(WebSocketFrame frame)
{
byte[] mask = new byte[4];
random.nextBytes(mask);
byte[] mask;
if (random != null)
{
mask = new byte[4];
random.nextBytes(mask);
}
else
{
// This is a weak random, but sufficient for a mask.
// Using a SecureRandom would result in lock contention
// Using a Random is as more predictable than this algorithm
// Using a onetime random is essentially a system time.
int pseudoRandom = (int)(System.identityHashCode(frame.hashCode()) ^ System.nanoTime());
mask = new byte[]
{
(byte)pseudoRandom,
(byte)(pseudoRandom >> 8),
(byte)(pseudoRandom >> 16),
(byte)(pseudoRandom >> 24),
};
}

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Jul 17, 2020

Contributor

This had me worried that consecutive small frames might have the same mask, so i tested it ...

    public static void main(String[] args)
    {
        int count = 20;

        RandomMasker masker = new RandomMasker();
        WebSocketFrame[] frames = new WebSocketFrame[count];

        for (int i = 0; i < count; i++)
        {
            frames[i] = new TextFrame().setPayload("ack");
            masker.setMask(frames[i]);
        }

        for (int i = 0; i < count; i++)
        {
            ByteBuffer maskBuffer = ByteBuffer.wrap(frames[i].getMask());
            System.out.printf("frame[%d].mask = %s%n", i, BufferUtil.toDetailString(maskBuffer));
        }
    }

Results in ...

frame[0].mask = HeapByteBuffer@2ef5e5e3[p=0,l=4,c=4,r=4]={<<<\xF0Oh\xEe>>>}
frame[1].mask = HeapByteBuffer@27a5f880[p=0,l=4,c=4,r=4]={<<<\xF8t\x89\xDa>>>}
frame[2].mask = HeapByteBuffer@1d29cf23[p=0,l=4,c=4,r=4]={<<<\xEe\xA4I\xD2>>>}
frame[3].mask = HeapByteBuffer@5f282abb[p=0,l=4,c=4,r=4]={<<<H\x8dY\xC5>>>}
frame[4].mask = HeapByteBuffer@167fdd33[p=0,l=4,c=4,r=4]={<<<\xE7\x1f\x8f\xC8>>>}
frame[5].mask = HeapByteBuffer@1e965684[p=0,l=4,c=4,r=4]={<<<=\xC8\xEc\xFf>>>}
frame[6].mask = HeapByteBuffer@4d95d2a2[p=0,l=4,c=4,r=4]={<<<\x10\xA8\xEd\xC6>>>}
frame[7].mask = HeapByteBuffer@53f65459[p=0,l=4,c=4,r=4]={<<<\xB5C\xA1\xE8>>>}
frame[8].mask = HeapByteBuffer@3b088d51[p=0,l=4,c=4,r=4]={<<<\xC7\x05\x1e\xFf>>>}
frame[9].mask = HeapByteBuffer@1786dec2[p=0,l=4,c=4,r=4]={<<<\xA3}\xAf\xC2>>>}
frame[10].mask = HeapByteBuffer@74650e52[p=0,l=4,c=4,r=4]={<<<\x03CP\xCd>>>}
frame[11].mask = HeapByteBuffer@15d0c81b[p=0,l=4,c=4,r=4]={<<<\xB2\x1dw\x8a>>>}
frame[12].mask = HeapByteBuffer@6acdbdf5[p=0,l=4,c=4,r=4]={<<<|\xA6\xBe\xA0>>>}
frame[13].mask = HeapByteBuffer@4b1c1ea0[p=0,l=4,c=4,r=4]={<<<\x07\xA2<\xB3>>>}
frame[14].mask = HeapByteBuffer@17579e0f[p=0,l=4,c=4,r=4]={<<<9\xA7^\xD6>>>}
frame[15].mask = HeapByteBuffer@4d41cee[p=0,l=4,c=4,r=4]={<<<\x83\x9d\x9b\xCf>>>}
frame[16].mask = HeapByteBuffer@3712b94[p=0,l=4,c=4,r=4]={<<<\x13\xDa\xA9\xB1>>>}
frame[17].mask = HeapByteBuffer@2833cc44[p=0,l=4,c=4,r=4]={<<<`g9\xCf>>>}
frame[18].mask = HeapByteBuffer@33f88ab[p=0,l=4,c=4,r=4]={<<<\x19\xD0\x93\xC4>>>}
frame[19].mask = HeapByteBuffer@27a8c74e[p=0,l=4,c=4,r=4]={<<<*m\xA3\xFe>>>}

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Jul 17, 2020

Contributor

BUT ...

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.
frame.setMask(mask);
}
}

0 comments on commit f6d3984

Please sign in to comment.