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>
In configuration and key management, long-term private and public keys
are octet strings. Private keys are generally stored in disk files,
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,
algorithm; when it's loaded the algorithm will be known from context.
The group id 00000000 is special. It should contain only one key,
for (const struct sigscheme_info *scheme=sigschemes;
scheme->name;
scheme++) {
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 };
st->databuf.start=st->databuf.base;
st->databuf.size=got;
struct cloc loc = { .file=st->path.buffer, .line=0 };
+ /* 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)",
FILE *maybe_f, bool_t unsup,
const char *message, va_list args)
{
FILE *maybe_f, bool_t unsup,
const char *message, va_list args)
{
- int class=unsup ? M_DEBUG : M_ERR;
slilog_part(l->u.tryload.log,class,"%s: ",l->what);
vslilog(l->u.tryload.log,class,message,args);
}
slilog_part(l->u.tryload.log,class,"%s: ",l->what);
vslilog(l->u.tryload.log,class,message,args);
}
}
#define LDFATAL(...) ({ load_err(l,0,0,0,__VA_ARGS__); goto error_out; })
}
#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 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; \
#define KEYFILE_GET(is) ({ \
uint##is##_t keyfile_get_tmp=keyfile_get_##is(l,f); \
if (!l->postreadcheck(l,f)) goto error_out; \
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) {
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);
" string from SSH1 private keyfile\n");
}
FREE(b);
cipher_type=fgetc(f);
KEYFILE_GET(32); /* "Reserved data" */
if (cipher_type != 0) {
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 */
}
/* Read the public key */
struct buffer_if *privkeydata,
struct sigprivkey_if **sigpriv_r,
struct log_if *log, struct cloc loc);
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;
struct sigscheme_info {
const char *name;