From 4f45f2309400719da3bfefb1917260bbd718d500 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Thu, 8 Feb 2024 13:33:59 -0500 Subject: [PATCH] Prevent old versions of the Android app from carrying out "change phone number" requests --- .../controllers/AccountControllerV2.java | 28 ++++++++++-- .../controllers/AccountControllerV2Test.java | 45 ++++++++++++++++++- 2 files changed, 69 insertions(+), 4 deletions(-) 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 350e3950f..7f0c0a760 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java @@ -8,7 +8,9 @@ import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name; import com.google.common.net.HttpHeaders; +import com.vdurmont.semver4j.Semver; import io.dropwizard.auth.Auth; +import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.Metrics; import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Tags; @@ -51,6 +53,10 @@ import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.ChangeNumberManager; +import org.whispersystems.textsecuregcm.util.ua.ClientPlatform; +import org.whispersystems.textsecuregcm.util.ua.UnrecognizedUserAgentException; +import org.whispersystems.textsecuregcm.util.ua.UserAgent; +import org.whispersystems.textsecuregcm.util.ua.UserAgentUtil; @Path("/v2/accounts") @io.swagger.v3.oas.annotations.tags.Tag(name = "Account") @@ -59,12 +65,17 @@ public class AccountControllerV2 { private static final String CHANGE_NUMBER_COUNTER_NAME = name(AccountControllerV2.class, "changeNumber"); private static final String VERIFICATION_TYPE_TAG_NAME = "verification"; + private static final Counter ANDROID_CHANGE_NUMBER_REJECTED_COUNTER = + Metrics.counter(name(AccountControllerV2.class, "androidChangeNumberRejected")); + private final AccountsManager accountsManager; private final ChangeNumberManager changeNumberManager; private final PhoneVerificationTokenManager phoneVerificationTokenManager; private final RegistrationLockVerificationManager registrationLockVerificationManager; private final RateLimiters rateLimiters; + private static final Semver MINIMUM_ANDROID_CHANGE_NUMBER_VERSION = new Semver("6.46.7"); + public AccountControllerV2(final AccountsManager accountsManager, final ChangeNumberManager changeNumberManager, final PhoneVerificationTokenManager phoneVerificationTokenManager, final RegistrationLockVerificationManager registrationLockVerificationManager, final RateLimiters rateLimiters) { @@ -91,13 +102,24 @@ public AccountControllerV2(final AccountsManager accountsManager, final ChangeNu name = "Retry-After", description = "If present, an positive integer indicating the number of seconds before a subsequent attempt could succeed")) public AccountIdentityResponse changeNumber(@Auth final AuthenticatedAccount authenticatedAccount, - @NotNull @Valid final ChangeNumberRequest request, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgent) + @NotNull @Valid final ChangeNumberRequest request, @HeaderParam(HttpHeaders.USER_AGENT) final String userAgentString) throws RateLimitExceededException, InterruptedException { if (!authenticatedAccount.getAuthenticatedDevice().isPrimary()) { throw new ForbiddenException(); } + // We can remove this check after old versions of the Android app expire on or after 2024-05-08 + try { + final UserAgent userAgent = UserAgentUtil.parseUserAgentString(userAgentString); + + if (userAgent.getPlatform().equals(ClientPlatform.ANDROID) && userAgent.getVersion().isLowerThan(MINIMUM_ANDROID_CHANGE_NUMBER_VERSION)) { + ANDROID_CHANGE_NUMBER_REJECTED_COUNTER.increment(); + throw new WebApplicationException(499); + } + } catch (final UnrecognizedUserAgentException ignored) { + } + final String number = request.number(); // Only verify and check reglock if there's a data change to be made... @@ -112,10 +134,10 @@ public AccountIdentityResponse changeNumber(@Auth final AuthenticatedAccount aut if (existingAccount.isPresent()) { registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), request.registrationLock(), - userAgent, RegistrationLockVerificationManager.Flow.CHANGE_NUMBER, verificationType); + userAgentString, RegistrationLockVerificationManager.Flow.CHANGE_NUMBER, verificationType); } - Metrics.counter(CHANGE_NUMBER_COUNTER_NAME, Tags.of(UserAgentTagUtil.getPlatformTag(userAgent), + Metrics.counter(CHANGE_NUMBER_COUNTER_NAME, Tags.of(UserAgentTagUtil.getPlatformTag(userAgentString), Tag.of(VERIFICATION_TYPE_TAG_NAME, verificationType.name()))) .increment(); } 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 4bb6f295c..9352dfb10 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java @@ -56,6 +56,7 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.ArgumentCaptor; @@ -88,7 +89,6 @@ import org.whispersystems.textsecuregcm.storage.RegistrationRecoveryPasswordsManager; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; import org.whispersystems.textsecuregcm.tests.util.AuthHelper; -import org.whispersystems.textsecuregcm.tests.util.KeysHelper; import org.whispersystems.textsecuregcm.util.SystemMapper; import org.whispersystems.textsecuregcm.util.Util; @@ -419,6 +419,49 @@ void recoveryPasswordManagerVerificationFalse() { } } + @ParameterizedTest + @CsvSource({ + "Signal-Android/4.68.3, true", + "Signal-Android/6.46.7, false", + "Signal-Android/6.46.8, false", + "Signal-Desktop/1.2.3 Linux, false", + "Signal-iOS/3.9.0 iOS/14.2, false", + "Not a real user-agent string, false" + }) + void changeNumberVersionEnforced(final String userAgentString, final boolean expectBlocked) throws Exception { + + when(registrationServiceClient.getSession(any(), any())) + .thenReturn(CompletableFuture.completedFuture( + Optional.of(new RegistrationServiceSession(new byte[16], NEW_NUMBER, true, null, null, null, + SESSION_EXPIRATION_SECONDS)))); + + try (final Response response = resources.getJerseyTest() + .target("/v2/accounts/number") + .request() + .header(HttpHeaders.AUTHORIZATION, + AuthHelper.getAuthHeader(AuthHelper.VALID_UUID, AuthHelper.VALID_PASSWORD)) + .header(HttpHeaders.USER_AGENT, userAgentString) + .put(Entity.entity( + new ChangeNumberRequest(encodeSessionId("session"), null, NEW_NUMBER, "123", new IdentityKey(Curve.generateKeyPair().getPublicKey()), + Collections.emptyList(), + Collections.emptyMap(), null, Collections.emptyMap()), + MediaType.APPLICATION_JSON_TYPE))) { + + if (expectBlocked) { + assertEquals(499, response.getStatus()); + + verify(changeNumberManager, never()) + .changeNumber(eq(AuthHelper.VALID_ACCOUNT), eq(NEW_NUMBER), any(), any(), any(), any(), any()); + } else { + assertEquals(200, response.getStatus()); + + verify(changeNumberManager) + .changeNumber(eq(AuthHelper.VALID_ACCOUNT), eq(NEW_NUMBER), any(), any(), any(), any(), any()); + + } + } + } + /** * Valid request JSON with the given Recovery Password */