From 5847300290fbf66a16c9eea4f94f30d0d78c5af6 Mon Sep 17 00:00:00 2001 From: Jonathan Klabunde Tomer <125505367+jkt-signal@users.noreply.github.com> Date: Wed, 12 Jul 2023 11:45:02 -0700 Subject: [PATCH] Revert "Allow use of the token returned with spam challenges as auth for the challenge verification request" --- service/config/sample-secrets-bundle.yml | 2 - service/config/sample.yml | 4 - .../WhisperServerConfiguration.java | 10 - .../textsecuregcm/WhisperServerService.java | 7 +- .../configuration/ChallengeConfiguration.java | 12 -- .../controllers/ChallengeController.java | 27 +-- .../entities/RateLimitChallenge.java | 2 - .../util/ChallengeTokenBlinder.java | 109 ---------- .../controllers/ChallengeControllerTest.java | 198 +----------------- 9 files changed, 8 insertions(+), 363 deletions(-) delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/configuration/ChallengeConfiguration.java delete mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/util/ChallengeTokenBlinder.java diff --git a/service/config/sample-secrets-bundle.yml b/service/config/sample-secrets-bundle.yml index f4da42916..b80b7a4a4 100644 --- a/service/config/sample-secrets-bundle.yml +++ b/service/config/sample-secrets-bundle.yml @@ -61,8 +61,6 @@ cdn.accessSecret: test # AWS Access Secret unidentifiedDelivery.certificate: ABCD1234 unidentifiedDelivery.privateKey: ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789AAAAAAA -challengeToken.blindingSecret: c3VwZXIgc2VjcmV0IGtleQ== - hCaptcha.apiKey: unset storageService.userAuthenticationTokenSharedSecret: AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= diff --git a/service/config/sample.yml b/service/config/sample.yml index 94547b686..cb4a21d14 100644 --- a/service/config/sample.yml +++ b/service/config/sample.yml @@ -230,10 +230,6 @@ unidentifiedDelivery: privateKey: secret://unidentifiedDelivery.privateKey expiresDays: 7 -challenge: - blindingSecret: secret://challengeToken.blindingSecret - tokenTtl: PT10M - recaptcha: projectPath: projects/example credentialConfigurationJson: "{ }" # service account configuration for backend authentication diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java index 79e8bba60..1e4599027 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerConfiguration.java @@ -23,7 +23,6 @@ import org.whispersystems.textsecuregcm.configuration.AwsAttachmentsConfiguratio import org.whispersystems.textsecuregcm.configuration.BadgesConfiguration; import org.whispersystems.textsecuregcm.configuration.BraintreeConfiguration; import org.whispersystems.textsecuregcm.configuration.CdnConfiguration; -import org.whispersystems.textsecuregcm.configuration.ChallengeConfiguration; import org.whispersystems.textsecuregcm.configuration.DatadogConfiguration; import org.whispersystems.textsecuregcm.configuration.DirectoryV2Configuration; import org.whispersystems.textsecuregcm.configuration.DynamoDbClientConfiguration; @@ -187,11 +186,6 @@ public class WhisperServerConfiguration extends Configuration { @JsonProperty private UnidentifiedDeliveryConfiguration unidentifiedDelivery; - @Valid - @NotNull - @JsonProperty - private ChallengeConfiguration challenge; - @Valid @NotNull @JsonProperty @@ -301,10 +295,6 @@ public class WhisperServerConfiguration extends Configuration { return dynamoDbTables; } - public ChallengeConfiguration getChallengeConfiguration() { - return challenge; - } - public RecaptchaConfiguration getRecaptchaConfiguration() { return recaptcha; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 20954d98d..4a3bcf50d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -185,7 +185,6 @@ import org.whispersystems.textsecuregcm.storage.VerificationSessionManager; import org.whispersystems.textsecuregcm.storage.VerificationSessions; import org.whispersystems.textsecuregcm.subscriptions.BraintreeManager; import org.whispersystems.textsecuregcm.subscriptions.StripeManager; -import org.whispersystems.textsecuregcm.util.ChallengeTokenBlinder; import org.whispersystems.textsecuregcm.util.DynamoDbFromConfig; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.UsernameHashZkProofVerifier; @@ -562,9 +561,7 @@ public class WhisperServerService extends Application maybeAuth, + public Response handleChallengeResponse(@Auth final AuthenticatedAccount auth, @Valid final AnswerChallengeRequest answerRequest, @HeaderParam(HttpHeaders.X_FORWARDED_FOR) final String forwardedFor, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent) throws RateLimitExceededException, IOException { @@ -97,20 +85,13 @@ public class ChallengeController { if (answerRequest instanceof final AnswerPushChallengeRequest pushChallengeRequest) { tags = tags.and(CHALLENGE_TYPE_TAG, "push"); - rateLimitChallengeManager.answerPushChallenge( - maybeAuth.orElseThrow(() -> new WebApplicationException(Response.Status.UNAUTHORIZED)).getAccount(), - pushChallengeRequest.getChallenge()); + rateLimitChallengeManager.answerPushChallenge(auth.getAccount(), pushChallengeRequest.getChallenge()); } else if (answerRequest instanceof AnswerRecaptchaChallengeRequest recaptchaChallengeRequest) { tags = tags.and(CHALLENGE_TYPE_TAG, "recaptcha"); - final Account account = maybeAuth - .map(AuthenticatedAccount::getAccount) - .or(() -> tokenBlinder.unblindAccountToken(recaptchaChallengeRequest.getToken()).flatMap(accounts::getByAccountIdentifier)) - .orElseThrow(() -> new WebApplicationException(Response.Status.UNAUTHORIZED)); - final String mostRecentProxy = HeaderUtils.getMostRecentProxy(forwardedFor).orElseThrow(() -> new BadRequestException()); boolean success = rateLimitChallengeManager.answerRecaptchaChallenge( - account, + auth.getAccount(), recaptchaChallengeRequest.getCaptcha(), mostRecentProxy, userAgent); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/RateLimitChallenge.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/RateLimitChallenge.java index 013556684..bb396ea42 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/RateLimitChallenge.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/RateLimitChallenge.java @@ -2,7 +2,6 @@ package org.whispersystems.textsecuregcm.entities; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import io.swagger.v3.oas.annotations.media.Schema; import java.util.List; import javax.validation.constraints.NotNull; @@ -10,7 +9,6 @@ public class RateLimitChallenge { @JsonProperty @NotNull - @Schema(description="An opaque token to be included along with the challenge result in the verification request") private final String token; @JsonProperty diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/ChallengeTokenBlinder.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/ChallengeTokenBlinder.java deleted file mode 100644 index efd5b18a6..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/ChallengeTokenBlinder.java +++ /dev/null @@ -1,109 +0,0 @@ -/* - * Copyright 2023 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.util; - -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.core.JsonProcessingException; -import java.io.IOException; -import java.nio.ByteBuffer; -import java.security.GeneralSecurityException; -import java.security.ProviderException; -import java.security.SecureRandom; -import java.time.Clock; -import java.time.Duration; -import java.time.Instant; -import java.util.Base64; -import java.util.Optional; -import java.util.UUID; -import javax.crypto.AEADBadTagException; -import javax.crypto.Cipher; -import javax.crypto.SecretKey; -import javax.crypto.spec.GCMParameterSpec; -import javax.crypto.spec.SecretKeySpec; -import org.whispersystems.textsecuregcm.configuration.ChallengeConfiguration; - -public class ChallengeTokenBlinder { - - private record Token( - UUID uuid, - Instant timestamp) { - } - - private static final ObjectMapper mapper = SystemMapper.jsonMapper(); - private final Clock clock; - private final Duration tokenTtl; - private final SecureRandom secureRandom = new SecureRandom(); - private final SecretKey blindingKey; - - public ChallengeTokenBlinder(final ChallengeConfiguration config, final Clock clock) { - this.blindingKey = new SecretKeySpec(config.blindingSecret().value(), "AES"); - this.tokenTtl = config.tokenTtl(); - this.clock = clock; - } - - public String generateBlindedAccountToken(UUID aci) { - - final Token token = new Token(aci, clock.instant()); - final byte[] serializedToken; - try { - serializedToken = mapper.writeValueAsBytes(token); - } catch (IOException e) { // should really, really never happen - throw new IllegalArgumentException(); - } - - final byte[] iv = new byte[12]; - secureRandom.nextBytes(iv); - final GCMParameterSpec parameterSpec = new GCMParameterSpec(128, iv); - - try { - final Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); - cipher.init(Cipher.ENCRYPT_MODE, blindingKey, parameterSpec); - final byte[] ciphertext = cipher.doFinal(serializedToken); - - final ByteBuffer byteBuffer = ByteBuffer.allocate(iv.length + ciphertext.length); - byteBuffer.put(iv); - byteBuffer.put(ciphertext); - return Base64.getUrlEncoder().withoutPadding().encodeToString(byteBuffer.array()); - } catch (GeneralSecurityException e) { - throw new IllegalArgumentException(e); - } - } - - public Optional unblindAccountToken(String token) { - final byte[] ciphertext; - try { - ciphertext = Base64.getUrlDecoder().decode(token); - } catch (IllegalArgumentException e) { - return Optional.empty(); - } - - final Token parsedToken; - - try { - final Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding"); - final GCMParameterSpec parameterSpec = new GCMParameterSpec(128, ciphertext, 0, 12); - cipher.init(Cipher.DECRYPT_MODE, blindingKey, parameterSpec); - - parsedToken = mapper.readValue(cipher.doFinal(ciphertext, 12, ciphertext.length - 12), Token.class); - } catch (ProviderException | AEADBadTagException | JsonProcessingException e) { - // the token doesn't successfully decrypt with this key, it's bogus (or from an older server version or before a key rotation) - return Optional.empty(); - } catch (IOException | GeneralSecurityException e) { // should never happen - throw new IllegalArgumentException(); - } - - Instant now = clock.instant(); - Instant intervalStart = now.minus(tokenTtl); - Instant tokenTime = parsedToken.timestamp(); - if (tokenTime.isAfter(now) || tokenTime.isBefore(intervalStart)) { - // expired or fraudulently-future token - return Optional.empty(); - } - - return Optional.of(parsedToken.uuid()); - } - -} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java index fab91edfe..c91b9dfc3 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ChallengeControllerTest.java @@ -21,13 +21,8 @@ import io.dropwizard.auth.PolymorphicAuthValueFactoryProvider; import io.dropwizard.testing.junit5.DropwizardExtensionsSupport; import io.dropwizard.testing.junit5.ResourceExtension; import java.io.IOException; -import java.time.Clock; import java.time.Duration; -import java.time.Instant; -import java.time.ZoneId; -import java.util.Optional; import java.util.Set; -import java.util.UUID; import javax.ws.rs.client.Entity; import javax.ws.rs.core.Response; import org.glassfish.jersey.test.grizzly.GrizzlyWebTestContainerFactory; @@ -36,30 +31,18 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.whispersystems.textsecuregcm.auth.AuthenticatedAccount; import org.whispersystems.textsecuregcm.auth.DisabledPermittedAuthenticatedAccount; -import org.whispersystems.textsecuregcm.configuration.ChallengeConfiguration; -import org.whispersystems.textsecuregcm.configuration.secrets.SecretBytes; import org.whispersystems.textsecuregcm.limits.RateLimitChallengeManager; import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.push.NotPushRegisteredException; -import org.whispersystems.textsecuregcm.storage.Accounts; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; -import org.whispersystems.textsecuregcm.util.ChallengeTokenBlinder; import org.whispersystems.textsecuregcm.util.SystemMapper; -import org.whispersystems.textsecuregcm.util.TestClock; @ExtendWith(DropwizardExtensionsSupport.class) class ChallengeControllerTest { + private static final RateLimitChallengeManager rateLimitChallengeManager = mock(RateLimitChallengeManager.class); - private static final Accounts accounts = mock(Accounts.class); - - private static final TestClock clock = TestClock.now(); - - private static final ChallengeTokenBlinder tokenBlinder = new ChallengeTokenBlinder( - new ChallengeConfiguration(new SecretBytes("super secret key".getBytes()), Duration.ofMinutes(10)), - clock); - - private static final ChallengeController challengeController = new ChallengeController(accounts, tokenBlinder, rateLimitChallengeManager); + private static final ChallengeController challengeController = new ChallengeController(rateLimitChallengeManager); private static final ResourceExtension EXTENSION = ResourceExtension.builder() .addProvider(AuthHelper.getAuthFilter()) @@ -73,9 +56,7 @@ class ChallengeControllerTest { @AfterEach void teardown() { - reset(accounts); reset(rateLimitChallengeManager); - clock.unpin(); } @Test @@ -207,181 +188,6 @@ class ChallengeControllerTest { verifyNoInteractions(rateLimitChallengeManager); } - @Test - void testHandleRecaptchaWithTokenAuth() throws Exception { - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "%s", - "captcha": "The value of the solved captcha token" - } - """.formatted(tokenBlinder.generateBlindedAccountToken(AuthHelper.VALID_UUID)); - - when(accounts.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(200, response.getStatus()); - - verify(rateLimitChallengeManager).answerRecaptchaChallenge(eq(AuthHelper.VALID_ACCOUNT), eq("The value of the solved captcha token"), eq("10.0.0.1"), anyString()); - } - - @Test - void testHandleRecaptchaWithExpiredToken() throws Exception { - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "%s", - "captcha": "The value of the solved captcha token" - } - """.formatted(tokenBlinder.generateBlindedAccountToken(AuthHelper.VALID_UUID)); - - clock.pin(clock.instant().plus(Duration.ofMinutes(20))); - when(accounts.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(401, response.getStatus()); - - verifyNoInteractions(rateLimitChallengeManager); - } - - @Test - void testHandleRecaptchaWithPostdatedToken() throws Exception { - clock.pin(clock.instant()); - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "%s", - "captcha": "The value of the solved captcha token" - } - """.formatted(tokenBlinder.generateBlindedAccountToken(AuthHelper.VALID_UUID)); - - clock.pin(clock.instant().minus(Duration.ofMinutes(1))); - when(accounts.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(AuthHelper.VALID_ACCOUNT)); - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(401, response.getStatus()); - - verifyNoInteractions(rateLimitChallengeManager); - } - - @Test - void testHandleRecaptchaNoAuthNonBase64Token() throws Exception { - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "This is not a valid auth token", - "captcha": "The value of the solved captcha token" - } - """; - - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(401, response.getStatus()); - - verifyNoInteractions(rateLimitChallengeManager); - } - - @Test - void testHandleRecaptchaNoAuthValidBase64Token() throws Exception { - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "Y2x1Y2sgY2x1Y2ssIGknbSBhIHBhcnJvdAo=", - "captcha": "The value of the solved captcha token" - } - """; - - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(401, response.getStatus()); - - verifyNoInteractions(rateLimitChallengeManager); - } - - @Test - void testHandleRecaptchaNoAuthTokenEncryptedWithWrongKey() throws Exception { - final String badToken = - new ChallengeTokenBlinder( - new ChallengeConfiguration(new SecretBytes("oh no, wrong key".getBytes()), Duration.ofMinutes(10)), - clock) - .generateBlindedAccountToken(AuthHelper.VALID_UUID); - - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "%s", - "captcha": "The value of the solved captcha token" - } - """.formatted(badToken); - - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(401, response.getStatus()); - - verifyNoInteractions(rateLimitChallengeManager); - } - - @Test - void testHandleRecaptchaWithTokenForBadAccount() throws Exception { - final UUID badUUID = UUID.randomUUID(); - final String recaptchaChallengeJson = """ - { - "type": "recaptcha", - "token": "%s", - "captcha": "The value of the solved captcha token" - } - """.formatted(tokenBlinder.generateBlindedAccountToken(badUUID)); - - when(accounts.getByAccountIdentifier(badUUID)).thenReturn(Optional.empty()); - when(rateLimitChallengeManager.answerRecaptchaChallenge(any(), any(), any(), any())) - .thenReturn(true); - - final Response response = EXTENSION.target("/v1/challenge") - .request() - .header(HttpHeaders.X_FORWARDED_FOR, "10.0.0.1") - .put(Entity.json(recaptchaChallengeJson)); - - assertEquals(401, response.getStatus()); - - verifyNoInteractions(rateLimitChallengeManager); - } - @Test void testHandleUnrecognizedAnswer() { final String unrecognizedJson = """