Skip to content

Commit cac0e71

Browse files
eclipsewebmastereclipsewebmaster
authored
Merge pull request from GHSA-pq4w-qm9g-qx68
Validate nonce length and content on receipt and before being used
2 parents d9d89aa + cc5403d commit cac0e71

File tree

8 files changed

+170
-32
lines changed

8 files changed

+170
-32
lines changed

‎opc-ua-sdk/sdk-client/src/main/java/org/eclipse/milo/opcua/sdk/client/api/identity/UsernameProvider.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
import java.security.GeneralSecurityException;
1717
import java.security.cert.X509Certificate;
1818
import java.util.List;
19-
import java.util.Optional;
2019
import java.util.function.Function;
2120
import java.util.stream.Collectors;
2221
import javax.crypto.Cipher;
@@ -37,6 +36,7 @@
3736
import org.eclipse.milo.opcua.stack.core.types.structured.UserNameIdentityToken;
3837
import org.eclipse.milo.opcua.stack.core.types.structured.UserTokenPolicy;
3938
import org.eclipse.milo.opcua.stack.core.util.CertificateUtil;
39+
import org.eclipse.milo.opcua.stack.core.util.NonceUtil;
4040
import org.slf4j.Logger;
4141
import org.slf4j.LoggerFactory;
4242

@@ -134,8 +134,10 @@ public SignedIdentityToken getIdentityToken(EndpointDescription endpoint,
134134
logger.warn("Error parsing SecurityPolicy for uri={}, falling back to no security.", securityPolicyUri);
135135
}
136136

137+
NonceUtil.validateNonce(serverNonce);
138+
137139
byte[] passwordBytes = password.getBytes(StandardCharsets.UTF_8);
138-
byte[] nonceBytes = Optional.ofNullable(serverNonce.bytes()).orElse(new byte[0]);
140+
byte[] nonceBytes = serverNonce.bytesOrEmpty();
139141

140142
ByteBuf buffer = Unpooled.buffer();
141143

‎opc-ua-sdk/sdk-client/src/main/java/org/eclipse/milo/opcua/sdk/client/api/identity/X509IdentityProvider.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.eclipse.milo.opcua.stack.core.types.structured.SignatureData;
2323
import org.eclipse.milo.opcua.stack.core.types.structured.UserTokenPolicy;
2424
import org.eclipse.milo.opcua.stack.core.types.structured.X509IdentityToken;
25+
import org.eclipse.milo.opcua.stack.core.util.NonceUtil;
2526
import org.eclipse.milo.opcua.stack.core.util.SignatureUtil;
2627
import org.slf4j.Logger;
2728
import org.slf4j.LoggerFactory;
@@ -67,6 +68,8 @@ public SignedIdentityToken getIdentityToken(EndpointDescription endpoint,
6768
logger.warn("Error parsing SecurityPolicy for uri={}", securityPolicyUri);
6869
}
6970

