Check presentation before verifying the signature

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<AuthenticatedBackupUser> 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(
+  private PresentationSignatureVerifier verifyPresentation(final BackupAuthCredentialPresentation presentation) {
-      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();
-    }
     try {
       presentation.verify(clock.instant(), serverSecretParams);
     } catch (VerificationFailedException e) {
@@ -559,7 +551,16 @@ public class BackupManager {
           .withCause(e)
           .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(() -> {
@@ -569,6 +570,7 @@ public class BackupManager {
                 .increment();
             return Status.PERMISSION_DENIED.withDescription("invalid receipt level").asRuntimeException();
           });
+    };
   }
 
   @VisibleForTesting

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());

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(