From dc88b4877238025f324774be01bdb2a364c12ba5 Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Tue, 23 Mar 2021 08:50:56 -0400 Subject: [PATCH] Disallow admin triggering of internal endpoints (#1030) * Disallow admin triggering of internal endpoints Stop simply relying on the presence of the X-AppEngine-QueueName as an indicator that an endpoint has been triggered internally, as this allows admins to trigger a remote execution vulnerability. We now supplement this check by ensuring that there is no authenticated user. Since only an admin user can set these headers, this means that the header must have been set by an internal request. Tested: In addition to the new unit test, verified on Crash that: - Internal requests are still getting authenticated via the internal auth mechanism. - Admin requests with the X-AppEngine-QueueName header are rejected as "unauthorized." * Reformatted. --- ...EngineInternalAuthenticationMechanism.java | 35 +++++++------------ .../auth/RequestAuthenticatorTest.java | 12 ++++++- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java b/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java index af79a48b7..35b44e266 100644 --- a/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java +++ b/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java @@ -17,6 +17,7 @@ package google.registry.request.auth; import static google.registry.request.auth.AuthLevel.APP; import static google.registry.request.auth.AuthLevel.NONE; +import com.google.appengine.api.users.UserService; import javax.inject.Inject; import javax.servlet.http.HttpServletRequest; @@ -24,29 +25,15 @@ import javax.servlet.http.HttpServletRequest; * Authentication mechanism which uses the X-AppEngine-QueueName header set by App Engine for * internal requests. * - *

- * Task queue push task requests set this header value to the actual queue name. Cron requests set - * this header value to __cron, since that's actually the name of the hidden queue used for cron + *

Task queue push task requests set this header value to the actual queue name. Cron requests + * set this header value to __cron, since that's actually the name of the hidden queue used for cron * requests. Cron also sets the header X-AppEngine-Cron, which we could check, but it's simpler just * to check the one. * - *

- * App Engine allows app admins to set these headers for testing purposes. This means that this auth - * method is somewhat unreliable - any app admin can access any internal endpoint and pretend to be - * the app itself by setting these headers, which would circumvent any finer-grained authorization - * if we added it in the future (assuming we did not apply it to the app itself). And App Engine's - * concept of an "admin" includes all project owners, editors and viewers. So anyone with access to - * the project will be able to access anything the app itself can access. - * - *

- * For now, it's probably okay to allow this behavior, especially since it could indeed be - * convenient for testing. If we wanted to revisit this decision in the future, we have a couple - * options for locking this down: - * - *

+ *

App Engine allows app admins to set these headers for testing purposes. Because of this, we + * also need to verify that the current user is null, indicating that there is no user, to prevent + * access by someone with "admin" privileges. If the user is an admin, UserService presumably must + * return a User object. * *

See task @@ -57,12 +44,16 @@ public class AppEngineInternalAuthenticationMechanism implements AuthenticationM // As defined in the App Engine request header documentation. private static final String QUEUE_NAME_HEADER = "X-AppEngine-QueueName"; + private UserService userService; + @Inject - public AppEngineInternalAuthenticationMechanism() {} + public AppEngineInternalAuthenticationMechanism(UserService userService) { + this.userService = userService; + } @Override public AuthResult authenticate(HttpServletRequest request) { - if (request.getHeader(QUEUE_NAME_HEADER) == null) { + if (request.getHeader(QUEUE_NAME_HEADER) == null || userService.getCurrentUser() != null) { return AuthResult.create(NONE); } else { return AuthResult.create(APP); diff --git a/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java b/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java index 0cb443cbd..5211a67a8 100644 --- a/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java +++ b/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java @@ -117,7 +117,7 @@ class RequestAuthenticatorTest { private RequestAuthenticator createRequestAuthenticator(UserService userService) { return new RequestAuthenticator( - new AppEngineInternalAuthenticationMechanism(), + new AppEngineInternalAuthenticationMechanism(fakeUserService), ImmutableList.of( new OAuthAuthenticationMechanism( fakeOAuthService, @@ -173,6 +173,16 @@ class RequestAuthenticatorTest { assertThat(authResult.get().userAuthInfo()).isEmpty(); } + @Test + void testInternalAuth_failForAdminUser() { + when(req.getHeader("X-AppEngine-QueueName")).thenReturn("__cron"); + fakeUserService.setUser(testUser, true /* isAdmin */); + + Optional authResult = runTest(fakeUserService, AUTH_INTERNAL_OR_ADMIN); + + assertThat(authResult).isEmpty(); + } + @Test void testAnyUserAnyMethod_notLoggedIn() { Optional authResult = runTest(fakeUserService, AUTH_ANY_USER_ANY_METHOD);