priv-cache etc.: private key algorithm is specified in key id

The idea that we would try various different algorithms to see who
could load a private key was a remnant of a previous design of key id
system.  The actually implemnted arrangements identify the algorithm
in the key id, so there is no need for probing.

In this commit we fix the spec, and change the calling convention for
loadpriv.  Now that we only call loadpriv once, it is allowed to
modify the buffer contents (although nothing makes use of this
relaxation right now).

We change loadpriv's one call site in privcache.c and its (currently
only) implementation, in rsa.c.

In privcache, the error message now definitely means that the algid
was unrecognised, so change it.

In rsa.c we make the log level M_ERR unconditionally (although in fact
verror_tryload now always gets unsup==0).  We delete the now-unused
LDUNSUP from rsa.c, but there is some more intrusive refactoring to do
next to tidy up now-unused stuff.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/NOTES b/NOTES
index ca8c04ac70f0092fd32cd41e1cc4477686279f66..8f92f6911fa203ec97f38dbec352fabb77655895 100644 (file)
--- a/NOTES
+++ b/NOTES
@@ -188,9 +188,10 @@ When deciding which public keys to accept, a relier should:
 
 In configuration and key management, long-term private and public keys
 are octet strings.  Private keys are generally stored in disk files,
-one key per file.  The octet string for a private key must identify
-the algorithm (although actually this is wrong and are going to change
-it later)..  The octet string for a public key need not identify the
+one key per file.  The octet string for a private key should identify
+the algorithm so that passing the private key to the code for the 
+wrong algorithm does not produce results which would leak or weaken
+the key.  The octet string for a public key need not identify the
 algorithm; when it's loaded the algorithm will be known from context.
 
 The group id 00000000 is special.  It should contain only one key,

diff --git a/privcache.c b/privcache.c
index 81b04fcc65f456e3585001fd7425236b37d66ba5..b8dc2adee323ce23b2625ac6fe9a313901d0b388 100644 (file)
--- a/privcache.c
+++ b/privcache.c
@@ -76,6 +76,9 @@ static struct sigprivkey_if *uncached_get(struct privcache *st,
     for (const struct sigscheme_info *scheme=sigschemes;
         scheme->name;
         scheme++) {
+       if (scheme->algid != id->b[GRPIDSZ])
+           continue;
+
        st->databuf.start=st->databuf.base;
        st->databuf.size=got;
        struct cloc loc = { .file=st->path.buffer, .line=0 };
@@ -94,9 +97,11 @@ static struct sigprivkey_if *uncached_get(struct privcache *st,
            }
            goto out;
        }
+       /* loadpriv will have logged */
+       goto out;
     }
 
-    slilog(log,M_ERR,"private key file %s not loaded (not recognised?)",
+    slilog(log,M_ERR,"private key file %s not loaded (unknown algid)",
           st->path.buffer);
 
   out:

diff --git a/rsa.c b/rsa.c
index b633f72d9eca65aae395609688a7596bdf7f47ed..f386b41ce200f688d0838b7f0db3baa004697009 100644 (file)
--- a/rsa.c
+++ b/rsa.c
@@ -76,7 +76,7 @@ static void verror_tryload(struct load_ctx *l, struct cloc loc,
                           FILE *maybe_f, bool_t unsup,
                           const char *message, va_list args)
 {
-    int class=unsup ? M_DEBUG : M_ERR;
+    int class=M_ERR;
     slilog_part(l->u.tryload.log,class,"%s: ",l->what);
     vslilog(l->u.tryload.log,class,message,args);
 }
@@ -442,9 +442,7 @@ bool_t rsa1_loadpub(const struct sigscheme_info *algo,
 }
 
 #define LDFATAL(...)      ({ load_err(l,0,0,0,__VA_ARGS__); goto error_out; })
-#define LDUNSUP(...)      ({ load_err(l,0,0,1,__VA_ARGS__); goto error_out; })
 #define LDFATAL_FILE(...) ({ load_err(l,0,f,0,__VA_ARGS__); goto error_out; })
-#define LDUNSUP_FILE(...) ({ load_err(l,0,f,1,__VA_ARGS__); goto error_out; })
 #define KEYFILE_GET(is)   ({                                   \
        uint##is##_t keyfile_get_tmp=keyfile_get_##is(l,f);     \
        if (!l->postreadcheck(l,f)) goto error_out;             \
@@ -526,7 +524,7 @@ static struct rsapriv *rsa_loadpriv_core(struct load_ctx *l,
     length=strlen(AUTHFILE_ID_STRING)+1;
     b=safe_malloc(length,"rsapriv_apply");
     if (fread(b,length,1,f)!=1 || memcmp(b,AUTHFILE_ID_STRING,length)!=0) {
-       LDUNSUP_FILE("failed to read magic ID"
+       LDFATAL_FILE("failed to read magic ID"
                     " string from SSH1 private keyfile\n");
     }
     FREE(b);
@@ -534,7 +532,7 @@ static struct rsapriv *rsa_loadpriv_core(struct load_ctx *l,
     cipher_type=fgetc(f);
     KEYFILE_GET(32); /* "Reserved data" */
     if (cipher_type != 0) {
-       LDUNSUP("we don't support encrypted keyfiles\n");
+       LDFATAL("we don't support encrypted keyfiles\n");
     }
 
     /* Read the public key */

diff --git a/secnet.h b/secnet.h
index bd63a7c63f90681fd64fc34bdac97a56bbf723cb..30a171d4da262160443b9029c1c137fe8f69e027 100644 (file)
--- a/secnet.h
+++ b/secnet.h
@@ -413,13 +413,13 @@ typedef bool_t sigscheme_loadpriv(const struct sigscheme_info *algo,
                                  struct buffer_if *privkeydata,
                                  struct sigprivkey_if **sigpriv_r,
                                  struct log_if *log, struct cloc loc);
-  /* privkeydata may contain data for any algorithm, not necessarily
-   * this one!  If it is not for this algorithm, return False and do
-   * not log anything (other than at M_DEBUG).  If it *is* for this
-   * algorithm but is wrong, log at M_ERROR.
-   * On entry privkeydata->base==start.  loadpriv may modify base and
-   * size, but not anything else.  So it may use unprepend and
-   * unappend. */
+  /* Ideally, check whether privkeydata contains data for any algorithm.
+   * That avoids security problems if a key file is misidentified (which
+   * might happen if the file is simply renamed).
+   * If there is an error (including that the key data is not for this
+   * algorithm, return False and log an error at M_ERROR.
+   * On entry privkeydata->base==start.  loadpriv may modify
+   * privkeydata, including the contents. */
 
 struct sigscheme_info {
     const char *name;