Re: From SHA1 to SHA256 in dhcpd sync
> > On 27.02.2017, at 16:10, Theo de Raadt wrote: > > > >>> > >>> 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. > > > > Well, I don't see the point of making the change. HMAC's are still safe. > > True, I don't mind either way. > > So let's keep "version 1" as it is. Denis, you should keep your change lying around and keep an eye on these files. When an extension for these protocols is proposed or commited, thereby creating an incompatibility, then bring the diffs forward.
Re: From SHA1 to SHA256 in dhcpd sync
> On 27.02.2017, at 16:10, Theo de Raadt wrote: > >>> >>> 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. > > Well, I don't see the point of making the change. HMAC's are still safe. True, I don't mind either way. So let's keep "version 1" as it is. Reyk
Re: From SHA1 to SHA256 in dhcpd sync
> > 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. Well, I don't see the point of making the change. HMAC's are still safe.
Re: From SHA1 to SHA256 in dhcpd sync
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.c13 Feb 2017 23:04:05 - 1.23
> +++ sync.c25 Feb 2017 15:12:52 -
> @@ -32,7 +32,7 @@
>
> #include
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -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.h4 Oct 2016 22:47:51 - 1.5
> +++ sync.h25 Feb 2017 15:12:52 -
> @@ -20,6 +20,8 @@
> #ifndef _DHCPD_SYNC
> #define _DHCPD_SYNC
>
> +#include
> +
> /*
> * 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_VERSION1
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_MAXSIZE1408
> #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_tsh_version;
u_int8_tsh_af;
u_int16_t sh_length;
u_int32_t sh_counter;
u_int8_tsh_hmac[DHCP_SYNC_HMAC_LEN];
u_int8_tsh_pad[4];
} __packed;
```
Before: 1+1+2+4+20+4 = 32
After: 1+2+2+4+32+0 = 40
```
struct dhcp_synchdr {
u_int8_tsh_version;
u_int8_tsh_af;
u_int16_t sh_length;
u_int32_t sh_counter;
u_int8_tsh_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
Re: From SHA1 to SHA256 in dhcpd sync
> It does also need some notice to users that old+new aren't compatible. > But as far as I'm aware SHA1 and even MD5 are still considered suitable > for HMAC aren't they? > You are right Stuart.
Re: From SHA1 to SHA256 in dhcpd sync
On 2017-02-25, Denis Fondras wrote: > Hi, > > A patch to get away from SHA1 in dhcpd It does also need some notice to users that old+new aren't compatible. But as far as I'm aware SHA1 and even MD5 are still considered suitable for HMAC aren't they?
From SHA1 to SHA256 in dhcpd sync
Hi,
A patch to get away from SHA1 in dhcpd
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 - 1.23
+++ sync.c 25 Feb 2017 15:12:52 -
@@ -32,7 +32,7 @@
#include
#include
-#include
+#include
#include
#include
#include
@@ -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 - 1.5
+++ sync.h 25 Feb 2017 15:12:52 -
@@ -20,6 +20,8 @@
#ifndef _DHCPD_SYNC
#define _DHCPD_SYNC
+#include
+
/*
* 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
#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"
