From 1ebc17352fc936ad41c47900cdda42bd21cd485b Mon Sep 17 00:00:00 2001 From: Ravi Khadiwala Date: Tue, 2 Apr 2024 14:39:51 -0500 Subject: [PATCH] Check presentation before verifying the signature --- .../textsecuregcm/backup/BackupManager.java | 56 ++++++++++--------- .../backup/BackupAuthTestUtil.java | 9 ++- .../backup/BackupManagerTest.java | 43 ++++++++++++++ 3 files changed, 80 insertions(+), 28 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java index 22ddd80ed..4e37fd56e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/backup/BackupManager.java @@ -102,7 +102,7 @@ public class BackupManager { // Note: this is a special case where we can't validate the presentation signature against the stored public key // because we are currently setting it. We check against the provided public key, but we must also verify that // there isn't an existing, different stored public key for the backup-id (verified with a condition expression) - final BackupTier backupTier = verifySignatureAndCheckPresentation(presentation, signature, publicKey); + final BackupTier backupTier = verifyPresentation(presentation).verifySignature(signature, publicKey); if (backupTier.compareTo(BackupTier.MESSAGES) < 0) { Metrics.counter(ZK_AUTHZ_FAILURE_COUNTER_NAME).increment(); throw Status.PERMISSION_DENIED @@ -444,6 +444,7 @@ public class BackupManager { public CompletableFuture authenticateBackupUser( final BackupAuthCredentialPresentation presentation, final byte[] signature) { + final PresentationSignatureVerifier signatureVerifier = verifyPresentation(presentation); return backupsDb .retrieveAuthenticationData(presentation.getBackupId()) .thenApply(optionalAuthenticationData -> { @@ -457,7 +458,7 @@ public class BackupManager { }); return new AuthenticatedBackupUser( presentation.getBackupId(), - verifySignatureAndCheckPresentation(presentation, signature, authenticationData.publicKey()), + signatureVerifier.verifySignature(signature, authenticationData.publicKey()), authenticationData.backupDir(), authenticationData.mediaDir()); }) .thenApply(result -> { @@ -527,26 +528,17 @@ public class BackupManager { .toFuture(); } + interface PresentationSignatureVerifier { + BackupTier verifySignature(byte[] signature, ECPublicKey publicKey); + } /** - * Verify the presentation and return the extracted backup tier + * Verify the presentation was issued by us, which should be done before checking the stored public key * * @param presentation A ZK credential presentation that encodes the backupId and the receipt level of the requester - * @return The backup tier this presentation supports + * @return A function that can be used to verify a signature provided with the presentation */ - private BackupTier verifySignatureAndCheckPresentation( - final BackupAuthCredentialPresentation presentation, - final byte[] signature, - final ECPublicKey publicKey) { - if (!publicKey.verifySignature(presentation.serialize(), signature)) { - Metrics.counter(ZK_AUTHN_COUNTER_NAME, - SUCCESS_TAG_NAME, String.valueOf(false), - FAILURE_REASON_TAG_NAME, "signature_validation") - .increment(); - throw Status.UNAUTHENTICATED - .withDescription("backup auth credential presentation signature verification failed") - .asRuntimeException(); - } + private PresentationSignatureVerifier verifyPresentation(final BackupAuthCredentialPresentation presentation) { try { presentation.verify(clock.instant(), serverSecretParams); } catch (VerificationFailedException e) { @@ -559,16 +551,26 @@ public class BackupManager { .withCause(e) .asRuntimeException(); } - - return BackupTier - .fromReceiptLevel(presentation.getReceiptLevel()) - .orElseThrow(() -> { - Metrics.counter(ZK_AUTHN_COUNTER_NAME, - SUCCESS_TAG_NAME, String.valueOf(false), - FAILURE_REASON_TAG_NAME, "invalid_receipt_level") - .increment(); - return Status.PERMISSION_DENIED.withDescription("invalid receipt level").asRuntimeException(); - }); + return (signature, publicKey) -> { + if (!publicKey.verifySignature(presentation.serialize(), signature)) { + Metrics.counter(ZK_AUTHN_COUNTER_NAME, + SUCCESS_TAG_NAME, String.valueOf(false), + FAILURE_REASON_TAG_NAME, "signature_validation") + .increment(); + throw Status.UNAUTHENTICATED + .withDescription("backup auth credential presentation signature verification failed") + .asRuntimeException(); + } + return BackupTier + .fromReceiptLevel(presentation.getReceiptLevel()) + .orElseThrow(() -> { + Metrics.counter(ZK_AUTHN_COUNTER_NAME, + SUCCESS_TAG_NAME, String.valueOf(false), + FAILURE_REASON_TAG_NAME, "invalid_receipt_level") + .increment(); + return Status.PERMISSION_DENIED.withDescription("invalid receipt level").asRuntimeException(); + }); + }; } @VisibleForTesting diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupAuthTestUtil.java b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupAuthTestUtil.java index 9efade859..4dbfdbfde 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupAuthTestUtil.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupAuthTestUtil.java @@ -37,9 +37,16 @@ public class BackupAuthTestUtil { public BackupAuthCredentialPresentation getPresentation( final BackupTier backupTier, final byte[] backupKey, final UUID aci) throws VerificationFailedException { + return getPresentation(params, backupTier, backupKey, aci); + } + + public BackupAuthCredentialPresentation getPresentation( + GenericServerSecretParams params, final BackupTier backupTier, final byte[] backupKey, final UUID aci) + throws VerificationFailedException { final BackupAuthCredentialRequestContext ctx = BackupAuthCredentialRequestContext.create(backupKey, aci); return ctx.receiveResponse( - ctx.getRequest().issueCredential(clock.instant().truncatedTo(ChronoUnit.DAYS), backupTier.getReceiptLevel(), params), + ctx.getRequest() + .issueCredential(clock.instant().truncatedTo(ChronoUnit.DAYS), backupTier.getReceiptLevel(), params), params.getPublicParams(), backupTier.getReceiptLevel()) .present(params.getPublicParams()); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java index 7171ff4de..b08be325a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/backup/BackupManagerTest.java @@ -57,6 +57,7 @@ import org.junit.jupiter.params.provider.ValueSource; import org.signal.libsignal.protocol.InvalidKeyException; import org.signal.libsignal.protocol.ecc.Curve; import org.signal.libsignal.protocol.ecc.ECKeyPair; +import org.signal.libsignal.zkgroup.GenericServerSecretParams; import org.signal.libsignal.zkgroup.VerificationFailedException; import org.signal.libsignal.zkgroup.backups.BackupAuthCredentialPresentation; import org.whispersystems.textsecuregcm.auth.AuthenticatedBackupUser; @@ -170,6 +171,48 @@ public class BackupManagerTest { backupUser); } + @Test + public void invalidPresentationNoPublicKey() throws VerificationFailedException { + final BackupAuthCredentialPresentation invalidPresentation = backupAuthTestUtil.getPresentation( + GenericServerSecretParams.generate(), + BackupTier.MESSAGES, backupKey, aci); + + final ECKeyPair keyPair = Curve.generateKeyPair(); + + // haven't set a public key yet, but should fail before hitting the database anyway + assertThatExceptionOfType(StatusRuntimeException.class) + .isThrownBy(() -> backupManager.authenticateBackupUser( + invalidPresentation, + keyPair.getPrivateKey().calculateSignature(invalidPresentation.serialize()))) + .extracting(StatusRuntimeException::getStatus) + .extracting(Status::getCode) + .isEqualTo(Status.UNAUTHENTICATED.getCode()); + } + + + @Test + public void invalidPresentationCorrectSignature() throws VerificationFailedException { + final BackupAuthCredentialPresentation presentation = backupAuthTestUtil.getPresentation( + BackupTier.MESSAGES, backupKey, aci); + final BackupAuthCredentialPresentation invalidPresentation = backupAuthTestUtil.getPresentation( + GenericServerSecretParams.generate(), + BackupTier.MESSAGES, backupKey, aci); + + final ECKeyPair keyPair = Curve.generateKeyPair(); + backupManager.setPublicKey( + presentation, + keyPair.getPrivateKey().calculateSignature(presentation.serialize()), + keyPair.getPublicKey()).join(); + + assertThatExceptionOfType(StatusRuntimeException.class) + .isThrownBy(() -> backupManager.authenticateBackupUser( + invalidPresentation, + keyPair.getPrivateKey().calculateSignature(invalidPresentation.serialize()))) + .extracting(StatusRuntimeException::getStatus) + .extracting(Status::getCode) + .isEqualTo(Status.UNAUTHENTICATED.getCode()); + } + @Test public void unknownPublicKey() throws VerificationFailedException { final BackupAuthCredentialPresentation presentation = backupAuthTestUtil.getPresentation(