From 9fcd759f42a3df955982a753162837d2034e26a8 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Fri, 27 Sep 2019 18:40:42 +0100
Subject: [PATCH] sig: Move unmarshalling responsibility into algorithm

Because site wants to first unpick the packet, and only later actually
check the signature, we provide two entrypoints.  The first, `unpick',
basically just computes the length.  So the result of `unpick' is
simply a note of the part of the buffer which contains the signature.

The alternative would be to have site.c handle the length, so there
would be one entrypoint `check' which would get a byte block.  This
would move complexity from the `unpick'/`check' interface to the
`sign' interface (which would have to negotiate about space).  It
would mean that for algorithms where signatures are of fixed size, we
couldn't omit the length field.

rsa.c needs to do some shenanigans: because it wants to use
mpz_set_str (for historical reasons), it needs the buffer to be
nul-terminated.  So `unpick' checks that there will be a spare byte
afterwards into which we can write the nul.  `check' writes the nul -
and puts the previous character back, so that we don't have to write
weird stuff in the algorithm api.  Doing better than this would be
turd-polishing since this algorithm is obsolete.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 rsa.c    | 27 +++++++++++++++++++++++++--
 secnet.h | 10 +++++++++-
 site.c   | 22 ++++++++--------------
 3 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/rsa.c b/rsa.c
index dfbc237..49672c2 100644
--- a/rsa.c
+++ b/rsa.c
@@ -183,9 +183,27 @@ static bool_t rsa_sign(void *sst, uint8_t *data, int32_t datalen,
     return ok;
 }
 
+static bool_t rsa_sig_unpick(void *sst, struct buffer_if *msg,
+			     struct alg_msg_data *sig)
+{
+    uint8_t *lp = buf_unprepend(msg, 2);
+    if (!lp) return False;
+    sig->siglen = get_uint16(lp);
+    sig->sigstart = buf_unprepend(msg, sig->siglen);
+    if (!sig->sigstart) return False;
+
+    /* In `rsa_sig_check' below, we assume that we can write a nul
+     * terminator following the signature.  Make sure there's enough space.
+     */
+    if (msg->start >= msg->base + msg->alloclen)
+	return False;
+
+    return True;
+}
+
 static sig_checksig_fn rsa_sig_check;
 static bool_t rsa_sig_check(void *sst, uint8_t *data, int32_t datalen,
-			    cstring_t signature)
+			    const struct alg_msg_data *sig)
 {
     struct rsapub *st=sst;
     MP_INT a, b, c;
@@ -197,7 +215,11 @@ static bool_t rsa_sig_check(void *sst, uint8_t *data, int32_t datalen,
 
     emsa_pkcs1(&st->n, &a, data, datalen);
 
-    mpz_set_str(&b, signature, 16);
+    /* Terminate signature with a '0' - already checked that this will fit */
+    int save = sig->sigstart[sig->siglen];
+    sig->sigstart[sig->siglen] = 0;
+    mpz_set_str(&b, sig->sigstart, 16);
+    sig->sigstart[sig->siglen] = save;
 
     mpz_powm(&c, &b, &st->e, &st->n);
 
@@ -223,6 +245,7 @@ static list_t *rsapub_apply(closure_t *self, struct cloc loc, dict_t *context,
     st->cl.apply=NULL;
     st->cl.interface=&st->ops;
     st->ops.st=st;
+    st->ops.unpick=rsa_sig_unpick;
     st->ops.check=rsa_sig_check;
     st->loc=loc;
 
diff --git a/secnet.h b/secnet.h
index bd4362e..5fafdcb 100644
--- a/secnet.h
+++ b/secnet.h
@@ -377,6 +377,11 @@ extern init_module log_module;
 
 struct buffer_if;
 
+struct alg_msg_data {
+    uint8_t *sigstart;
+    int32_t siglen;
+};
+
 /* PURE closure requires no interface */
 
 /* RESOLVER interface */
@@ -413,10 +418,13 @@ struct random_if {
 
 /* SIGPUBKEY interface */
 
+typedef bool_t sig_unpick_fn(void *sst, struct buffer_if *msg,
+			     struct alg_msg_data *sig);
 typedef bool_t sig_checksig_fn(void *st, uint8_t *data, int32_t datalen,
-			       cstring_t signature);
+			       const struct alg_msg_data *sig);
 struct sigpubkey_if {
     void *st;
+    sig_unpick_fn *unpick;
     sig_checksig_fn *check;
 };
 
diff --git a/site.c b/site.c
index 6e5c5a6..73d5b64 100644
--- a/site.c
+++ b/site.c
@@ -534,8 +534,7 @@ struct msg {
     int32_t pklen;
     char *pk;
     int32_t hashlen;
-    int32_t siglen;
-    char *sig;
+    struct alg_msg_data sig;
 };
 
 static int32_t wait_timeout(struct site *st) {
@@ -745,17 +744,12 @@ static bool_t unpick_msg(struct site *st, uint32_t type,
     CHECK_AVAIL(msg,m->pklen);
     m->pk=buf_unprepend(msg,m->pklen);
     m->hashlen=msg->start-m->hashstart;
-    CHECK_AVAIL(msg,2);
-    m->siglen=buf_unprepend_uint16(msg);
-    CHECK_AVAIL(msg,m->siglen);
-    m->sig=buf_unprepend(msg,m->siglen);
-    CHECK_EMPTY(msg);
 
-    /* In `process_msg3_msg4' below, we assume that we can write a nul
-     * terminator following the signature.  Make sure there's enough space.
-     */
-    if (msg->start >= msg->base + msg->alloclen)
+    if (!st->pubkey->unpick(st->pubkey->st,msg,&m->sig)) {
 	return False;
+    }
+
+    CHECK_EMPTY(msg);
 
     return True;
 }
@@ -898,9 +892,9 @@ static bool_t process_msg3_msg4(struct site *st, struct msg *m)
     hst=st->hash->init();
     st->hash->update(hst,m->hashstart,m->hashlen);
     st->hash->final(hst,hash);
-    /* Terminate signature with a '0' - already checked that this will fit */
-    m->sig[m->siglen]=0;
-    if (!st->pubkey->check(st->pubkey->st,hash,st->hash->len,m->sig)) {
+    if (!st->pubkey->check(st->pubkey->st,
+			   hash,st->hash->len,
+			   &m->sig)) {
 	slog(st,LOG_SEC,"msg3/msg4 signature failed check!");
 	free(hash);
 	return False;
-- 
2.30.2