Index | Thread | Search

From:
Theo Buehler <tb@theobuehler.org>
Subject:
Re: isakmpd sha2-{384,512} block size
To:
YASUOKA Masahiko <yasuoka@openbsd.org>
Cc:
tech@openbsd.org
Date:
Wed, 20 Nov 2024 11:40:42 +0100

Download raw body.

Thread
On Wed, Nov 20, 2024 at 06:20:52PM +0900, YASUOKA Masahiko wrote:
> Anybody?
> 
> This will solve actual problems, connecting from Android for an example.

Your diff reads correct to me, so ok.

This is independent of your diff, but I am not sure the approach via
function pointer casts assigning to init(), update(), final() and then
calling those directly is kosher. I think that void * and MD5_CTX * are
not compatible types, so I believe calling MD5Init, etc via a function
pointer of type void (*)(void *), etc is undefined behavior. This would
probably need another layer of indirection to be safe.

> 
> On Wed, 21 Aug 2024 14:07:57 +0900 (JST)
> YASUOKA Masahiko <yasuoka@openbsd.org> wrote:
> > On Tue, 20 Aug 2024 09:42:48 +0900 (JST)
> > YASUOKA Masahiko <yasuoka@openbsd.org> wrote:
> >> I noticed isakmpd can't decrypt quick mode messages when the phase-1
> >> is established with sha2-384 or sha2-512.  In hash.c, hmac_init() is
> >> using 64 for the block length, but the block length of sha2-384 or
> >> sha2-512 should be 128.
> > 
> > Previous used the fixed local value 64 for the block length.  It isn't
> > correct for SHA2-384 or SHA2-512.  The diff changes to use the block
> > sizes from the header file from the crypto library.
> > 
> > Problems actually happen if we use HMAC-SHA2-384 or -512 for
> > L2TP/IPsec.  Below error messages are written in the /var/log/daemon
> > when the error happens.
> > 
> >   133854.054627 Default message_parse_payloads: reserved field non-zero: d6    
> >   133854.054878 Default dropped message from 100.64.1.3 port 4500 due to notification type PAYLOAD_MALFORMED
> > 
> > I verified the diff actually fixes this problem.
> > 
> > Let me share my test procedure with a strongswan and its result:
> > 
> > # strongswan:/etc/ipsec.conf
> > conn vmm-test
> >     keyexchange=ikev1
> >     authby=secret
> >     forceencaps=yes
> >     leftfirewall=yes
> >     right=100.64.2.3
> >     leftprotoport=17/1701
> >     rightprotoport=17/1701
> >     ike=aes256-sha2_512-modp2048!
> >     esp=aes256-sha2_512
> >     auto=add
> >     type=transport
> > 
> > # strongswan:/etc/ipsec.conf
> > 100.64.2.3 : PSK "SECRET"
> > 
> > # openbsd:/etc/ipsec.conf
> > ike passive esp transport \
> >     proto udp from { 100.64.2.3 } port 1701 to any \
> >     main auth "hmac-sha2-512" enc "aes" group "modp2048" \
> >     quick auth "hmac-sha2" enc "aes" \
> >     psk "SECRET"
> > 
> > Before the patch:
> > 
> >   133854.054627 Default message_parse_payloads: reserved field non-zero: d6    
> >   133854.054878 Default dropped message from 100.64.1.3 port 4500 due to notification type PAYLOAD_MALFORMED
> > 
> > (IPsec SA is not established)
> > 
> > After the patch:
> > 
> >   134355.816327 Default isakmpd: phase 1 done (as responder): initiator id 100.64.1.3, responder id 100.64.2.3, src: 100.64.2.3 dst: 100.64.1.3 
> >   134355.919719 Default isakmpd: quick mode done (as responder): src: 100.64.2.3 dst: 100.64.1.3
> > 
> > (IPsec SA is established)
> > 
> >> ok?
> >> 
> >> Index: sbin/isakmpd/hash.c
> >> ===================================================================
> >> RCS file: /cvs/src/sbin/isakmpd/hash.c,v
> >> diff -u -p -r1.24 hash.c
> >> --- sbin/isakmpd/hash.c	15 Oct 2015 06:35:54 -0000	1.24
> >> +++ sbin/isakmpd/hash.c	20 Jul 2024 18:21:40 -0000
> >> @@ -56,7 +56,7 @@ static unsigned char digest[HASH_MAX];
> >>  
> >>  static struct hash hashes[] = {
> >>      {
> >> -	HASH_MD5, 5, MD5_SIZE, (void *)&Ctx.md5ctx, digest,
> >> +	HASH_MD5, 5, MD5_SIZE, MD5_BLOCK_LENGTH, (void *)&Ctx.md5ctx, digest,
> >>  	sizeof(MD5_CTX), (void *)&Ctx2.md5ctx,
> >>  	(void (*)(void *))MD5Init,
> >>  	(void (*)(void *, unsigned char *, unsigned int))MD5Update,
> >> @@ -64,32 +64,32 @@ static struct hash hashes[] = {
> >>  	hmac_init,
> >>  	hmac_final
> >>      }, {
> >> -	HASH_SHA1, 6, SHA1_SIZE, (void *)&Ctx.sha1ctx, digest,
> >> -	sizeof(SHA1_CTX), (void *)&Ctx2.sha1ctx,
> >> +	HASH_SHA1, 6, SHA1_SIZE, SHA1_BLOCK_LENGTH, (void *)&Ctx.sha1ctx,
> >> +	digest, sizeof(SHA1_CTX), (void *)&Ctx2.sha1ctx,
> >>  	(void (*)(void *))SHA1Init,
> >>  	(void (*)(void *, unsigned char *, unsigned int))SHA1Update,
> >>  	(void (*)(unsigned char *, void *))SHA1Final,
> >>  	hmac_init,
> >>  	hmac_final
> >>      }, {
> >> -	HASH_SHA2_256, 7, SHA2_256_SIZE, (void *)&Ctx.sha2ctx, digest,
> >> -	sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx,
> >> +	HASH_SHA2_256, 7, SHA2_256_SIZE, SHA256_BLOCK_LENGTH,
> >> +	(void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx,
> >>  	(void (*)(void *))SHA256Init,
> >>  	(void (*)(void *, unsigned char *, unsigned int))SHA256Update,
> >>  	(void (*)(u_int8_t *, void *))SHA256Final,
> >>  	hmac_init,
> >>  	hmac_final
> >>      }, {
> >> -	HASH_SHA2_384, 8, SHA2_384_SIZE, (void *)&Ctx.sha2ctx, digest,
> >> -	sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx,
> >> +	HASH_SHA2_384, 8, SHA2_384_SIZE, SHA384_BLOCK_LENGTH,
> >> +	(void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx,
> >>  	(void (*)(void *))SHA384Init,
> >>  	(void (*)(void *, unsigned char *, unsigned int))SHA384Update,
> >>  	(void (*)(u_int8_t *, void *))SHA384Final,
> >>  	hmac_init,
> >>  	hmac_final
> >>      }, {
> >> -	HASH_SHA2_512, 9, SHA2_512_SIZE, (void *)&Ctx.sha2ctx, digest,
> >> -	sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx,
> >> +	HASH_SHA2_512, 9, SHA2_512_SIZE, SHA512_BLOCK_LENGTH,
> >> +	(void *)&Ctx.sha2ctx, digest, sizeof(SHA2_CTX), (void *)&Ctx2.sha2ctx,
> >>  	(void (*)(void *))SHA512Init,
> >>  	(void (*)(void *, unsigned char *, unsigned int))SHA512Update,
> >>  	(void (*)(u_int8_t *, void *))SHA512Final,
> >> @@ -122,11 +122,11 @@ hash_get(enum hashes hashtype)
> >>  void
> >>  hmac_init(struct hash *hash, unsigned char *okey, unsigned int len)
> >>  {
> >> -	unsigned int    i, blocklen = HMAC_BLOCKLEN;
> >> -	unsigned char   key[HMAC_BLOCKLEN];
> >> +	unsigned int    i;
> >> +	unsigned char   key[128];
> >>  
> >> -	bzero(key, blocklen);
> >> -	if (len > blocklen) {
> >> +	bzero(key, sizeof(key));
> >> +	if (len > hash->blocklen) {
> >>  		/* Truncate key down to blocklen */
> >>  		hash->Init(hash->ctx);
> >>  		hash->Update(hash->ctx, okey, len);
> >> @@ -136,19 +136,19 @@ hmac_init(struct hash *hash, unsigned ch
> >>  	}
> >>  
> >>  	/* HMAC I and O pad computation */
> >> -	for (i = 0; i < blocklen; i++)
> >> +	for (i = 0; i < hash->blocklen; i++)
> >>  		key[i] ^= HMAC_IPAD_VAL;
> >>  
> >>  	hash->Init(hash->ctx);
> >> -	hash->Update(hash->ctx, key, blocklen);
> >> +	hash->Update(hash->ctx, key, hash->blocklen);
> >>  
> >> -	for (i = 0; i < blocklen; i++)
> >> +	for (i = 0; i < hash->blocklen; i++)
> >>  		key[i] ^= (HMAC_IPAD_VAL ^ HMAC_OPAD_VAL);
> >>  
> >>  	hash->Init(hash->ctx2);
> >> -	hash->Update(hash->ctx2, key, blocklen);
> >> +	hash->Update(hash->ctx2, key, hash->blocklen);
> >>  
> >> -	explicit_bzero(key, blocklen);
> >> +	explicit_bzero(key, sizeof(key));
> >>  }
> >>  
> >>  /*
> >> Index: sbin/isakmpd/hash.h
> >> ===================================================================
> >> RCS file: /cvs/src/sbin/isakmpd/hash.h,v
> >> diff -u -p -r1.8 hash.h
> >> --- sbin/isakmpd/hash.h	10 Jun 2006 20:10:02 -0000	1.8
> >> +++ sbin/isakmpd/hash.h	20 Jul 2024 18:21:40 -0000
> >> @@ -53,6 +53,7 @@ struct hash {
> >>  	enum hashes     type;
> >>  	int             id;	/* ISAKMP/Oakley ID */
> >>  	u_int8_t        hashsize;	/* Size of the hash */
> >> +	unsigned	blocklen;	/* The hash's block length */
> >>  	void           *ctx;	/* Pointer to a context, for HMAC ictx */
> >>  	unsigned char  *digest;	/* Pointer to a digest */
> >>  	int             ctxsize;
> >> @@ -68,7 +69,6 @@ struct hash {
> >>  
> >>  #define HMAC_IPAD_VAL	0x36
> >>  #define HMAC_OPAD_VAL	0x5C
> >> -#define HMAC_BLOCKLEN	64
> >>  
> >>  extern struct hash *hash_get(enum hashes);
> >>  extern void     hmac_init(struct hash *, unsigned char *, unsigned int);
> >> 
> > 
> > 
>