diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index 9b575f544..8c0adc355 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -131,6 +131,7 @@ public class AccountController { private static final String ACCOUNT_VERIFY_COUNTER_NAME = name(AccountController.class, "verify"); private static final String CAPTCHA_ATTEMPT_COUNTER_NAME = name(AccountController.class, "captcha"); private static final String CHALLENGE_ISSUED_COUNTER_NAME = name(AccountController.class, "challengeIssued"); + private static final String INVALID_ACCOUNT_ATTRS_COUNTER_NAME = name(AccountController.class, "invalidAccountAttrs"); private static final DistributionSummary REREGISTRATION_IDLE_DAYS_DISTRIBUTION = DistributionSummary .builder(name(AccountController.class, "reregistrationIdleDays")) @@ -391,6 +392,11 @@ public class AccountController { rateLimiters.getVerifyLimiter().validate(number); + if (!AccountsManager.validNewAccountAttributes(accountAttributes)) { + Metrics.counter(INVALID_ACCOUNT_ATTRS_COUNTER_NAME, Tags.of(UserAgentTagUtil.getPlatformTag(userAgent))).increment(); + throw new WebApplicationException(Response.status(422, "account attributes invalid").build()); + } + // Note that successful verification depends on being able to find a stored verification code for the given number. // We check that numbers are normalized before we store verification codes, and so don't need to re-assert // normalization here. 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 6565e8996..3ddf998d0 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java @@ -69,6 +69,7 @@ public class RegistrationController { private static final String REGION_CODE_TAG_NAME = "regionCode"; private static final String VERIFICATION_TYPE_TAG_NAME = "verification"; private static final String ACCOUNT_ACTIVATED_TAG_NAME = "accountActivated"; + private static final String INVALID_ACCOUNT_ATTRS_COUNTER_NAME = name(RegistrationController.class, "invalidAccountAttrs"); private final AccountsManager accounts; private final PhoneVerificationTokenManager phoneVerificationTokenManager; @@ -118,6 +119,10 @@ public class RegistrationController { final String password = authorizationHeader.getPassword(); RateLimiter.adaptLegacyException(() -> rateLimiters.getRegistrationLimiter().validate(number)); + if (!AccountsManager.validNewAccountAttributes(registrationRequest.accountAttributes())) { + Metrics.counter(INVALID_ACCOUNT_ATTRS_COUNTER_NAME, Tags.of(UserAgentTagUtil.getPlatformTag(userAgent))).increment(); + throw new WebApplicationException(Response.status(422, "account attributes invalid").build()); + } final PhoneVerificationRequest.VerificationType verificationType = phoneVerificationTokenManager.verify(number, registrationRequest); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index c3f20a249..c37cc6ea0 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -28,6 +28,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.OptionalInt; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.atomic.AtomicReference; @@ -330,6 +331,21 @@ public class AccountsManager { return updatedAccount.get(); } + public static boolean validNewAccountAttributes(final AccountAttributes accountAttributes) { + if (!validRegistrationId(accountAttributes.getRegistrationId())) { + return false; + } + final OptionalInt pniRegistrationId = accountAttributes.getPhoneNumberIdentityRegistrationId(); + if (pniRegistrationId.isPresent() && !validRegistrationId(pniRegistrationId.getAsInt())) { + return false; + } + return true; + } + + private static boolean validRegistrationId(int registrationId) { + return registrationId > 0 && registrationId <= Device.MAX_REGISTRATION_ID; + } + public Account updatePniKeys(final Account account, final byte[] pniIdentityKey, final Map pniSignedPreKeys, diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java index 7ed2808d4..e6d2197fe 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java @@ -20,6 +20,7 @@ public class Device { public static final long MASTER_ID = 1; public static final int MAXIMUM_DEVICE_ID = 256; + public static final int MAX_REGISTRATION_ID = 0x3FFF; public static final List ALL_POSSIBLE_DEVICE_IDS = LongStream.range(1, MAXIMUM_DEVICE_ID).boxed().collect(Collectors.toList()); @JsonProperty diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index c948e34aa..5f4c1a02a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -1081,11 +1081,13 @@ class AccountControllerTest { when(registrationServiceClient.checkVerificationCode(sessionId, "1234", AccountController.REGISTRATION_RPC_TIMEOUT)) .thenReturn(CompletableFuture.completedFuture(true)); + final AccountAttributes attrs = new AccountAttributes(true, 1, "test", "", true, new Device.DeviceCapabilities()); + resources.getJerseyTest() .target("/v1/accounts/code/1234") .request() .header(HttpHeaders.AUTHORIZATION, AuthHelper.getProvisioningAuthHeader(SENDER, "bar")) - .put(Entity.entity(new AccountAttributes(), MediaType.APPLICATION_JSON_TYPE), AccountIdentityResponse.class); + .put(Entity.entity(attrs, MediaType.APPLICATION_JSON_TYPE), AccountIdentityResponse.class); verify(accountsManager).create(eq(SENDER), eq("bar"), any(), any(), anyList()); 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 0bc6885aa..e7d2ad175 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java @@ -16,12 +16,14 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.fasterxml.jackson.core.JsonProcessingException; import com.google.i18n.phonenumbers.PhoneNumberUtil; import io.dropwizard.testing.junit5.DropwizardExtensionsSupport; import io.dropwizard.testing.junit5.ResourceExtension; import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.Base64; +import java.util.HashMap; import java.util.Map; import java.util.Optional; import java.util.UUID; @@ -129,6 +131,51 @@ class RegistrationControllerTest { } } + static Stream invalidRegistrationId() { + return Stream.of( + Arguments.of(Optional.of(1), Optional.of(1), 200), + Arguments.of(Optional.of(1), Optional.empty(), 200), + Arguments.of(Optional.of(0x3FFF), Optional.empty(), 200), + Arguments.of(Optional.empty(), Optional.of(1), 422), + Arguments.of(Optional.of(Integer.MAX_VALUE), Optional.empty(), 422), + Arguments.of(Optional.of(0x3FFF + 1), Optional.empty(), 422), + Arguments.of(Optional.of(1), Optional.of(0x3FFF + 1), 422) + ); + } + + @ParameterizedTest + @MethodSource() + void invalidRegistrationId(Optional registrationId, Optional pniRegistrationId, int statusCode) throws InterruptedException, JsonProcessingException { + final Invocation.Builder request = resources.getJerseyTest() + .target("/v1/registration") + .request() + .header(HttpHeaders.AUTHORIZATION, AuthHelper.getProvisioningAuthHeader(NUMBER, PASSWORD)); + when(registrationServiceClient.getSession(any(), any())) + .thenReturn( + CompletableFuture.completedFuture( + Optional.of(new RegistrationServiceSession(new byte[16], NUMBER, true, null, null, null, + SESSION_EXPIRATION_SECONDS)))); + when(accountsManager.create(any(), any(), any(), any(), any())) + .thenReturn(mock(Account.class)); + + final String recoveryPassword = encodeRecoveryPassword(new byte[0]); + + final Map accountAttrs = new HashMap<>(); + accountAttrs.put("recoveryPassword", recoveryPassword); + registrationId.ifPresent(id -> accountAttrs.put("registrationId", id)); + pniRegistrationId.ifPresent(id -> accountAttrs.put("pniRegistrationId", id)); + final String json = SystemMapper.jsonMapper().writeValueAsString(Map.of( + "sessionId", encodeSessionId("sessionId"), + "recoveryPassword", recoveryPassword, + "accountAttributes", accountAttrs, + "skipDeviceTransfer", true + )); + + try (Response response = request.post(Entity.json(json))) { + assertEquals(statusCode, response.getStatus()); + } + } + @Test void missingBasicAuthorization() { final Invocation.Builder request = resources.getJerseyTest() @@ -745,7 +792,8 @@ class RegistrationControllerTest { "sessionId": "%s", "recoveryPassword": "%s", "accountAttributes": { - "recoveryPassword": "%s" + "recoveryPassword": "%s", + "registrationId": 1 }, "skipDeviceTransfer": %s }