GRPC with TLS mutual authentication: Server accepts untrusted client certificate

[matthild]

A Java GRPC server configured to require client authentication appears to accept client certificates issued by CAs that the server does not trust. If so, this is a dangerous security vulnerability.

Here is a minimal project that reproduces the problem. Curiously, pretty much exactly the same code works correctly when part of the grpc-java repository's test suites; see the tests added in this pull request.

Perhaps I'm simply missing something; perhaps there is a genuine bug; or perhaps there is some subtle interaction of JARs or whatnot that causes this code to work in some environments but to be silently dangerous in others.

Output of the minimal project is below. This output is from Java 1.8.0_65 on Mac OS X (El Capitan); the test fails in the same way on Linux (Debian 8). The output shows that the SSL engine in use is OpenSSL.

$ ./run_test 
Netty can use OpenSSL: true
Running test...
Jan 16, 2016 9:51:02 PM io.grpc.internal.TransportSet$1 run
INFO: Created transport io.grpc.netty.NettyClientTransport@1e7c7811(localhost/127.0.0.1:64417) for localhost/127.0.0.1:64417
Jan 16, 2016 9:51:02 PM io.grpc.internal.TransportSet$TransportListener transportReady
INFO: Transport io.grpc.netty.NettyClientTransport@1e7c7811(localhost/127.0.0.1:64417) for localhost/127.0.0.1:64417 is ready
Exception in thread "main" Jan 16, 2016 9:51:02 PM io.grpc.internal.TransportSet$TransportListener transportShutdown
INFO: Transport io.grpc.netty.NettyClientTransport@1e7c7811(localhost/127.0.0.1:64417) for localhost/127.0.0.1:64417 is being shutdown
Jan 16, 2016 9:51:02 PM io.grpc.internal.TransportSet$TransportListener transportTerminated
INFO: Transport io.grpc.netty.NettyClientTransport@1e7c7811(localhost/127.0.0.1:64417) for localhost/127.0.0.1:64417 is terminated
java.lang.AssertionError: TLS handshake should have failed, but didn't; received RPC response: text: "Request said: dummy text"

    at org.junit.Assert.fail(Assert.java:88)
    at io.grpc.testing.integration.TlsTest.serverRejectsUntrustedClientCert(TlsTest.java:84)
    at RunTest.main(RunTest.java:14)

[ejona86]

I don't believe the server is accepting the client's certificate. Instead, I believe that the server configuration to require a client cert is not functioning. I see the same behavior if the client doesn't provide any client certificate.

[matthild]

Hm, interesting -- thanks. One data point is that the noClientAuthFailure test added in PR #1327 passes, so the server configuration to require a client cert can function in some situations, at least.

[matthild]

In light of the fix in PR #1334, it seems that you are right.

I looked a bit deeper into what's happening here. Calling clientAuth(ClientAuth.REQUIRE) ultimately causes ClientAuth.REQUIRE to be propagated to Netty's OpenSslEngine constructor, which stores ClientAuth.REQUIRE in the clientAuth member but does not pass it to tcnative. The setClientAuth method in the same class contains code for informing tcnative, but nothing calls setClientAuth.

I tweaked the OpenSslEngine constructor to change

this.clientAuth = clientMode ? ClientAuth.NONE : checkNotNull(clientAuth, "clientAuth");
ssl = SSL.newSSL(sslCtx, !clientMode);

to

ssl = SSL.newSSL(sslCtx, !clientMode);
setClientAuth(clientMode ? ClientAuth.NONE : checkNotNull(clientAuth, "clientAuth"));

(swapping the order because setClientAuth depends on ssl being initialized already). I rebuilt Netty, replaced the standard netty-handler-4.1.0.Beta8.jar with the newly built one, and the TLS handshake now fails (as it should). The client sees an io.grpc.StatusRuntimeException: UNAVAILABLE caused by a java.nio.channels.ClosedChannelException.

I'll see about contributing the fix to Netty.

[nmittler]

FYI @Scottmitch @normanmaurer (from Netty)

@matthild good catch, and thanks for investigating!

Looks like a pretty easy fix. I can roll a PR if that's easier for you ... either way.

[normanmaurer]

@nmittler this was already fixed:

netty/netty@eb577c5

;)

[nmittler]

@normanmaurer even better! Has that change been released?

[normanmaurer]

@nmittler nope...today :)

[nmittler]

@normanmaurer sounds good! I'll upgrade grpc as soon as it's available. Thanks!

[ejona86]

@normanmaurer, thanks! And I didn't see the bug when I looked at OpenSslEngine because it was already fixed! (I was looking at those same lines)

[ejona86]

This should be solved by #1367, but I'll leave it open as a reminder to remove the assume()s in TlsTest so that it will be verified in the unit tests.

[carl-mastrangelo]

Is there anything more to do here, now that #1367 is fixed?