Add debugging context to signature validation failures
This commit is contained in:
		
							parent
							
								
									8a587d1d12
								
							
						
					
					
						commit
						a733f5c615
					
				|  | @ -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(), | ||||
|  |  | |||
|  | @ -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()); | ||||
|  |  | |||
|  | @ -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<SignedPreKey<?>> 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<Response> 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); | ||||
|     } | ||||
| 
 | ||||
|  |  | |||
|  | @ -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, | ||||
|  |  | |||
|  | @ -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<Byte, Integer> pniRegistrationIds) implements PhoneVerificationRequest { | ||||
| 
 | ||||
|   @AssertTrue | ||||
|   public boolean isSignatureValidOnEachSignedPreKey() { | ||||
|   public boolean isSignatureValidOnEachSignedPreKey(@Nullable final String userAgent) { | ||||
|     List<SignedPreKey<?>> 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 | ||||
|  |  | |||
|  | @ -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<Byte, Integer> pniRegistrationIds) { | ||||
| 
 | ||||
|   @AssertTrue | ||||
|   public boolean isSignatureValidOnEachSignedPreKey() { | ||||
|   public boolean isSignatureValidOnEachSignedPreKey(@Nullable final String userAgent) { | ||||
|     List<SignedPreKey<?>> 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"); | ||||
|   } | ||||
| 
 | ||||
| } | ||||
|  |  | |||
|  | @ -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<SignedPreKey<?>> spks) { | ||||
|     final boolean success = spks.stream().allMatch(spk -> spk.signatureValid(identityKey)); | ||||
|   public static boolean validatePreKeySignatures(final IdentityKey identityKey, | ||||
|       final Collection<SignedPreKey<?>> 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; | ||||
|  |  | |||
|  | @ -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 | ||||
|  |  | |||
		Loading…
	
		Reference in New Issue
	
	 Jon Chambers
						Jon Chambers