From 7487a709d0f0cadafba68008ddf99278d55e625b Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 13 Feb 2020 17:10:56 +0000 Subject: [PATCH] rsa: Bring hash selection in-house In 13b8fbf4548f3457b02afd36e9284d39839d6f85 sig: Move hashing into algorithm we introduced a scheme were for rsa1 the hash function is stored in the signature scheme's key structure, but provided by the caller. The intent was to allow defaulting, with context-specific overrides. However, this does not work correctly. In particular, most sites have a single "local-key" setting at the top level in the main config, but take "hash" keys from the sites file. The result is that as the various sites are initialised, ->sethash is called multiple times, once for each site. Possibly with different hash_if's. I did not foresee this and it is clearly wrong. If all the hash_if's are sha1 then this is harmless. However, they might not be, in particular if certain site(s) or vpn(s) in the sites file(s) specify a different hash. Such a configuration would be rather wrong, because it would imply reuse of the same raw RSA key material with a different hash function. (Also since the default hash is sha1 and historically the only alternative was md5, this is surely wrong simply because it implies md5 is being used somewhere.) But it has come to my attention that such installations exist. Even a non-operational, vestigial, use of a different hash, can cause lossage. To fix this properly and allow hash-agility with a single private key, we would have to have call sites continue to look up the hash, but to pass in into the signature function. This is too annoying, particularly when it is in support only of unreasonable and very old configurations. Instead, change the semantics so that the two rsa closure verbs nail down their hash at key load time, defaulting to sha1. The "hash" config key is now looked up sort of implicitly in the context. This is slightly odd, but it has roughly the right effect with sites.conf files generated by make-secnet-sites. And it is contained within the rsa1 signature scheme which is a thing we should be replacing anyway. This change makes it more clearly impossible (as it has, in fact, been since 0.4.x) to use the same loaded private key with different hashes. Installations which are only using sha1 with their rsa1 will just keep working an all is well. Installations which are using md5 everywhere can be made to work by adding a global config hash= setting in every instance. Installations which are using a mixture have a more complicated task to keep things working (maybe loading the key twice, or propagating hash information in sites files, or something), if they don't want a flag day transition to sha1. In the future for rsa1, what hash a site is using becomes a property which should be carried with public key; so a non-sha1 hash must be specified in the config file (alongside `local-key') and also documented in the sites file entry. For forthcoming non-rsa1 algorithms hash choice will be handled within the signature scheme in a less irregular way, and this "hash" key will thereby become obsolete. Reported-by: Matthew Vernon Signed-off-by: Ian Jackson --- README | 7 +++++-- rsa.c | 27 +++++++++++++++------------ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/README b/README index e23701c..200da37 100644 --- a/README +++ b/README @@ -403,8 +403,6 @@ site: dict argument key (sigpubkey closure): our peer's public key (obsolete) transform (transform closure): how to mangle packets sent between sites dh (dh closure) - hash (hash closure): used for keys whose algorithm (or public - or private key file) does not imply the hash function key-lifetime (integer): max lifetime of a session key, in ms [one hour; mobile: 2 days] setup-retries (integer): max number of times to transmit a key negotiation @@ -606,6 +604,11 @@ rsa-public: string,string arg1: encryption key (decimal) arg2: modulus (decimal) +The sigscheme is hardcoded to use sha1. Both rsa-private and +rsa-public look for the following config key in their context: + hash (hash closure): hash function [sha1] + + ** dh Defines: diff --git a/rsa.c b/rsa.c index 145df5e..b30fa01 100644 --- a/rsa.c +++ b/rsa.c @@ -128,23 +128,22 @@ struct rsapub { static const char *hexchars="0123456789abcdef"; -static void rsa_sethash(struct rsacommon *c, struct hash_if *hash, +static void rsa_sethash(struct load_ctx *l, + struct rsacommon *c, const struct hash_if **in_ops) { - free(c->hashbuf); + struct hash_if *hash=0; + if (l->deprdict) + hash=find_cl_if(l->deprdict,"hash",CL_HASH,False,"site",l->loc); + if (!hash) + hash=sha1_hash_if; c->hashbuf=safe_malloc(hash->hlen, "generate_msg"); *in_ops=hash; } -static void rsa_pub_sethash(void *sst, struct hash_if *hash) -{ - struct rsapub *st=sst; - rsa_sethash(&st->common, hash, &st->ops.hash); -} -static void rsa_priv_sethash(void *sst, struct hash_if *hash) -{ - struct rsapriv *st=sst; - rsa_sethash(&st->common, hash, &st->ops.hash); -} + +static void rsa_pub_sethash(void *sst, struct hash_if *hash) { } +static void rsa_priv_sethash(void *sst, struct hash_if *hash) { } + static void rsacommon_dispose(struct rsacommon *c) { free(c->hashbuf); @@ -365,6 +364,8 @@ static struct rsapub *rsa_loadpub_core(RSAPUB_BNS(RSAPUB_LOADCORE_DEFBN) RSAPUB_BNS(RSAPUB_LOADCORE_GETBN) + rsa_sethash(l,&st->common,&st->ops.hash); + return st; error_out: @@ -635,6 +636,8 @@ static struct rsapriv *rsa_loadpriv_core(struct load_ctx *l, fatal_perror("rsa-private (%s:%d): ferror",loc.file,loc.line); } + rsa_sethash(l,&st->common,&st->ops.hash); + /* * Now verify the validity of the key, and set up the auxiliary * values for fast CRT signing. -- 2.30.2