On Mon, Feb 26, 2024 at 02:15:39PM +0100, Rafał Ramocki wrote:
> Hello, 
> 
> I've found that after restart of standby node of OpenBSD firewall cluster after sasyncd startup flows are synchronized from master but SAD's are not. I've investigated problem and found that this diff breaks this function: 
> 
> [ https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/pfkeyv2.h.diff?r1=1.89&r2=1.90&f=h | https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/pfkeyv2.h.diff?r1=1.89&r2=1.90&f=h ] 
> 
> And probably there is other patch that make this function not work properly. The reason is because during synchronization of states sasyncd aquiers states, encrypts almost-raw kernel message, send it to other sasyncd daemon where this message is decrypted and sent to kernel to set new states. Fortunately kernel in function pfkeyv2_parsemessage() performs sanity checks of received from sasyncd message and finds out that it knows nothing about SADB_X_EXT_REPLAY extension end returns EINVAL and state is not created. I've added for tests part of code to allow this extension to pass sanity checks and then I've hit another extension that it is not allowed on message which is SADB_X_EXT_COUNTER. After adding this to parser function new states are created as they should be but i suppose that in some cases SADB_X_EXT_MTU extension also may be set and if so sasyncd silently will not create states either. 
> 
> please find out patch in attachment. 
> 
> How I discovered found error and solution? Following message that should create state is not valid according to the kernel: 
> 
> net_read: post decrypt 
> 0203000243000000000000000000000002000100cdc34e701001050c0402000004000200 
> 000000000000000000000000d2e3cd650000000000000000000000000400040000000000 
> 0000000000000000a80c0000000000000000000000000000040003000000000000000000 
> 00000000100e0000000000000000000000000000030005000000000010020000d45b18a6 
> 00000000000000000300060000000000100200000d3012d5000000000000000005000a00 
> 0100000000000000000000003231322e39312e32342e3136362f33320000000000000000 
> 04000b0001000000000000000000000031332e34382e31382e3231332f33320005000800 
> 000100005374b0606dc43282d401cf26ad15693c1336852981a56cd6c57c0098ef22dc51 
> 05000900000100002883b249a6964a07aa7f86571e70eb495e200fb692cff1434a78c49a 
> 05b14d38010014000102000001001300000000000300150000000000100200000a020f00 
> 0000000000000000030011000000000010020000ffffff00000000000000000003001600 
> 0000000010020000ac1f00000000000000000000030012000000000010020000fffff000 
> 000000000000000001001f00119400000200270000000000010000000000000009002400 
> 000000000000000000000000000000000000000000000000000000000000000000000000 
> 000000000000000000000000000000000000000000000000000000000000000008070605 
> 04030201 
> net_read: computed hash 
> 1ee903f3fa270d5e55dafddf0f251e8230d080cc 
> net_handle_messages: got msg type 1 len 536 from peer 172.31.255.245 
> pfkey_queue_message: pfkey ADD len 536 seq 12 
> sasyncd: pfkey: msg ADD write() failed on socket 4: Invalid argument 
> net_read: got hash 
> 
> So I've enabled "encdebug", recompiled kernel, restarted it found following message related to this "Invalid argument" message: 
> 
> /bsd: pfkeyv2_parsemessage: extension header 39 not permitted on message type 3 
> 
> When I've added handling of SADB_X_EXT_REPLAY to the parser function and restarted the kernel states was still not created but this time following error was recorded: 
> 
> /bsd: pfkeyv2_parsemessage: extension header 36 not permitted on message type 3 
> 
> So I've added similar change for extension SADB_X_EXT_COUNTER too. I additionally see that in pfkeyv2.h in my version of OpenBSD (7.3) there is also SADB_X_EXT_MTU extension that will cause message parser fail if it will appear. Additionally in current there is also SADB_X_EXT_IFACE extension that that pfkeyv2_parsemessage is also unaware. 
> 
> -- 
> Rafal Ramocki 

Thanks Rafal, the patch looks correct to me and seems to be relatively
low risk, but I have no way to test it or reproduce the issue.

Anyone using or caring about sasync who can confirm the issue and provide
test feedback?

> 
> Index: net/pfkeyv2_parsemessage.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pfkeyv2_parsemessage.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 pfkeyv2_parsemessage.c
> --- net/pfkeyv2_parsemessage.c	14 Jul 2021 22:39:26 -0000	1.60
> +++ net/pfkeyv2_parsemessage.c	16 Feb 2024 16:32:31 -0000
> @@ -143,9 +143,9 @@ uint64_t sadb_exts_allowed_in[SADB_MAX+1
>  	/* GETSPI */
>  	BITMAP_ADDRESS_SRC | BITMAP_ADDRESS_DST | BITMAP_SPIRANGE,
>  	/* UPDATE */
> -	BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_ADDRESS_PROXY | BITMAP_KEY | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN,
> +	BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_ADDRESS_PROXY | BITMAP_KEY | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN | BITMAP_X_REPLAY | BITMAP_X_COUNTER,
>  	/* ADD */
> -	BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_KEY | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_LIFETIME_LASTUSE | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN,
> +	BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_KEY | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_LIFETIME_LASTUSE | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN | BITMAP_X_REPLAY | BITMAP_X_COUNTER,
>  	/* DELETE */
>  	BITMAP_SA | BITMAP_ADDRESS_SRC | BITMAP_ADDRESS_DST | BITMAP_X_RDOMAIN,
>  	/* GET */
> @@ -858,6 +858,19 @@ pfkeyv2_parsemessage(void *p, int len, v
>  				return (EINVAL);
>  			}
>  			break;
> +		case SADB_X_EXT_REPLAY:
> +			if (i != sizeof(struct sadb_x_replay)) {
> +				DPRINTF("bad REPLAY header length");
> +				return (EINVAL);
> +			}
> +			break;
> +		case SADB_X_EXT_COUNTER:
> +			if (i != sizeof(struct sadb_x_counter)) {
> +				DPRINTF("bad COUNTER header length");
> +				return (EINVAL);
> +			}
> +			break;
> +
>  #if NPF > 0
>  		case SADB_X_EXT_TAG:
>  			if (i < sizeof(struct sadb_x_tag)) {