From 29bf0f396549d483ffed265eb53fc15538c2e08e Mon Sep 17 00:00:00 2001 From: Michael Muller Date: Fri, 5 Feb 2021 07:50:51 -0500 Subject: [PATCH] Make BiqueryPollJobAction endpoint internal only (#955) * Make BiqueryPollJobAction endpoint internal only This endpoint makes use of java object deserialization, which allows a malicious actor to craft a request that can initiate overly broad actions on the server. Since this endpoint is not widely used for operational purposes, limit its authorization to "internal only" so that no user agents (even with admin privs) can access it. --- .../export/BigqueryPollJobAction.java | 5 ++- .../google/registry/request/auth/Auth.java | 17 ++++--- .../registry/request/RequestHandlerTest.java | 44 +++++++++++++++++++ .../module/backend/backend_routing.txt | 2 +- 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/google/registry/export/BigqueryPollJobAction.java b/core/src/main/java/google/registry/export/BigqueryPollJobAction.java index a5a4aebbf..2877f24b7 100644 --- a/core/src/main/java/google/registry/export/BigqueryPollJobAction.java +++ b/core/src/main/java/google/registry/export/BigqueryPollJobAction.java @@ -45,13 +45,16 @@ import org.joda.time.Duration; /** * An action which polls the state of a bigquery job. If it is completed then it will log its * completion state; otherwise it will return a failure code so that the task will be retried. + * + *

Note that this is AUTH_INTERNAL_ONLY: we don't allow "admin" for this to mitigate a + * vulnerability, see b/177308043. */ @Action( service = Action.Service.BACKEND, path = BigqueryPollJobAction.PATH, method = {Action.Method.GET, Action.Method.POST}, automaticallyPrintOk = true, - auth = Auth.AUTH_INTERNAL_OR_ADMIN) + auth = Auth.AUTH_INTERNAL_ONLY) public class BigqueryPollJobAction implements Runnable { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); diff --git a/core/src/main/java/google/registry/request/auth/Auth.java b/core/src/main/java/google/registry/request/auth/Auth.java index d76dec6c3..30cf2a062 100644 --- a/core/src/main/java/google/registry/request/auth/Auth.java +++ b/core/src/main/java/google/registry/request/auth/Auth.java @@ -65,13 +65,18 @@ public enum Auth { AUTH_PUBLIC_OR_INTERNAL( ImmutableList.of(AuthMethod.INTERNAL, AuthMethod.API), AuthLevel.APP, UserPolicy.PUBLIC), - /** - * Allows only admins or App Engine task-queue access. - */ + /** Allows only admins or App Engine task-queue access. */ AUTH_INTERNAL_OR_ADMIN( - ImmutableList.of(AuthMethod.INTERNAL, AuthMethod.API), - AuthLevel.APP, - UserPolicy.ADMIN); + ImmutableList.of(AuthMethod.INTERNAL, AuthMethod.API), AuthLevel.APP, UserPolicy.ADMIN), + + /** + * Allows only App Engine task-queue access. + * + *

In general, prefer AUTH_INTERNAL_OR_ADMIN. This level of access should be reserved for + * endpoints that have some sensitivity (it was introduced to mitigate a remote-shell + * vulnerability). + */ + AUTH_INTERNAL_ONLY(ImmutableList.of(AuthMethod.INTERNAL), AuthLevel.APP, UserPolicy.IGNORED); private final AuthSettings authSettings; diff --git a/core/src/test/java/google/registry/request/RequestHandlerTest.java b/core/src/test/java/google/registry/request/RequestHandlerTest.java index 6169b0b2e..6fc29cb10 100644 --- a/core/src/test/java/google/registry/request/RequestHandlerTest.java +++ b/core/src/test/java/google/registry/request/RequestHandlerTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.POST; +import static google.registry.request.auth.Auth.AUTH_INTERNAL_ONLY; import static google.registry.request.auth.Auth.AUTH_INTERNAL_OR_ADMIN; import static google.registry.request.auth.Auth.AUTH_PUBLIC; import static org.mockito.ArgumentMatchers.any; @@ -140,6 +141,17 @@ public final class RequestHandlerTest { } } + @Action( + service = Action.Service.DEFAULT, + path = "/auth/internal", + auth = AUTH_INTERNAL_ONLY, + method = GET) + public class AuthInternalAction extends AuthBase { + AuthInternalAction(AuthResult authResult) { + super(authResult); + } + } + public class Component { private RequestModule requestModule = null; @@ -179,6 +191,10 @@ public final class RequestHandlerTest { public AuthAdminUserAction authAdminUserAction() { return new AuthAdminUserAction(component.getRequestModule().provideAuthResult()); } + + public AuthInternalAction authInternalAction() { + return new AuthInternalAction(component.getRequestModule().provideAuthResult()); + } } /** Fake Builder for the fake component above to satisfy RequestHandler expectations. */ @@ -462,4 +478,32 @@ public final class RequestHandlerTest { assertThat(providedAuthResult.userAuthInfo().get().oauthTokenInfo()).isEmpty(); assertMetric("/auth/adminUser", GET, AuthLevel.USER, true); } + + @Test + void testInternalAuthNeeded_failure() throws Exception { + when(req.getMethod()).thenReturn("GET"); + when(req.getRequestURI()).thenReturn("/auth/internal"); + when(requestAuthenticator.authorize(AUTH_INTERNAL_ONLY.authSettings(), req)) + .thenReturn(Optional.empty()); + + handler.handleRequest(req, rsp); + + verify(rsp).sendError(403, "Not authorized"); + assertThat(providedAuthResult).isNull(); + } + + @Test + void testInternalAuthNeeded_success() throws Exception { + when(req.getMethod()).thenReturn("GET"); + when(req.getRequestURI()).thenReturn("/auth/internal"); + when(requestAuthenticator.authorize(AUTH_INTERNAL_ONLY.authSettings(), req)) + .thenReturn(Optional.of(AuthResult.create(AuthLevel.APP))); + + handler.handleRequest(req, rsp); + + assertThat(providedAuthResult).isNotNull(); + assertThat(providedAuthResult.authLevel()).isEqualTo(AuthLevel.APP); + assertThat(providedAuthResult.userAuthInfo()).isEmpty(); + assertMetric("/auth/internal", GET, AuthLevel.APP, true); + } } diff --git a/core/src/test/resources/google/registry/module/backend/backend_routing.txt b/core/src/test/resources/google/registry/module/backend/backend_routing.txt index 6ee85fba4..f52e3d54a 100644 --- a/core/src/test/resources/google/registry/module/backend/backend_routing.txt +++ b/core/src/test/resources/google/registry/module/backend/backend_routing.txt @@ -24,7 +24,7 @@ PATH CLASS METHOD /_dr/task/icannReportingUpload IcannReportingUploadAction POST n INTERNAL,API APP ADMIN /_dr/task/nordnUpload NordnUploadAction POST y INTERNAL,API APP ADMIN /_dr/task/nordnVerify NordnVerifyAction POST y INTERNAL,API APP ADMIN -/_dr/task/pollBigqueryJob BigqueryPollJobAction GET,POST y INTERNAL,API APP ADMIN +/_dr/task/pollBigqueryJob BigqueryPollJobAction GET,POST y INTERNAL APP IGNORED /_dr/task/publishDnsUpdates PublishDnsUpdatesAction POST y INTERNAL,API APP ADMIN /_dr/task/publishInvoices PublishInvoicesAction POST n INTERNAL,API APP ADMIN /_dr/task/publishSpec11 PublishSpec11ReportAction POST n INTERNAL,API APP ADMIN