Security implementation broken

ST-DDT commented 4 years ago

The security implementation (#155) as released in v4.0.0 is vulnerable to concurrency issues.

Impact: high (only affects authenticated users, loss/gain of random authentication)
Reproducibility: easy (two concurrent calls, especially streaming calls)

Do NOT use that feature.

(Originally posted in https://github.com/LogNet/grpc-spring-boot-starter/pull/155#issuecomment-694340123 . I moved it here for better visibility)

Proof

Here a JUnit test that can be used to verify the issue (Click to expand)

````java package org.lognet.springboot.grpc.auth; import static org.junit.Assert.assertThrows; import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; import org.junit.runner.RunWith; import org.lognet.springboot.grpc.GrpcServerTestBase; import org.lognet.springboot.grpc.demo.DemoApp; import org.lognet.springboot.grpc.security.AuthCallCredentials; import org.lognet.springboot.grpc.security.AuthHeader; import org.lognet.springboot.grpc.security.EnableGrpcSecurity; import org.lognet.springboot.grpc.security.GrpcSecurity; import org.lognet.springboot.grpc.security.GrpcSecurityConfigurerAdapter; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.boot.test.context.TestConfiguration; import org.springframework.context.annotation.Import; import org.springframework.security.authentication.dao.DaoAuthenticationProvider; import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.userdetails.User; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.crypto.password.NoOpPasswordEncoder; import org.springframework.security.provisioning.InMemoryUserDetailsManager; import org.springframework.test.context.junit4.SpringRunner; import com.google.protobuf.Empty; import io.grpc.Status; import io.grpc.StatusRuntimeException; import io.grpc.examples.SecuredGreeterGrpc; import io.grpc.examples.GreeterGrpc.GreeterFutureStub; import lombok.extern.slf4j.Slf4j; @SpringBootTest(classes = DemoApp.class, properties = "spring.cloud.service-registry.auto-registration.enabled=false") @RunWith(SpringRunner.class) @Import({ ConcurrentAuthConfigTest.TestCfg.class }) @Slf4j public class ConcurrentAuthConfigTest extends GrpcServerTestBase { private AuthCallCredentials callCredentials = new AuthCallCredentials( AuthHeader.builder().basic("test", "test".getBytes())); @TestConfiguration static class TestCfg { @EnableGrpcSecurity public class DemoGrpcSecurityConfig extends GrpcSecurityConfigurerAdapter { @Override public void configure(GrpcSecurity builder) throws Exception { DaoAuthenticationProvider provider = new DaoAuthenticationProvider(); List roles = Arrays.asList(new SimpleGrantedAuthority("ROLE_TEST")); UserDetailsService users = new InMemoryUserDetailsManager(new User("test", "test", roles)); provider.setUserDetailsService(users); provider.setPasswordEncoder(NoOpPasswordEncoder.getInstance()); builder.authenticationProvider(provider); builder.authorizeRequests().anyMethod().authenticated(); } } } @Test public void concurrentTest() throws InterruptedException { System.out.println(); final SecuredGreeterGrpc.SecuredGreeterBlockingStub unsecuredFutureStub = SecuredGreeterGrpc .newBlockingStub(selectedChanel); final SecuredGreeterGrpc.SecuredGreeterBlockingStub securedFutureStub = unsecuredFutureStub .withCallCredentials(callCredentials); int parallelTests = 10; List threads = new ArrayList<>(); // Number of threads that passed the test AtomicInteger successCounter = new AtomicInteger(0); AtomicInteger failureCounter = new AtomicInteger(0); Runnable authenticated = () -> { securedFutureStub.sayAuthHello(Empty.getDefaultInstance()).getMessage(); // log.warn("Call passed"); }; Runnable unauthenticated = () -> { StatusRuntimeException err = assertThrows(StatusRuntimeException.class, () -> unsecuredFutureStub.sayAuthHello(Empty.getDefaultInstance()).getMessage()); assertEquals(Status.Code.UNAUTHENTICATED, err.getStatus().getCode()); // log.warn("Call blocked"); }; // Check that the assertions work as is (single threaded) authenticated.run(); unauthenticated.run(); for (int i = 0; i < parallelTests; i++) { Thread success = new Thread(() -> { for (int j = 0; j < 1000; j++) { authenticated.run(); } successCounter.incrementAndGet(); log.info("All passed"); }); success.setUncaughtExceptionHandler((thread, ex) -> { log.error("SECURITY ???", ex); }); threads.add(success); Thread failure = new Thread(() -> { for (int j = 0; j < 1000; j++) { unauthenticated.run(); } failureCounter.incrementAndGet(); log.info("All passed"); }); failure.setUncaughtExceptionHandler((thread, ex) -> { log.error("SECURITY BYPASSED", ex); }); threads.add(failure); } Collections.shuffle(threads); for (Thread thread : threads) { thread.start(); } for (Thread thread : threads) { thread.join(); } assertAll(() -> assertEquals(parallelTests, successCounter.get()), () -> assertEquals(parallelTests, failureCounter.get())); } @Override protected GreeterFutureStub beforeGreeting(GreeterFutureStub stub) { return stub.withCallCredentials(callCredentials); } } ````

You will encounter the following errors in the test:

java.lang.NullPointerException: null // (Current authentication instance in authenticated call)
    at org.lognet.springboot.grpc.demo.SecuredGreeterService.sayAuthHello(SecuredGreeterService.java:22) ~[main/:na]
    at io.grpc.examples.SecuredGreeterGrpc$MethodHandlers.invoke(SecuredGreeterGrpc.java:209) ~[main/:na]
    at io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:172) ~[grpc-stub-1.29.0.jar:1.29.0]
    at java.base/java.util.Optional.ifPresent(Optional.java:183) ~[na:na]
    at org.lognet.springboot.grpc.security.SecurityInterceptor$SecurityServerCallListener.onHalfClose(SecurityInterceptor.java:43) ~[main/:na]
    at io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:331) ~[grpc-core-1.29.0.jar:1.29.0]
    at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:818) ~[grpc-core-1.29.0.jar:1.29.0]
    at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37) ~[grpc-core-1.29.0.jar:1.29.0]
    at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:123) ~[grpc-core-1.29.0.jar:1.29.0]
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) ~[na:na]
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) ~[na:na]
    at java.base/java.lang.Thread.run(Thread.java:834) ~[na:na]

