Rust verification library: fix comparison of different length signatu…

…res (#1190)

There was a bug in the code which meant that the signatures are only
compared up to the length of the shorter signature, which means that an
attacker can just pass `v1,` as the signature and that will always pass
verification.

This change fixes it so that the length of the signature is also taken
into account when comparing, to make sure that it's always the same
length before comparing.

Manually verified all of the other libraries are correct, and added
tests to JavaScript and Go (even though they were also not affected).

Many thanks to Fredrik Meringdal (@fmeringdal) for the report.

go/webhook_test.go

@@ -101,6 +101,14 @@ func TestWebhook(t *testing.T) {
 			},
 			expectedErr: true,
 		},
+		{
+			name:        "partial signature is invalid",
+			testPayload: newTestPayload(time.Now()),
+			modifyPayload: func(tp *testPayload) {
+				tp.header.Set("svix-signature", "v1,")
+			},
+			expectedErr: true,
+		},
 		{
 			name:        "old timestamp fails",
 			testPayload: newTestPayload(time.Now().Add(tolerance * -1)),

javascript/src/webhook.test.ts

@@ -103,6 +103,23 @@ test("invalid signature throws error", () => {
   }).toThrowError(WebhookVerificationError);
 });
 
+test("partial signature throws error", () => {
+  const wh = new Webhook("MfKQ9r8GKYqrTwjUPD8ILPZIo2LaLaSw");
+
+  const testPayload = new TestPayload();
+  testPayload.header["svix-signature"] = testPayload.header["svix-signature"].slice(0, 8);
+
+  expect(() => {
+    wh.verify(testPayload.payload, testPayload.header);
+  }).toThrowError(WebhookVerificationError);
+
+  testPayload.header["svix-signature"] = "v1,";
+
+  expect(() => {
+    wh.verify(testPayload.payload, testPayload.header);
+  }).toThrowError(WebhookVerificationError);
+});
+
 test("valid signature is valid and returns valid json", () => {
   const wh = new Webhook("MfKQ9r8GKYqrTwjUPD8ILPZIo2LaLaSw");
 

rust/src/webhooks.rs

@@ -86,10 +86,13 @@ impl Webhook {
             .filter_map(|x| x.split_once(','))
             .filter(|x| x.0 == SIGNATURE_VERSION)
             .any(|x| {
-                x.1.bytes()
-                    .zip(expected_signature.bytes())
-                    .fold(0, |acc, (a, b)| acc | (a ^ b))
-                    == 0
+                (x.1.len() == expected_signature.len())
+                    && (x
+                        .1
+                        .bytes()
+                        .zip(expected_signature.bytes())
+                        .fold(0, |acc, (a, b)| acc | (a ^ b))
+                        == 0)
             })
             .then_some(())
             .ok_or(WebhookError::InvalidSignature)
@@ -223,6 +226,43 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_verify_partial_signature() {
+        let secret = "whsec_C2FVsBQIhrscChlQIMV+b5sSYspob7oD".to_owned();
+        let msg_id = "msg_27UH4WbU6Z5A5EzD8u03UvzRbpk";
+        let payload = br#"{"email":"test@example.com","username":"test_user"}"#;
+        let wh = Webhook::new(&secret).unwrap();
+
+        let signature = wh
+            .sign(msg_id, OffsetDateTime::now_utc().unix_timestamp(), payload)
+            .unwrap();
+
+        // Just `v1,`
+        for mut headers in [
+            get_svix_headers(msg_id, &signature),
+            get_unbranded_headers(msg_id, &signature),
+        ] {
+            let partial = format!(
+                "{},",
+                signature.split(',').collect::<Vec<&str>>().first().unwrap()
+            );
+            headers.insert(SVIX_MSG_SIGNATURE_KEY, partial.parse().unwrap());
+            headers.insert(UNBRANDED_MSG_SIGNATURE_KEY, partial.parse().unwrap());
+            assert!(wh.verify(payload, &headers).is_err());
+        }
+
+        // Non-empty but still partial signature (first few bytes)
+        for mut headers in [
+            get_svix_headers(msg_id, &signature),
+            get_unbranded_headers(msg_id, &signature),
+        ] {
+            let partial = &signature[0..8];
+            headers.insert(SVIX_MSG_SIGNATURE_KEY, partial.parse().unwrap());
+            headers.insert(UNBRANDED_MSG_SIGNATURE_KEY, partial.parse().unwrap());
+            assert!(wh.verify(payload, &headers).is_err());
+        }
+    }
+
     #[test]
     fn test_verify_incorrect_timestamp() {
         let secret = "whsec_C2FVsBQIhrscChlQIMV+b5sSYspob7oD".to_owned();