From: Rafał Ramocki Subject: Re: Sasyncd is unable to synchronize states at startup To: Tobias Heider Cc: tech Date: Thu, 17 Apr 2025 16:43:38 +0200 I'm attaching patch with least invasive and smallest possible change that solves the problem of "permission denied". ----- Original Message ----- From: "Rafał Ramocki" To: "Tobias Heider" Cc: "tech" Sent: Thursday, April 17, 2025 4:18:56 PM Subject: Re: Sasyncd is unable to synchronize states at startup Hello Tobias, I'm sorry for refreshing quite old topic. I see that this change was applied and is publicly available and it works - states aresynchronized at startup. But there is one additional problem I've missed last time or maybe some other changes popped up in the mean time. During sasyncd start, when states are synchronized there are some permission denied errors. I've investigated it and figured out that permission denied is returned by the kernel. In pfkeyv2.c in line 2111 there is comparission of message type with list of allowed extensions in sadb_exts_allowed_out array: if ((seen & sadb_exts_allowed_out[smsg->sadb_msg_type]) != seen) { rval = EPERM; goto realret; } I'm not sure why it is "out" not "in" since this is part of function pfkeyv2_dosend that is described as "Handle all messages from userland to kernel". Maybe should be sadb_exts_allowed_in not sadb_exts_allowed_out? But I'm not sure if I correctly understand the logic. If *_out is correct thought, so I think we should add extensions that are not allowed now but synchronized by sasyncd and those are: SADB_X_EXT_LIFETIME_LASTUSE SADB_X_EXT_COUNTER SADB_X_EXT_REPLAY And I've tested that. Adding this is solving problem. But as I mentioned I'm not sure if this is correct solution. Additionaly just after that there is another comparission with sadb_exts_required_out this time: if ((seen & sadb_exts_required_out[smsg->sadb_msg_type]) != sadb_exts_required_out[smsg->sadb_msg_type]) { rval = EPERM; goto realret; } since sadb_exts_required_out seams to be a subset of sadb_exts_allowed_out it looks redundant. I would like to prepare patches this problem but I think I need some guidence parts of code I'm wondering at are 23 years old. Can you help me? ----- Original Message ----- From: "Tobias Heider" To: "Rafał Ramocki" Cc: "tech" Sent: Thursday, March 7, 2024 3:49:14 PM Subject: Re: Sasyncd is unable to synchronize states at startup 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)) { Index: pfkeyv2_parsemessage.c =================================================================== RCS file: /cvs/src/sys/net/pfkeyv2_parsemessage.c,v diff -u -p -r1.63 pfkeyv2_parsemessage.c --- pfkeyv2_parsemessage.c 23 Jul 2024 20:04:51 -0000 1.63 +++ pfkeyv2_parsemessage.c 17 Apr 2025 14:38:51 -0000 @@ -218,7 +218,7 @@ const uint64_t sadb_exts_allowed_out[SAD /* UPDATE */ BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_ADDRESS_PROXY | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN | BITMAP_X_IFACE, /* ADD */ - BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN | BITMAP_X_IFACE, + BITMAP_SA | BITMAP_LIFETIME | BITMAP_ADDRESS | BITMAP_IDENTITY | BITMAP_X_FLOW | BITMAP_X_UDPENCAP | BITMAP_X_LIFETIME_LASTUSE | BITMAP_X_TAG | BITMAP_X_TAP | BITMAP_X_RDOMAIN | BITMAP_X_COUNTER | BITMAP_X_REPLAY | BITMAP_X_IFACE, /* DELETE */ BITMAP_SA | BITMAP_ADDRESS_SRC | BITMAP_ADDRESS_DST | BITMAP_X_RDOMAIN, /* GET */