This error is caused by another call that completed and thus the security being cleared before this call was processed. However, it is also possible that if two authenticated calls were to happen at the same time to gain the authentication of the other call/user.

Note

Due to the structure of this library's interceptor you cannot (not sure) bypass authentication, but can still obtain a different users authentication (or loose your own as shown in the above stacktrace). Especially for streaming calls.

EDIT: You can also bypass security annotations (in non-streaming calls only) if the server uses them instead of the GrpcSecurityConfigurerAdapter (Not tested, but it's the same as the above error).

  public static <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
        Context context,
        ServerCall<ReqT, RespT> call,
        Metadata headers,
        ServerCallHandler<ReqT, RespT> next) {
    Context previous = context.attach();
    try {
      return new ContextualizedServerCallListener<>(
          next.startCall(call, headers),
          context);
    } finally {
      context.detach(previous);
    }
  }

https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/Contexts.java#L72-L80

    @Override
    public void onMessage(ReqT message) {
      Context previous = context.attach();
      try {
        super.onMessage(message);
      } finally {
        context.detach(previous);
      }
    }

As you can see the context is cleared after the method call and reattached and cleared for the nested calls.

jvmlet commented 4 years ago

This is what I meant - Authentication will be passed via managed context

ST-DDT commented 4 years ago

