Add missing sanity checks in ssh_dss_verify.

The standard says we should be checking that both r,s are in the range
[1,q-1]. Previously we were effectively reducing s mod q in the course
of inversion, and modinv() was guaranteeing never to return zero; the
remaining missing checks were benign. But the change from Bignum to
mp_int altered the error behaviour, and combined with the missing
upper bound check on s, made it possible to continue verification with
w == 0 mod q, which is a bad case.

Added a small DSA test case, including a check that none of these
types of signatures validates.

sshdss.c

@@ -132,7 +132,13 @@ static bool dss_verify(ssh_key *key, ptrlen sig, ptrlen data)
 	return false;
     }
 
-    if (mp_eq_integer(s, 0)) {
+    /* Basic sanity checks: 0 < r,s < q */
+    unsigned invalid = 0;
+    invalid |= mp_eq_integer(r, 0);
+    invalid |= mp_eq_integer(s, 0);
+    invalid |= mp_cmp_hs(r, dss->q);
+    invalid |= mp_cmp_hs(s, dss->q);
+    if (invalid) {
         mp_free(r);
         mp_free(s);
         return false;

test/cryptsuite.py

@@ -1242,6 +1242,28 @@ culpa qui officia deserunt mollit anim id est laborum.
              'f91e026778bb2d389a9dd88153405189e6ba438b213c5387284103d2267fd055'
         )
 
+    def testDSA(self):
+        p = 0xe93618c54716992ffd54e79df6e1b0edd517f7bbe4a49d64631eb3efe8105f676e8146248cfb4f05720862533210f0c2ab0f9dd61dbc0e5195200c4ebd95364b
+        q = 0xf3533bcece2e164ca7c5ce64bc1e395e9a15bbdd
+        g = 0x5ac9d0401c27d7abfbc5c17cdc1dc43323cd0ef18b79e1909bdace6d17af675a10d37dde8bd8b70e72a8666592216ccb00614629c27e870e4fbf393b812a9f05
+        y = 0xac3ddeb22d65a5a2ded4a28418b2a748d8e5e544ba5e818c137d7b042ef356b0ef6d66cfca0b3ab5affa2969522e7b07bee60562fa4869829a5afce0ad0c4cd0
+        x = 0x664f8250b7f1a5093047fe0c7fe4b58e46b73295
+        pubblob = ssh_string(b"ssh-dss") + b"".join(map(ssh2_mpint, [p,q,g,y]))
+        privblob = ssh2_mpint(x)
+        pubkey = ssh_key_new_pub('dsa', pubblob)
+        privkey = ssh_key_new_priv('dsa', pubblob, privblob)
+
+        sig = ssh_key_sign(privkey, b"hello, world", 0)
+        self.assertTrue(ssh_key_verify(pubkey, sig, b"hello, world"))
+        self.assertFalse(ssh_key_verify(pubkey, sig, b"hello, again"))
+
+        badsig0 = unhex('{:040x}{:040x}'.format(1, 0))
+        badsigq = unhex('{:040x}{:040x}'.format(1, q))
+        self.assertFalse(ssh_key_verify(pubkey, badsig0, "hello, world"))
+        self.assertFalse(ssh_key_verify(pubkey, badsigq, "hello, world"))
+        self.assertFalse(ssh_key_verify(pubkey, badsig0, "hello, again"))
+        self.assertFalse(ssh_key_verify(pubkey, badsigq, "hello, again"))
+
 class standard_test_vectors(MyTestBase):
     def testAES(self):
         def vector(cipher, key, plaintext, ciphertext):