From 7cbbf73cc9c195c4442b012b831cad067a234fe0 Mon Sep 17 00:00:00 2001 From: Ameya Lokare Date: Fri, 16 Aug 2024 10:33:44 -0700 Subject: [PATCH] Add registration recovery checker --- .../textsecuregcm/WhisperServerService.java | 11 +++- .../auth/PhoneVerificationTokenManager.java | 17 +++++-- .../controllers/AccountControllerV2.java | 9 ++-- .../controllers/RegistrationController.java | 9 ++-- .../spam/RegistrationRecoveryChecker.java | 19 +++++++ .../textsecuregcm/spam/SpamFilter.java | 8 +++ .../controllers/AccountControllerV2Test.java | 41 ++++++++++++++- .../RegistrationControllerTest.java | 51 ++++++++++++++++++- 8 files changed, 151 insertions(+), 14 deletions(-) create mode 100644 service/src/main/java/org/whispersystems/textsecuregcm/spam/RegistrationRecoveryChecker.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index 679f35a73..b400766ba 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -204,6 +204,7 @@ import org.whispersystems.textsecuregcm.securevaluerecovery.SecureValueRecovery2Client; import org.whispersystems.textsecuregcm.spam.ChallengeConstraintChecker; import org.whispersystems.textsecuregcm.spam.RegistrationFraudChecker; +import org.whispersystems.textsecuregcm.spam.RegistrationRecoveryChecker; import org.whispersystems.textsecuregcm.spam.ReportSpamTokenProvider; import org.whispersystems.textsecuregcm.spam.SpamChecker; import org.whispersystems.textsecuregcm.spam.SpamFilter; @@ -675,8 +676,6 @@ public void run(WhisperServerConfiguration config, Environment environment) thro final RegistrationLockVerificationManager registrationLockVerificationManager = new RegistrationLockVerificationManager( accountsManager, clientPresenceManager, svr2CredentialsGenerator, svr3CredentialsGenerator, registrationRecoveryPasswordsManager, pushNotificationManager, rateLimiters); - final PhoneVerificationTokenManager phoneVerificationTokenManager = new PhoneVerificationTokenManager( - registrationServiceClient, registrationRecoveryPasswordsManager); final ReportedMessageMetricsListener reportedMessageMetricsListener = new ReportedMessageMetricsListener( accountsManager); @@ -1059,6 +1058,12 @@ protected void configureServer(final ServerBuilder serverBuilder) { log.warn("No registration-fraud-checkers found; using default (no-op) provider as a default"); return RegistrationFraudChecker.noop(); }); + final RegistrationRecoveryChecker registrationRecoveryChecker = spamFilter + .map(SpamFilter::getRegistrationRecoveryChecker) + .orElseGet(() -> { + log.warn("No registration-recovery-checkers found; using default (no-op) provider as a default"); + return RegistrationRecoveryChecker.noop(); + }); spamFilter.map(SpamFilter::getReportedMessageListener).ifPresent(reportMessageManager::addListener); @@ -1072,6 +1077,8 @@ protected void configureServer(final ServerBuilder serverBuilder) { }); + final PhoneVerificationTokenManager phoneVerificationTokenManager = new PhoneVerificationTokenManager( + registrationServiceClient, registrationRecoveryPasswordsManager, registrationRecoveryChecker); final List commonControllers = Lists.newArrayList( new AccountController(accountsManager, rateLimiters, turnTokenGenerator, registrationRecoveryPasswordsManager, usernameHashZkProofVerifier), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/PhoneVerificationTokenManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/PhoneVerificationTokenManager.java index d09469306..2dd23b6d2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/PhoneVerificationTokenManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/PhoneVerificationTokenManager.java @@ -17,12 +17,14 @@ import javax.ws.rs.ForbiddenException; import javax.ws.rs.NotAuthorizedException; import javax.ws.rs.ServerErrorException; +import javax.ws.rs.container.ContainerRequestContext; import javax.ws.rs.core.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.entities.PhoneVerificationRequest; import org.whispersystems.textsecuregcm.entities.RegistrationServiceSession; import org.whispersystems.textsecuregcm.registration.RegistrationServiceClient; +import org.whispersystems.textsecuregcm.spam.RegistrationRecoveryChecker; import org.whispersystems.textsecuregcm.storage.RegistrationRecoveryPasswordsManager; public class PhoneVerificationTokenManager { @@ -33,17 +35,21 @@ public class PhoneVerificationTokenManager { private final RegistrationServiceClient registrationServiceClient; private final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager; + private final RegistrationRecoveryChecker registrationRecoveryChecker; public PhoneVerificationTokenManager(final RegistrationServiceClient registrationServiceClient, - final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager) { + final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager, + final RegistrationRecoveryChecker registrationRecoveryChecker) { this.registrationServiceClient = registrationServiceClient; this.registrationRecoveryPasswordsManager = registrationRecoveryPasswordsManager; + this.registrationRecoveryChecker = registrationRecoveryChecker; } /** * Checks if a {@link PhoneVerificationRequest} has a token that verifies the caller has confirmed access to the e164 * number * + * @param requestContext the container request context * @param number the e164 presented for verification * @param request the request with exactly one verification token (RegistrationService sessionId or registration * recovery password) @@ -54,13 +60,13 @@ public PhoneVerificationTokenManager(final RegistrationServiceClient registratio * @throws ForbiddenException if the recovery password is not valid * @throws InterruptedException if verification did not complete before a timeout */ - public PhoneVerificationRequest.VerificationType verify(final String number, final PhoneVerificationRequest request) + public PhoneVerificationRequest.VerificationType verify(final ContainerRequestContext requestContext, final String number, final PhoneVerificationRequest request) throws InterruptedException { final PhoneVerificationRequest.VerificationType verificationType = request.verificationType(); switch (verificationType) { case SESSION -> verifyBySessionId(number, request.decodeSessionId()); - case RECOVERY_PASSWORD -> verifyByRecoveryPassword(number, request.recoveryPassword()); + case RECOVERY_PASSWORD -> verifyByRecoveryPassword(requestContext, number, request.recoveryPassword()); } return verificationType; @@ -97,8 +103,11 @@ private void verifyBySessionId(final String number, final byte[] sessionId) thro } } - private void verifyByRecoveryPassword(final String number, final byte[] recoveryPassword) + private void verifyByRecoveryPassword(final ContainerRequestContext requestContext, final String number, final byte[] recoveryPassword) throws InterruptedException { + if (!registrationRecoveryChecker.checkRegistrationRecoveryAttempt(requestContext, number)) { + throw new ForbiddenException("recoveryPassword couldn't be verified"); + } try { final boolean verified = registrationRecoveryPasswordsManager.verify(number, recoveryPassword) .get(VERIFICATION_TIMEOUT_SECONDS, TimeUnit.SECONDS); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java index 44c558fc3..f278a9ed2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java @@ -32,6 +32,8 @@ import javax.ws.rs.Path; import javax.ws.rs.Produces; import javax.ws.rs.WebApplicationException; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import org.whispersystems.textsecuregcm.auth.AuthenticatedDevice; @@ -98,7 +100,8 @@ public AccountControllerV2(final AccountsManager accountsManager, name = "Retry-After", description = "If present, an positive integer indicating the number of seconds before a subsequent attempt could succeed")) public AccountIdentityResponse changeNumber(@Mutable @Auth final AuthenticatedDevice authenticatedDevice, - @NotNull @Valid final ChangeNumberRequest request, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgentString) + @NotNull @Valid final ChangeNumberRequest request, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgentString, + @Context final ContainerRequestContext requestContext) throws RateLimitExceededException, InterruptedException { if (!authenticatedDevice.getAuthenticatedDevice().isPrimary()) { @@ -116,8 +119,8 @@ public AccountIdentityResponse changeNumber(@Mutable @Auth final AuthenticatedDe rateLimiters.getRegistrationLimiter().validate(number); - final PhoneVerificationRequest.VerificationType verificationType = - phoneVerificationTokenManager.verify(number, request); + final PhoneVerificationRequest.VerificationType verificationType = phoneVerificationTokenManager.verify( + requestContext, number, request); final Optional existingAccount = accountsManager.getByE164(number); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java index ebbd029b8..91c7c9c1e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java @@ -29,6 +29,8 @@ import javax.ws.rs.Path; import javax.ws.rs.Produces; import javax.ws.rs.WebApplicationException; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import org.whispersystems.textsecuregcm.auth.BasicAuthorizationHeader; @@ -102,7 +104,8 @@ public AccountIdentityResponse register( @HeaderParam(HttpHeaders.AUTHORIZATION) @NotNull final BasicAuthorizationHeader authorizationHeader, @HeaderParam(HeaderUtils.X_SIGNAL_AGENT) final String signalAgent, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent, - @NotNull @Valid final RegistrationRequest registrationRequest) throws RateLimitExceededException, InterruptedException { + @NotNull @Valid final RegistrationRequest registrationRequest, + @Context final ContainerRequestContext requestContext) throws RateLimitExceededException, InterruptedException { final String number = authorizationHeader.getUsername(); final String password = authorizationHeader.getPassword(); @@ -113,8 +116,8 @@ public AccountIdentityResponse register( rateLimiters.getRegistrationLimiter().validate(number); - final PhoneVerificationRequest.VerificationType verificationType = phoneVerificationTokenManager.verify(number, - registrationRequest); + final PhoneVerificationRequest.VerificationType verificationType = phoneVerificationTokenManager.verify( + requestContext, number, registrationRequest); final Optional existingAccount = accounts.getByE164(number); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/spam/RegistrationRecoveryChecker.java b/service/src/main/java/org/whispersystems/textsecuregcm/spam/RegistrationRecoveryChecker.java new file mode 100644 index 000000000..91e7a9d3b --- /dev/null +++ b/service/src/main/java/org/whispersystems/textsecuregcm/spam/RegistrationRecoveryChecker.java @@ -0,0 +1,19 @@ +package org.whispersystems.textsecuregcm.spam; + +import javax.ws.rs.container.ContainerRequestContext; + +public interface RegistrationRecoveryChecker { + + /** + * Determine if a registration recovery attempt should be allowed or not + * + * @param requestContext The container request context for a registration recovery attempt + * @param e164 The E164 formatted phone number of the requester + * @return true if the registration recovery attempt is allowed, false otherwise. + */ + boolean checkRegistrationRecoveryAttempt(final ContainerRequestContext requestContext, final String e164); + + static RegistrationRecoveryChecker noop() { + return (ignoredCtx, ignoredE164) -> true; + } +} diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/spam/SpamFilter.java b/service/src/main/java/org/whispersystems/textsecuregcm/spam/SpamFilter.java index 5612633cf..89cba9e9f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/spam/SpamFilter.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/spam/SpamFilter.java @@ -79,4 +79,12 @@ public interface SpamFilter extends Managed { * @return a {@link ChallengeConstraintChecker} controlled by the spam filter */ ChallengeConstraintChecker getChallengeConstraintChecker(); + + /** + * Return a checker that will be called to determine if a user is allowed to use their + * registration recovery password to re-register + * + * @return a {@link RegistrationRecoveryChecker} controlled by the spam filter + */ + RegistrationRecoveryChecker getRegistrationRecoveryChecker(); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java index daca6eb5c..d2f94920f 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java @@ -80,6 +80,7 @@ import org.whispersystems.textsecuregcm.mappers.NonNormalizedPhoneNumberExceptionMapper; import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.registration.RegistrationServiceClient; +import org.whispersystems.textsecuregcm.spam.RegistrationRecoveryChecker; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountBadge; import org.whispersystems.textsecuregcm.storage.AccountsManager; @@ -111,6 +112,7 @@ class AccountControllerV2Test { RegistrationLockVerificationManager.class); private final RateLimiters rateLimiters = mock(RateLimiters.class); private final RateLimiter registrationLimiter = mock(RateLimiter.class); + private final RegistrationRecoveryChecker registrationRecoveryChecker = mock(RegistrationRecoveryChecker.class); private final ResourceExtension resources = ResourceExtension.builder() .addProperty(ServerProperties.UNWRAP_COMPLETION_STAGE_IN_WRITER_ENABLE, Boolean.TRUE) @@ -123,7 +125,8 @@ class AccountControllerV2Test { .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) .addResource( new AccountControllerV2(accountsManager, changeNumberManager, - new PhoneVerificationTokenManager(registrationServiceClient, registrationRecoveryPasswordsManager), + new PhoneVerificationTokenManager(registrationServiceClient, registrationRecoveryPasswordsManager, + registrationRecoveryChecker), registrationLockVerificationManager, rateLimiters)) .build(); @@ -382,6 +385,8 @@ void registrationLock(final RegistrationLockError error) throws Exception { void recoveryPasswordManagerVerificationTrue() throws Exception { when(registrationRecoveryPasswordsManager.verify(any(), any())) .thenReturn(CompletableFuture.completedFuture(true)); + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())) + .thenReturn(true); final Invocation.Builder request = resources.getJerseyTest() .target("/v2/accounts/number") @@ -418,6 +423,40 @@ void recoveryPasswordManagerVerificationFalse() { } } + @Test + void registrationRecoveryCheckerAllowsAttempt() { + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())).thenReturn(true); + when(registrationRecoveryPasswordsManager.verify(any(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); + + final Invocation.Builder request = resources.getJerseyTest() + .target("/v2/accounts/number") + .request() + .header(HttpHeaders.AUTHORIZATION, + AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)); + final byte[] recoveryPassword = new byte[32]; + try (Response response = request.put(Entity.json(requestJsonRecoveryPassword(recoveryPassword, NEW_NUMBER)))) { + assertEquals(200, response.getStatus()); + } + } + + @Test + void registrationRecoveryCheckerDisallowsAttempt() { + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())).thenReturn(false); + when(registrationRecoveryPasswordsManager.verify(any(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); + + final Invocation.Builder request = resources.getJerseyTest() + .target("/v2/accounts/number") + .request() + .header(HttpHeaders.AUTHORIZATION, + AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)); + final byte[] recoveryPassword = new byte[32]; + try (Response response = request.put(Entity.json(requestJsonRecoveryPassword(recoveryPassword, NEW_NUMBER)))) { + assertEquals(403, response.getStatus()); + } + } + /** * Valid request JSON with the given Recovery Password */ diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java index 93fedc889..9ffe908c0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java @@ -71,6 +71,7 @@ import org.whispersystems.textsecuregcm.mappers.NonNormalizedPhoneNumberExceptionMapper; import org.whispersystems.textsecuregcm.mappers.RateLimitExceededExceptionMapper; import org.whispersystems.textsecuregcm.registration.RegistrationServiceClient; +import org.whispersystems.textsecuregcm.spam.RegistrationRecoveryChecker; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.DeviceSpec; @@ -97,6 +98,7 @@ class RegistrationControllerTest { RegistrationLockVerificationManager.class); private final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager = mock( RegistrationRecoveryPasswordsManager.class); + private final RegistrationRecoveryChecker registrationRecoveryChecker = mock(RegistrationRecoveryChecker.class); private final RateLimiters rateLimiters = mock(RateLimiters.class); private final RateLimiter registrationLimiter = mock(RateLimiter.class); @@ -110,7 +112,8 @@ class RegistrationControllerTest { .setTestContainerFactory(new GrizzlyWebTestContainerFactory()) .addResource( new RegistrationController(accountsManager, - new PhoneVerificationTokenManager(registrationServiceClient, registrationRecoveryPasswordsManager), + new PhoneVerificationTokenManager(registrationServiceClient, registrationRecoveryPasswordsManager, + registrationRecoveryChecker), registrationLockVerificationManager, rateLimiters)) .build(); @@ -239,6 +242,7 @@ void registrationServiceTimeout() { @Test void recoveryPasswordManagerVerificationFailureOrTimeout() { + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())).thenReturn(true); when(registrationRecoveryPasswordsManager.verify(any(), any())) .thenReturn(CompletableFuture.failedFuture(new RuntimeException())); @@ -284,6 +288,7 @@ static Stream registrationServiceSessionCheck() { @Test void recoveryPasswordManagerVerificationTrue() throws InterruptedException { + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())).thenReturn(true); when(registrationRecoveryPasswordsManager.verify(any(), any())) .thenReturn(CompletableFuture.completedFuture(true)); @@ -317,6 +322,50 @@ void recoveryPasswordManagerVerificationFalse() throws InterruptedException { } } + @Test + void registrationRecoveryCheckerAllowsAttempt() throws InterruptedException { + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())).thenReturn(true); + when(registrationRecoveryPasswordsManager.verify(any(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); + + final Account account = mock(Account.class); + when(account.getPrimaryDevice()).thenReturn(mock(Device.class)); + + when(accountsManager.create(any(), any(), any(), any(), any(), any())) + .thenReturn(account); + + final Invocation.Builder request = resources.getJerseyTest() + .target("/v1/registration") + .request() + .header(HttpHeaders.AUTHORIZATION, AuthHelper.getProvisioningAuthHeader(NUMBER, PASSWORD)); + final byte[] recoveryPassword = new byte[32]; + try (Response response = request.post(Entity.json(requestJsonRecoveryPassword(recoveryPassword)))) { + assertEquals(200, response.getStatus()); + } + } + + @Test + void registrationRecoveryCheckerDisallowsAttempt() throws InterruptedException { + when(registrationRecoveryChecker.checkRegistrationRecoveryAttempt(any(), any())).thenReturn(false); + when(registrationRecoveryPasswordsManager.verify(any(), any())) + .thenReturn(CompletableFuture.completedFuture(true)); + + final Account account = mock(Account.class); + when(account.getPrimaryDevice()).thenReturn(mock(Device.class)); + + when(accountsManager.create(any(), any(), any(), any(), any(), any())) + .thenReturn(account); + + final Invocation.Builder request = resources.getJerseyTest() + .target("/v1/registration") + .request() + .header(HttpHeaders.AUTHORIZATION, AuthHelper.getProvisioningAuthHeader(NUMBER, PASSWORD)); + final byte[] recoveryPassword = new byte[32]; + try (Response response = request.post(Entity.json(requestJsonRecoveryPassword(recoveryPassword)))) { + assertEquals(403, response.getStatus()); + } + } + @CartesianTest @CartesianTest.MethodFactory("registrationLockAndDeviceTransfer") void registrationLockAndDeviceTransfer(