I'm not sure whether i got you right or whether you got me right. What is important is not that it uses Spring's SecurityContextHolder or grpc's Context, but that it gets set at the start of startCall, onMessage, `onHalfClose, ... and cleared before existing each of them.

markbanierink commented 3 years ago

I'm not sure if I have configured it correctly, but now with the implementation resolving this issue, the security context is cleared after returning the listener. When using spring security in our domain services (@PreAuthorize for instance), there is no authentication set anymore in the SecurityContextHolder, since these are called after the security context is cleared in the finally block of the SecurityInterceptor.

Is this deliberately implemented this way?
Should we implement our own GrpcInterceptor return a listener that sets the authentication in security context (again)?

jvmlet commented 3 years ago

SecurityContextHolder was never exposed to the end user to obtain currently logged in user. It's grpc security internal implementation. Please use GrpcSecurity.AUTHENTICATION_CONTEXT_KEY.get() to get the current user. @PreAuthorize is not supported yet.

markbanierink commented 3 years ago

Thank you for the quick response. So, in order to use spring security features further down in our domain services, we have to add an interceptor like this and set the security context ourselves:

public class AuthenticationInterceptor implements ServerInterceptor {

    @Override
    public <ReqT, RespT> Listener<ReqT> interceptCall(ServerCall<ReqT, RespT> call, Metadata headers, ServerCallHandler<ReqT, RespT> next) {

        return new SimpleForwardingServerCallListener<ReqT>(next.startCall(call, headers)) {
            @Override
            public void onHalfClose() {
                try {
                    SecurityContextHolder.getContext().setAuthentication((Authentication)GrpcSecurity.AUTHENTICATION_CONTEXT_KEY.get());
                    super.onHalfClose();
                } finally {
                    SecurityContextHolder.clearContext();
                }
            }
        };
    }   
}

Just for me to understand, but why wasn't something like this implemented in the SecurityInterceptor?

jvmlet commented 3 years ago

SecurityContextHolder usesThreadLocal as repository for storing the Authentication object. While this approach might fit the servlet base MVC implementation where filters are executed on the same thread, grpc java doesn't guarantee this when executing interceptors and listeners. Instead it provides Contextualized listener if I recall properly that gets attached and detached before/after invoking the listener. It might be using the same thread local to store the context, but this is transparent for the user as long as he uses Context API and not ThreadLocal directly. BTW, here we can see how bad the static functions could be affecting the flexibility. I would prefer to see the accessor pattern for accessing the security context instead of static function you can't override/substitute. Having said this, I felt that cleaning the security context right after authenticating the user would be the right thing to do to not compromise the executing thread.

Which security features you are talking about further down in your services?

markbanierink commented 3 years ago

We currently use @PreAuthorize and @PostFilter "further down" in our beans. These annotations use the SecurityContextHolder, hence our need to set the Authentication there. We preferably don't touch the SecurityContextHolder ourselves, spring security handles all this for us, setting the Authentication by the AuthenticationProvider. Methods annotated with these annotations perform our business logic and are called from the api-layer (gRPC, but possibly also REST, GraphQL, etc.), so it is inconvenient to secure our gRPC services, since we obviously prefer to do this in a single place at the domain level. With regard to the interceptors possibly running in another thread, I agree cleaning the context after authentication is the right thing to do. Maybe something like our implementation (probably extended for onMessage(), etc.) could be added to the default grpc security configuration, so spring security can be used by default?

jvmlet commented 3 years ago

Sure, I'll try to accomplish this once I'm back from vacation( ~ end of July) as well as @PreAuthorize and @PostFilter integration.

markbanierink commented 3 years ago

If the Authentication is added to the SecurityContextHolder, then @PreAuthorize etc. automatically works through spring security.

markbanierink commented 3 years ago

I have created https://github.com/LogNet/grpc-spring-boot-starter/pull/233 as a possible implementation.

LogNet / grpc-spring-boot-starter

Security implementation broken #157

Proof

Note

See also