andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Correctly synchronize before trying to set key material to fix possible native crash (#9566)

Browse Source 
Motivation:

When using io.netty.handler.ssl.openssl.useTasks=true we may call ReferenceCountedOpenSslEngine.setKeyMaterial(...) from another thread and so need to synchronize and also check if the engine was destroyed in the meantime to eliminate of the possibility of a native crash.
The same is try when trying to access the authentication methods.

Modification:

- Add synchronized and isDestroyed() checks where missing
- Add null checks for the case when a callback is executed by another thread after the engine was destroyed already
- Move code for master key extraction to ReferenceCountedOpenSslEngine to ensure there can be no races.

Result:

No native crash possible anymore when using io.netty.handler.ssl.openssl.useTasks=true
This commit is contained in:
Norman Maurer 2019-09-16 11:14:08 +02:00
parent f44e424655
commit cda8ee95b3
6 changed files with 52 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java
View File

@ -15,8 +15,6 @@
*/
package io.netty.handler.ssl;
import io.netty.internal.tcnative.SSL;
import javax.net.ssl.SSLException;
import javax.net.ssl.X509ExtendedKeyManager;
import javax.net.ssl.X509KeyManager;
@ -66,15 +64,19 @@ final class OpenSslKeyMaterialManager {
}
void setKeyMaterialServerSide(ReferenceCountedOpenSslEngine engine) throws SSLException {
long ssl = engine.sslPointer();
String[] authMethods = SSL.authenticationMethods(ssl);
String[] authMethods = engine.authMethods();
if (authMethods.length == 0) {
return;
}
Set<String> aliases = new HashSet<>(authMethods.length);
for (String authMethod : authMethods) {
String type = KEY_TYPES.get(authMethod);
if (type != null) {
String alias = chooseServerAlias(engine, type);
if (alias != null && aliases.add(alias)) {
setKeyMaterial(engine, alias);
if (!setKeyMaterial(engine, alias)) {
return;
}
}
}
}
@ -91,13 +93,11 @@ final class OpenSslKeyMaterialManager {
}
}
private void setKeyMaterial(ReferenceCountedOpenSslEngine engine, String alias) throws SSLException {
private boolean setKeyMaterial(ReferenceCountedOpenSslEngine engine, String alias) throws SSLException {
OpenSslKeyMaterial keyMaterial = null;
try {
keyMaterial = provider.chooseKeyMaterial(engine.alloc, alias);
if (keyMaterial != null) {
engine.setKeyMaterial(keyMaterial);
}
return keyMaterial == null || engine.setKeyMaterial(keyMaterial);
} catch (SSLException e) {
throw e;
} catch (Exception e) {

4
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java
View File

@ -261,6 +261,10 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted
@Override
public void handle(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) throws Exception {
final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl);
// May be null if it was destroyed in the meantime.
if (engine == null) {
return;
}
try {
final Set<String> keyTypesSet = supportedClientKeyTypes(keyTypeBytes);
final String[] keyTypes = keyTypesSet.toArray(new String[0]);

6
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java
View File

@ -679,8 +679,12 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
@Override
public final int verify(long ssl, byte[][] chain, String auth) {
X509Certificate[] peerCerts = certificates(chain);
final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl);
if (engine == null) {
// May be null if it was destroyed in the meantime.
return CertificateVerifier.X509_V_ERR_UNSPECIFIED;
}
X509Certificate[] peerCerts = certificates(chain);
try {
verify(engine, peerCerts, auth);
return CertificateVerifier.X509_V_OK;

32
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
View File

@ -48,6 +48,7 @@ import java.util.Set;
import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
import java.util.concurrent.locks.Lock;
import javax.crypto.spec.SecretKeySpec;
import javax.net.ssl.SSLEngine;
import javax.net.ssl.SSLEngineResult;
import javax.net.ssl.SSLException;
@ -366,9 +367,29 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
leak = leakDetection ? leakDetector.track(this) : null;
}
final void setKeyMaterial(OpenSslKeyMaterial keyMaterial) throws Exception {
SSL.setKeyMaterial(ssl, keyMaterial.certificateChainAddress(), keyMaterial.privateKeyAddress());
final synchronized String[] authMethods() {
if (isDestroyed()) {
return EmptyArrays.EMPTY_STRINGS;
}
return SSL.authenticationMethods(ssl);
}
final boolean setKeyMaterial(OpenSslKeyMaterial keyMaterial) throws Exception {
synchronized (this) {
if (isDestroyed()) {
return false;
}
SSL.setKeyMaterial(ssl, keyMaterial.certificateChainAddress(), keyMaterial.privateKeyAddress());
}
localCertificateChain = keyMaterial.certificateChain();
return true;
}
final synchronized SecretKeySpec masterKey() {
if (isDestroyed()) {
return null;
}
return new SecretKeySpec(SSL.getMasterKey(ssl), "AES");
}
/**
@ -385,7 +406,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
}
synchronized (this) {
SSL.setOcspResponse(ssl, response);
if (!isDestroyed()) {
SSL.setOcspResponse(ssl, response);
}
}
}
@ -403,6 +426,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
}
synchronized (this) {
if (isDestroyed()) {
return EmptyArrays.EMPTY_BYTES;
}
return SSL.getOcspResponse(ssl);
}
}

4
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java
View File

@ -202,6 +202,10 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted
@Override
public void handle(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) throws Exception {
final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl);
if (engine == null) {
// Maybe null if destroyed in the meantime.
return;
}
try {
// For now we just ignore the asn1DerEncodedPrincipals as this is kind of inline with what the
// OpenJDK SSLEngineImpl does.

4
handler/src/main/java/io/netty/handler/ssl/SslMasterKeyHandler.java
View File

@ -18,7 +18,6 @@ package io.netty.handler.ssl;
import io.netty.buffer.ByteBufUtil;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelInboundHandlerAdapter;
import io.netty.internal.tcnative.SSL;
import io.netty.util.internal.ReflectionUtil;
import io.netty.util.internal.SystemPropertyUtil;
import io.netty.util.internal.logging.InternalLogger;
@ -140,8 +139,7 @@ public abstract class SslMasterKeyHandler extends ChannelInboundHandlerAdapter {
}
accept(secretKey, sslSession);
} else if (OpenSsl.isAvailable() && engine instanceof ReferenceCountedOpenSslEngine) {
SecretKeySpec secretKey = new SecretKeySpec(
SSL.getMasterKey(((ReferenceCountedOpenSslEngine) engine).sslPointer()), "AES");
SecretKeySpec secretKey = ((ReferenceCountedOpenSslEngine) engine).masterKey();
accept(secretKey, sslSession);
}
}