Skip to content

Commit

Permalink
Prevent old versions of the Android app from carrying out "change pho…
Browse files Browse the repository at this point in the history 
…ne number" requests
  • Loading branch information
@jon-signal
jon-signal committed Feb 9, 2024
1 parent c5dc01e commit 4f45f23
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 4 deletions.
28 changes: 25 additions & 3 deletions service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")
Expand All @@ -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) {
Expand All @@ -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...
Expand All @@ -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();
}
Expand Down
45 changes: 44 additions & 1 deletion ...e/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
*/
Expand Down

0 comments on commit 4f45f23

Please sign in to comment.