From a733f5c615418168efdbc891b3c12b6d2747b55d Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Mon, 25 Mar 2024 18:20:30 -0400 Subject: [PATCH] Add debugging context to signature validation failures --- .../controllers/AccountControllerV2.java | 10 +++++++ .../controllers/DeviceController.java | 12 ++++++--- .../controllers/KeysController.java | 13 ++++++--- .../controllers/RegistrationController.java | 4 +++ .../entities/ChangeNumberRequest.java | 5 ++-- ...eNumberIdentityKeyDistributionRequest.java | 6 ++--- .../entities/PreKeySignatureValidator.java | 27 ++++++++++++------- .../entities/RegistrationRequest.java | 8 +++--- 8 files changed, 58 insertions(+), 27 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 4a33df350..a3bcadc96 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java @@ -22,6 +22,7 @@ import io.swagger.v3.oas.annotations.responses.ApiResponse; import java.time.Instant; import java.util.Optional; import java.util.UUID; +import javax.annotation.Nullable; import javax.validation.Valid; import javax.validation.constraints.NotNull; import javax.ws.rs.BadRequestException; @@ -124,6 +125,10 @@ public class AccountControllerV2 { } catch (final UnrecognizedUserAgentException ignored) { } + if (!request.isSignatureValidOnEachSignedPreKey(userAgentString)) { + throw new WebApplicationException("Invalid signature", 422); + } + final String number = request.number(); // Only verify and check reglock if there's a data change to be made... @@ -195,12 +200,17 @@ public class AccountControllerV2 { content = @Content(schema = @Schema(implementation = StaleDevices.class))) public AccountIdentityResponse distributePhoneNumberIdentityKeys( @Mutable @Auth final AuthenticatedAccount authenticatedAccount, + @HeaderParam(HttpHeaders.USER_AGENT) @Nullable final String userAgentString, @NotNull @Valid final PhoneNumberIdentityKeyDistributionRequest request) { if (!authenticatedAccount.getAuthenticatedDevice().isPrimary()) { throw new ForbiddenException(); } + if (!request.isSignatureValidOnEachSignedPreKey(userAgentString)) { + throw new WebApplicationException("Invalid signature", 422); + } + try { final Account updatedAccount = changeNumberManager.updatePniKeys( authenticatedAccount.getAccount(), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java index 6e504344b..c51cf22fb 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/DeviceController.java @@ -26,7 +26,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.UUID; -import java.util.concurrent.CompletableFuture; +import javax.annotation.Nullable; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; import javax.validation.Valid; @@ -64,7 +64,6 @@ import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.Device; import org.whispersystems.textsecuregcm.storage.Device.DeviceCapabilities; import org.whispersystems.textsecuregcm.storage.DeviceSpec; -import org.whispersystems.textsecuregcm.util.Util; import org.whispersystems.textsecuregcm.util.VerificationCode; import org.whispersystems.websocket.auth.Mutable; import org.whispersystems.websocket.auth.ReadOnly; @@ -184,6 +183,7 @@ public class DeviceController { name = "Retry-After", description = "If present, an positive integer indicating the number of seconds before a subsequent attempt could succeed")) public DeviceResponse linkDevice(@HeaderParam(HttpHeaders.AUTHORIZATION) BasicAuthorizationHeader authorizationHeader, + @HeaderParam(HttpHeaders.USER_AGENT) @Nullable String userAgent, @NotNull @Valid LinkDeviceRequest linkDeviceRequest, @Context ContainerRequest containerRequest) throws RateLimitExceededException, DeviceLimitExceededException { @@ -199,9 +199,13 @@ public class DeviceController { final boolean allKeysValid = PreKeySignatureValidator.validatePreKeySignatures(account.getIdentityKey(IdentityType.ACI), - List.of(deviceActivationRequest.aciSignedPreKey(), deviceActivationRequest.aciPqLastResortPreKey())) + List.of(deviceActivationRequest.aciSignedPreKey(), deviceActivationRequest.aciPqLastResortPreKey()), + userAgent, + "link-device") && PreKeySignatureValidator.validatePreKeySignatures(account.getIdentityKey(IdentityType.PNI), - List.of(deviceActivationRequest.pniSignedPreKey(), deviceActivationRequest.pniPqLastResortPreKey())); + List.of(deviceActivationRequest.pniSignedPreKey(), deviceActivationRequest.pniPqLastResortPreKey()), + userAgent, + "link-device"); if (!allKeysValid) { throw new WebApplicationException(Response.status(422).build()); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java index 515fdf990..fdd3bada6 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/KeysController.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import javax.annotation.Nullable; import javax.validation.Valid; import javax.validation.constraints.NotNull; import javax.ws.rs.Consumes; @@ -130,7 +131,7 @@ public class KeysController { final Device device = auth.getAuthenticatedDevice(); final UUID identifier = account.getIdentifier(identityType); - checkSignedPreKeySignatures(setKeysRequest, account.getIdentityKey(identityType)); + checkSignedPreKeySignatures(setKeysRequest, account.getIdentityKey(identityType), userAgent); final Tag platformTag = UserAgentTagUtil.getPlatformTag(userAgent); final Tag primaryDeviceTag = Tag.of(PRIMARY_DEVICE_TAG_NAME, String.valueOf(auth.getAuthenticatedDevice().isPrimary())); @@ -174,7 +175,10 @@ public class KeysController { .thenApply(Util.ASYNC_EMPTY_RESPONSE); } - private void checkSignedPreKeySignatures(final SetKeysRequest setKeysRequest, final IdentityKey identityKey) { + private void checkSignedPreKeySignatures(final SetKeysRequest setKeysRequest, + final IdentityKey identityKey, + @Nullable final String userAgent) { + final List> signedPreKeys = new ArrayList<>(); if (setKeysRequest.pqPreKeys() != null) { @@ -190,7 +194,7 @@ public class KeysController { } final boolean allSignaturesValid = - signedPreKeys.isEmpty() || PreKeySignatureValidator.validatePreKeySignatures(identityKey, signedPreKeys); + signedPreKeys.isEmpty() || PreKeySignatureValidator.validatePreKeySignatures(identityKey, signedPreKeys, userAgent, "set-keys"); if (!allSignaturesValid) { throw new WebApplicationException("Invalid signature", 422); @@ -397,13 +401,14 @@ public class KeysController { @Deprecated(forRemoval = true) public CompletableFuture setSignedKey( @ReadOnly @Auth final AuthenticatedAccount auth, + @HeaderParam(HttpHeaders.USER_AGENT) @Nullable final String userAgent, @Valid final ECSignedPreKey signedPreKey, @QueryParam("identity") @DefaultValue("aci") final IdentityType identityType) { final UUID identifier = auth.getAccount().getIdentifier(identityType); final byte deviceId = auth.getAuthenticatedDevice().getId(); - if (!PreKeySignatureValidator.validatePreKeySignatures(auth.getAccount().getIdentityKey(identityType), List.of(signedPreKey))) { + if (!PreKeySignatureValidator.validatePreKeySignatures(auth.getAccount().getIdentityKey(identityType), List.of(signedPreKey), userAgent, "set-signed-pre-key")) { throw new WebApplicationException("Invalid signature", 422); } 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 13fcd1e18..0527e62d0 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java @@ -107,6 +107,10 @@ public class RegistrationController { final String number = authorizationHeader.getUsername(); final String password = authorizationHeader.getPassword(); + if (!registrationRequest.isEverySignedKeyValid(userAgent)) { + throw new WebApplicationException("Invalid signature", 422); + } + RateLimiter.adaptLegacyException(() -> rateLimiters.getRegistrationLimiter().validate(number)); final PhoneVerificationRequest.VerificationType verificationType = phoneVerificationTokenManager.verify(number, diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangeNumberRequest.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangeNumberRequest.java index 0f487169c..da4d443c1 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangeNumberRequest.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/ChangeNumberRequest.java @@ -68,8 +68,7 @@ public record ChangeNumberRequest( @Schema(description="the new phone-number-identity registration ID for each enabled device on the account, including this one") @NotNull Map pniRegistrationIds) implements PhoneVerificationRequest { - @AssertTrue - public boolean isSignatureValidOnEachSignedPreKey() { + public boolean isSignatureValidOnEachSignedPreKey(@Nullable final String userAgent) { List> spks = new ArrayList<>(); if (devicePniSignedPrekeys != null) { spks.addAll(devicePniSignedPrekeys.values()); @@ -77,7 +76,7 @@ public record ChangeNumberRequest( if (devicePniPqLastResortPrekeys != null) { spks.addAll(devicePniPqLastResortPrekeys.values()); } - return spks.isEmpty() || PreKeySignatureValidator.validatePreKeySignatures(pniIdentityKey, spks); + return spks.isEmpty() || PreKeySignatureValidator.validatePreKeySignatures(pniIdentityKey, spks, userAgent, "change-number"); } @AssertTrue diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/PhoneNumberIdentityKeyDistributionRequest.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/PhoneNumberIdentityKeyDistributionRequest.java index 47501985a..cfa46f232 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/PhoneNumberIdentityKeyDistributionRequest.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/PhoneNumberIdentityKeyDistributionRequest.java @@ -11,6 +11,7 @@ import io.swagger.v3.oas.annotations.media.Schema; import java.util.ArrayList; import java.util.List; import java.util.Map; +import javax.annotation.Nullable; import javax.validation.Valid; import javax.validation.constraints.AssertTrue; import javax.validation.constraints.NotNull; @@ -53,13 +54,12 @@ public record PhoneNumberIdentityKeyDistributionRequest( @Schema(description="The new registration ID to use for the phone-number identity of each device, including this one.") Map pniRegistrationIds) { - @AssertTrue - public boolean isSignatureValidOnEachSignedPreKey() { + public boolean isSignatureValidOnEachSignedPreKey(@Nullable final String userAgent) { List> spks = new ArrayList<>(devicePniSignedPrekeys.values()); if (devicePniPqLastResortPrekeys != null) { spks.addAll(devicePniPqLastResortPrekeys.values()); } - return spks.isEmpty() || PreKeySignatureValidator.validatePreKeySignatures(pniIdentityKey, spks); + return spks.isEmpty() || PreKeySignatureValidator.validatePreKeySignatures(pniIdentityKey, spks, userAgent, "distribute-pni-keys"); } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/PreKeySignatureValidator.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/PreKeySignatureValidator.java index 67a712426..e3193d585 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/PreKeySignatureValidator.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/PreKeySignatureValidator.java @@ -4,22 +4,31 @@ */ package org.whispersystems.textsecuregcm.entities; -import static com.codahale.metrics.MetricRegistry.name; - -import io.micrometer.core.instrument.Counter; import io.micrometer.core.instrument.Metrics; -import java.util.Collection; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; import org.signal.libsignal.protocol.IdentityKey; +import org.whispersystems.textsecuregcm.metrics.MetricsUtil; +import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; + +import javax.annotation.Nullable; +import java.util.Collection; public abstract class PreKeySignatureValidator { - public static final Counter INVALID_SIGNATURE_COUNTER = - Metrics.counter(name(PreKeySignatureValidator.class, "invalidPreKeySignature")); + public static final String INVALID_SIGNATURE_COUNTER_NAME = + MetricsUtil.name(PreKeySignatureValidator.class, "invalidPreKeySignature"); - public static boolean validatePreKeySignatures(final IdentityKey identityKey, final Collection> spks) { - final boolean success = spks.stream().allMatch(spk -> spk.signatureValid(identityKey)); + public static boolean validatePreKeySignatures(final IdentityKey identityKey, + final Collection> signedPreKeys, + @Nullable final String userAgent, + final String context) { + + final boolean success = signedPreKeys.stream().allMatch(signedPreKey -> signedPreKey.signatureValid(identityKey)); if (!success) { - INVALID_SIGNATURE_COUNTER.increment(); + Metrics.counter(INVALID_SIGNATURE_COUNTER_NAME, + Tags.of(UserAgentTagUtil.getPlatformTag(userAgent), Tag.of("context", context))) + .increment(); } return success; diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/entities/RegistrationRequest.java b/service/src/main/java/org/whispersystems/textsecuregcm/entities/RegistrationRequest.java index fb684f9b6..5702aa226 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/entities/RegistrationRequest.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/entities/RegistrationRequest.java @@ -14,6 +14,7 @@ import com.google.common.annotations.VisibleForTesting; import io.swagger.v3.oas.annotations.media.Schema; import java.util.List; import java.util.Optional; +import javax.annotation.Nullable; import javax.validation.Valid; import javax.validation.constraints.AssertTrue; import javax.validation.constraints.NotNull; @@ -96,8 +97,7 @@ public record RegistrationRequest(@Schema(requiredMode = Schema.RequiredMode.NOT new DeviceActivationRequest(aciSignedPreKey, pniSignedPreKey, aciPqLastResortPreKey, pniPqLastResortPreKey, apnToken, gcmToken)); } - @AssertTrue - public boolean isEverySignedKeyValid() { + public boolean isEverySignedKeyValid(@Nullable final String userAgent) { if (deviceActivationRequest().aciSignedPreKey() == null || deviceActivationRequest().pniSignedPreKey() == null || deviceActivationRequest().aciPqLastResortPreKey() == null || @@ -105,8 +105,8 @@ public record RegistrationRequest(@Schema(requiredMode = Schema.RequiredMode.NOT return false; } - return PreKeySignatureValidator.validatePreKeySignatures(aciIdentityKey(), List.of(deviceActivationRequest().aciSignedPreKey(), deviceActivationRequest().aciPqLastResortPreKey())) - && PreKeySignatureValidator.validatePreKeySignatures(pniIdentityKey(), List.of(deviceActivationRequest().pniSignedPreKey(), deviceActivationRequest().pniPqLastResortPreKey())); + return PreKeySignatureValidator.validatePreKeySignatures(aciIdentityKey(), List.of(deviceActivationRequest().aciSignedPreKey(), deviceActivationRequest().aciPqLastResortPreKey()), userAgent, "register") + && PreKeySignatureValidator.validatePreKeySignatures(pniIdentityKey(), List.of(deviceActivationRequest().pniSignedPreKey(), deviceActivationRequest().pniPqLastResortPreKey()), userAgent, "register"); } @VisibleForTesting