71+
NonceUtil.validateNonce(serverNonce);
72+
7073
X509IdentityToken token = new X509IdentityToken(
7174
policyId,
7275
ByteString.of(certificate.getEncoded())

‎opc-ua-sdk/sdk-client/src/main/java/org/eclipse/milo/opcua/sdk/client/session/SessionFsmFactory.java‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -873,18 +873,20 @@ private static CompletableFuture<OpcUaSession> activateSession(
873873
try {
874874
EndpointDescription endpoint = client.getConfig().getEndpoint();
875875

876-
ByteString serverNonce = csr.getServerNonce();
876+
ByteString csrNonce = csr.getServerNonce();
877+
878+
NonceUtil.validateNonce(csrNonce);
877879

878880
SignedIdentityToken signedIdentityToken =
879881
client.getConfig().getIdentityProvider()
880-
.getIdentityToken(endpoint, serverNonce);
882+
.getIdentityToken(endpoint, csrNonce);
881883

882884
UserIdentityToken userIdentityToken = signedIdentityToken.getToken();
883885
SignatureData userTokenSignature = signedIdentityToken.getSignature();
884886

885887
ActivateSessionRequest request = new ActivateSessionRequest(
886888
client.newRequestHeader(csr.getAuthenticationToken()),
887-
buildClientSignature(client.getConfig(), serverNonce),
889+
buildClientSignature(client.getConfig(), csrNonce),
888890
new SignedSoftwareCertificate[0],
889891
new String[0],
890892
ExtensionObject.encode(client.getSerializationContext(), userIdentityToken),
@@ -896,19 +898,27 @@ private static CompletableFuture<OpcUaSession> activateSession(
896898
return stackClient.sendRequest(request)
897899
.thenApply(ActivateSessionResponse.class::cast)
898900
.thenCompose(asr -> {
899-
OpcUaSession session = new OpcUaSession(
900-
csr.getAuthenticationToken(),
901-
csr.getSessionId(),
902-
client.getConfig().getSessionName().get(),
903-
csr.getRevisedSessionTimeout(),
904-
csr.getMaxRequestMessageSize(),
905-
csr.getServerCertificate(),
906-
csr.getServerSoftwareCertificates()
907-
);
901+
try {
902+
ByteString asrNonce = asr.getServerNonce();
903+
904+
NonceUtil.validateNonce(asrNonce);
905+
906+
OpcUaSession session = new OpcUaSession(
907+
csr.getAuthenticationToken(),
908+
csr.getSessionId(),
909+
client.getConfig().getSessionName().get(),
910+
csr.getRevisedSessionTimeout(),
911+
csr.getMaxRequestMessageSize(),
912+
csr.getServerCertificate(),
913+
csr.getServerSoftwareCertificates()
914+
);
908915

909-
session.setServerNonce(asr.getServerNonce());
916+
session.setServerNonce(asrNonce);
910917

911-
return completedFuture(session);
918+
return completedFuture(session);
919+
} catch (UaException e) {
920+
return failedFuture(e);
921+
}
912922
});
913923
} catch (Exception ex) {
914924
return failedFuture(ex);

‎opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/SessionManager.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,7 @@ public void onCreateSession(ServiceRequest serviceRequest) throws UaException {
187187

188188
ByteString clientNonce = request.getClientNonce();
189189

190-
if (clientNonce.isNotNull() && (clientNonce.length() < 32)) {
191-
throw new UaException(StatusCodes.Bad_NonceInvalid);
192-
}
190+
NonceUtil.validateNonce(clientNonce);
193191

194192
if (securityPolicy != SecurityPolicy.None && clientNonces.contains(clientNonce)) {
195193
throw new UaException(StatusCodes.Bad_NonceInvalid);

‎opc-ua-stack/stack-client/src/main/java/org/eclipse/milo/opcua/stack/client/transport/uasc/UascClientMessageHandler.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -532,6 +532,8 @@ public void onMessageDecoded(ByteBuf message, long requestId) {
532532
secureChannel.setChannelId(oscr.getSecurityToken().getChannelId().longValue());
533533
logger.debug("Received OpenSecureChannelResponse.");
534534

535+
NonceUtil.validateNonce(oscr.getServerNonce(), secureChannel.getSecurityPolicy());
536+
535537
installSecurityToken(ctx, oscr);
536538

537539
handshakeFuture.complete(secureChannel);

‎opc-ua-stack/stack-core/src/main/java/org/eclipse/milo/opcua/stack/core/util/NonceUtil.java‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,22 @@
1616
import java.util.concurrent.TimeUnit;
1717
import java.util.concurrent.atomic.AtomicReference;
1818

19+
import org.bouncycastle.util.Arrays;
20+
import org.eclipse.milo.opcua.stack.core.StatusCodes;
21+
import org.eclipse.milo.opcua.stack.core.UaException;
1922
import org.eclipse.milo.opcua.stack.core.security.SecurityPolicy;
2023
import org.eclipse.milo.opcua.stack.core.types.builtin.ByteString;
2124
import org.slf4j.LoggerFactory;
2225

2326
public class NonceUtil {
2427

28+
/**
29+
* The minimum required nonce length for validation by {@link NonceUtil#validateNonce(ByteString)}.
30+
* <p>
31+
* This is specified by OPC UA Part 4 to be least 32 bytes in the CreateSession and ActivateSession services.
32+
*/
33+
public static final int MINIMUM_NONCE_LENGTH = 32;
34+
2535
private static volatile boolean SECURE_RANDOM_ENABLED = true;
2636

2737
private static final AtomicReference<SecureRandom> SECURE_RANDOM = new AtomicReference<>();
@@ -94,6 +104,9 @@ public static ByteString generateNonce(int length) {
94104
* <p>
95105
* The length is determined by the policy: see {@link #getNonceLength(SecurityPolicy)}.
96106
*
107+
* <b>Important</b>: this should only be used to generate a nonce exchanged when establishing a secure channel.
108+
* The nonce used by sessions has different length requirements.
109+
*
97110
* @param securityPolicy the {@link SecurityPolicy} to use when determining the nonce length.
98111
* @return a nonce of the appropriate length for the given security policy.
99112
*/
@@ -111,7 +124,7 @@ public static ByteString generateNonce(SecurityPolicy securityPolicy) {
111124
* @param securityPolicy the {@link SecurityPolicy} in use.
112125
* @return the minimum nonce length for use with {@code securityPolicy}.
113126
*/
114-
public static int getNonceLength(SecurityPolicy securityPolicy) {
127+
private static int getNonceLength(SecurityPolicy securityPolicy) {
115128
switch (securityPolicy) {
116129
case Basic128Rsa15:
117130
return 16;
@@ -126,4 +139,50 @@ public static int getNonceLength(SecurityPolicy securityPolicy) {
126139
}
127140
}
128141

142+
/**
143+
* Validate that {@code nonce} meets the minimum length requirement for {@code securityPolicy} and is non-zeroes.
144+
* <p>
145+
* <b>Important</b>: this should only be used to validate the nonce exchanged when establishing a secure channel.
146+
* The nonce used by sessions has different length requirements.
147+
*
148+
* @param nonce the nonce to validate.
149+
* @param securityPolicy the {@link SecurityPolicy} dictating the minimum nonce length.
150+
* @throws UaException if nonce validation failed.
151+
*/
152+
public static void validateNonce(ByteString nonce, SecurityPolicy securityPolicy) throws UaException {
153+
validateNonce(nonce, getNonceLength(securityPolicy));
154+
}
155+
156+
/**
157+
* Validate that {@code nonce} is at least {@link NonceUtil#MINIMUM_NONCE_LENGTH} and is non-zeroes.
158+
*
159+
* @param nonce the nonce to validate.
160+
* @throws UaException if nonce validation failed.
161+
*/
162+
public static void validateNonce(ByteString nonce) throws UaException {
163+
validateNonce(nonce, MINIMUM_NONCE_LENGTH);
164+
}
165+
166+
/**
167+
* Validate that {@code nonce} is at least {@code minimumLength} and is non-zeroes.
168+
*
169+
* @param nonce the nonce to validate.
170+
* @param minimumLength the minimum required nonce length
171+
* @throws UaException if nonce validation failed.
172+
*/
173+
public static void validateNonce(ByteString nonce, int minimumLength) throws UaException {
174+
byte[] bs = nonce.bytesOrEmpty();
175+
176+
if (bs.length < minimumLength) {
177+
throw new UaException(
178+
StatusCodes.Bad_NonceInvalid,
179+
"nonce must be at least " + minimumLength + " bytes"
180+
);
181+
}
182+
183+
if (bs.length > 0 && Arrays.areAllZeroes(bs, 0, bs.length)) {
184+
throw new UaException(StatusCodes.Bad_NonceInvalid, "nonce must be non-zero");
185+
}
186+
}
187+
129188
}

‎opc-ua-stack/stack-core/src/test/java/org/eclipse/milo/opcua/stack/core/util/NonceUtilTest.java‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/*
2+
* Copyright (c) 2019 the Eclipse Milo Authors
3+
*
4+
* This program and the accompanying materials are made
5+
* available under the terms of the Eclipse Public License 2.0
6+
* which is available at https://www.eclipse.org/legal/epl-2.0/
7+
*
8+
* SPDX-License-Identifier: EPL-2.0
9+
*/
10+
11+
package org.eclipse.milo.opcua.stack.core.util;
12+
13+
import java.util.Arrays;
14+
15+
import org.eclipse.milo.opcua.stack.core.UaException;
16+
import org.eclipse.milo.opcua.stack.core.security.SecurityPolicy;
17+
import org.eclipse.milo.opcua.stack.core.types.builtin.ByteString;
18+
import org.testng.annotations.Test;
19+
20+
import static org.testng.Assert.assertEquals;
21+
import static org.testng.Assert.assertThrows;
22+
23+
public class NonceUtilTest {
24+
25+
@Test
26+
public void testSecureChannelNonceGeneration() {
27+
for (SecurityPolicy securityPolicy : SecurityPolicy.values()) {
28+
ByteString nonce = NonceUtil.generateNonce(securityPolicy);
29+
30+
switch (securityPolicy) {
31+
case None:
32+
assertEquals(nonce.length(), 0);
33+
break;
34+
case Basic128Rsa15:
35+
assertEquals(nonce.length(), 16);
36+
break;
37+
default:
38+
assertEquals(nonce.length(), 32);
39+
}
40+
}
41+
}
42+
43+
@Test
44+
public void testNonceGeneration() throws UaException {
45+
for (int i = 32; i < 256; i++) {
46+
ByteString nonce = NonceUtil.generateNonce(i);
47+
assertEquals(nonce.length(), i);
48+
NonceUtil.validateNonce(nonce);
49+
}
50+
}
51+
52+
@Test
53+
public void testShortNonceThrows() {
54+
ByteString nonce = NonceUtil.generateNonce(NonceUtil.MINIMUM_NONCE_LENGTH - 1);
55+
56+
assertThrows(() -> NonceUtil.validateNonce(nonce));
57+
}
58+
59+
@Test
60+
public void testNonceOfZeroesThrows() {
61+
byte[] bs = new byte[NonceUtil.MINIMUM_NONCE_LENGTH];
62+
Arrays.fill(bs, (byte) 0);
63+
ByteString nonce = ByteString.of(bs);
64+
65+
assertThrows(() -> NonceUtil.validateNonce(nonce));
66+
}
67+
68+
@Test
69+
public void testZeroLengthNonceValidation() throws UaException {
70+
// an empty nonce is valid in the secure channel layer when no security is used.
71+
NonceUtil.validateNonce(ByteString.of(new byte[0]), 0);
72+
}
73+
74+
}

‎opc-ua-stack/stack-server/src/main/java/org/eclipse/milo/opcua/stack/server/transport/uasc/UascServerAsymmetricHandler.java‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,13 @@
5858
import org.eclipse.milo.opcua.stack.core.types.structured.ResponseHeader;
5959
import org.eclipse.milo.opcua.stack.core.util.BufferUtil;
6060
import org.eclipse.milo.opcua.stack.core.util.EndpointUtil;
61+
import org.eclipse.milo.opcua.stack.core.util.NonceUtil;
6162
import org.eclipse.milo.opcua.stack.server.UaStackServer;
6263
import org.slf4j.Logger;
6364
import org.slf4j.LoggerFactory;
6465

6566
import static org.eclipse.milo.opcua.stack.core.types.builtin.unsigned.Unsigned.uint;
6667
import static org.eclipse.milo.opcua.stack.core.util.NonceUtil.generateNonce;
67-
import static org.eclipse.milo.opcua.stack.core.util.NonceUtil.getNonceLength;
6868

6969
public class UascServerAsymmetricHandler extends ByteToMessageDecoder implements HeaderDecoder {
7070

@@ -434,17 +434,7 @@ private OpenSecureChannelResponse openSecureChannel(
434434
// Validate the remote nonce; it must be non-null and the correct length for the security algorithm.
435435
ByteString remoteNonce = request.getClientNonce();
436436

437-
if (remoteNonce == null || remoteNonce.isNull()) {
438-
throw new UaException(StatusCodes.Bad_SecurityChecksFailed, "remote nonce must be non-null");
439-
}
440-
441-
if (remoteNonce.length() < getNonceLength(secureChannel.getSecurityPolicy())) {
442-
String message = String.format(
443-
"remote nonce length must be at least %d bytes",
444-
getNonceLength(secureChannel.getSecurityPolicy()));
445-
446-
throw new UaException(StatusCodes.Bad_SecurityChecksFailed, message);
447-
}
437+
NonceUtil.validateNonce(remoteNonce, secureChannel.getSecurityPolicy());
448438

449439
ByteString localNonce = generateNonce(secureChannel.getSecurityPolicy());
450440

0 commit comments

Comments
 (0)