On Sat, Feb 25, 2017 at 04:15:07PM +0100, Denis Fondras wrote:
> Hi,
>
> A patch to get away from SHA1 in dhcpd
>
HMAC-SHA1 is not affected by the published collision, but I'm not
against switching the sync protocol to SHA2. Performance also doesn't
matter that much here as the typical sync rate is fairly small.
Once done, it should also be done for spamd-sync where the protocol came from.
See comments below.
>
> Index: sync.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/dhcpd/sync.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 sync.c
> --- sync.c 13 Feb 2017 23:04:05 -0000 1.23
> +++ sync.c 25 Feb 2017 15:12:52 -0000
> @@ -32,7 +32,7 @@
>
> #include <errno.h>
> #include <netdb.h>
> -#include <sha1.h>
> +#include <sha2.h>
> #include <string.h>
> #include <syslog.h>
> #include <unistd.h>
> @@ -140,7 +140,7 @@ sync_init(const char *iface, const char
> }
> }
>
> - sync_key = SHA1File(DHCP_SYNC_KEY, NULL);
> + sync_key = SHA256File(DHCP_SYNC_KEY, NULL);
> if (sync_key == NULL) {
> if (errno != ENOENT) {
> log_warn("failed to open sync key");
> @@ -270,7 +270,7 @@ sync_recv(void)
> /* Compute and validate HMAC */
> memcpy(hmac[0], hdr->sh_hmac, DHCP_SYNC_HMAC_LEN);
> explicit_bzero(hdr->sh_hmac, DHCP_SYNC_HMAC_LEN);
> - HMAC(EVP_sha1(), sync_key, strlen(sync_key), buf, len,
> + HMAC(EVP_sha256(), sync_key, strlen(sync_key), buf, len,
> hmac[1], &hmac_len);
> if (bcmp(hmac[0], hmac[1], DHCP_SYNC_HMAC_LEN) != 0)
> goto trunc;
> @@ -404,7 +404,7 @@ sync_lease(struct lease *lease)
> memset(&pad, 0, sizeof(pad));
>
> HMAC_CTX_init(&ctx);
> - HMAC_Init(&ctx, sync_key, strlen(sync_key), EVP_sha1());
> + HMAC_Init(&ctx, sync_key, strlen(sync_key), EVP_sha256());
>
> leaselen = sizeof(lv);
> padlen = DHCP_ALIGN(leaselen) - leaselen;
> Index: sync.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/dhcpd/sync.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 sync.h
> --- sync.h 4 Oct 2016 22:47:51 -0000 1.5
> +++ sync.h 25 Feb 2017 15:12:52 -0000
> @@ -20,6 +20,8 @@
> #ifndef _DHCPD_SYNC
> #define _DHCPD_SYNC
>
> +#include <sha2.h>
> +
> /*
> * dhcpd(8) synchronisation protocol.
> *
> @@ -28,14 +30,14 @@
> * It is a simple Type-Length-Value based protocol, it allows easy
> * extension with future subtypes and bulk transfers by sending multiple
> * entries at once. The unencrypted messages will be authenticated using
> - * HMAC-SHA1.
> + * HMAC-SHA256.
> *
> */
>
> #define DHCP_SYNC_VERSION 1
This should be bumped to version 2
> #define DHCP_SYNC_MCASTADDR "224.0.1.240" /* XXX choose valid address */
> #define DHCP_SYNC_MCASTTTL IP_DEFAULT_MULTICAST_TTL
> -#define DHCP_SYNC_HMAC_LEN 20 /* SHA1 */
> +#define DHCP_SYNC_HMAC_LEN SHA256_DIGEST_LENGTH
> #define DHCP_SYNC_MAXSIZE 1408
> #define DHCP_SYNC_KEY "/var/db/dhcpd.key"
>
You should also look at the struct below and adjust the padding to
keep it aligned to 8 bytes:
```
struct dhcp_synchdr {
u_int8_t sh_version;
u_int8_t sh_af;
u_int16_t sh_length;
u_int32_t sh_counter;
u_int8_t sh_hmac[DHCP_SYNC_HMAC_LEN];
u_int8_t sh_pad[4];
} __packed;
```
Before: 1+1+2+4+20+4 = 32
After: 1+2+2+4+32+0 = 40
```
struct dhcp_synchdr {
u_int8_t sh_version;
u_int8_t sh_af;
u_int16_t sh_length;
u_int32_t sh_counter;
u_int8_t sh_hmac[DHCP_SYNC_HMAC_LEN];
} __packed;
```
So we increase the HMAC_LEN by 12 bytes but removing the now unneeded
padding reduces the overhead to a total of 8 bytes. I think that's
worth it.
Reyk
