[Openvpn-devel] [L] Change in openvpn[master]: oob: Add control message TLV encoding (P_CONTROL_OOB_V1)

Attention is currently required from: flichtenheld, plaisthos.

stipa has posted comments on this change by stipa. ( http://gerrit.openvpn.net/c/openvpn/+/1741?usp=email )

Change subject: oob: Add control message TLV encoding (P_CONTROL_OOB_V1)
......................................................................


Patch Set 9:

(9 comments)

Patchset:

PS8: 
> The spec I wrote cover both out of band and also inband MTU check packets that have the same format. […]
I will move the code which touches both OOB and future inband into control_msg.c/h and leave only OOB-specific code in oob.c/oob_client.c


File CMakeLists.txt:

http://gerrit.openvpn.net/c/openvpn/+/1741/comment/c99a69f9_76cadbc8?usp=email :
PS8, Line 868:         )
> Do we really need an extra unit test for this? Can we instead just add this as extra compilation fil […]
I would keep it separate. There are already 19 test drivers here, many for smaller modules, so per module seemed to be the pattern. Also test_pkt links the crypto backends, tls_crypt and reliable, while oob_testdriver only needs a few files, so I would not link crypto stack to it.


File src/openvpn/oob.c:

http://gerrit.openvpn.net/c/openvpn/+/1741/comment/e5bb0109_e5122bc3?usp=email :
PS8, Line 32:  * Consume @p value_len bytes of TLV value, of which @p consumed have already
> What is @p? That is not used anywhere else.
We do use it together with param name in doxygen in other files (init.h for example), but somewhat inconsistently. Let me remove it and use only @param and bare parameter names.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/b28e0c8b_70ef40c7?usp=email :
PS8, Line 37:  * @param value_len  the TLV's declared value length
> declared as in the length in the header or declared as in the spec or as in expected.
Will replace with "the value length from the TLV header"


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/6c0b4a64_7b69b0a2?usp=email :
PS8, Line 38:  * @param consumed   number of value bytes already read from @p buf
> We normally do this by buf_advance instead of this way.
Yep, will call buf_advance() directly.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/b4561618_b5c30c7a?usp=email :
PS8, Line 52: bool
> Why just have a method that wraps buf_write_u16? That seems uncessary.
agreed, will remove the wrapper and call buf_write_u16 directly.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/1537b583_56bd7781?usp=email :
PS8, Line 91:     *value_len = (uint16_t)len;
> I feel instead of passing the same values with pointers again and again we should instead use proper […]
Yep, will add struct oob_tlv_header struct.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/9db80515_ad07ed38?usp=email :
PS8, Line 168: }
> The formatting of this method is not really great.
yep agreed, will fix.


File src/openvpn/ssl_pkt.h:

http://gerrit.openvpn.net/c/openvpn/+/1741/comment/23c27391_45a5ff38?usp=email :
PS8, Line 70: #define P_CONTROL_OOB_V1 12
> That feels like a very hacky way of doing this and also will already create problem with the spec th […]
The root cause is that P_LAST_OPCODE has 2 different meanings:

 - could this opcode appear on an established session
 - highest opcode that exists

At the moment the last opcode which could appear on a session is 11 and 12/13 are
OOB. When 14 (inband) appears, this will indeed break.

Let me change it in the following way:

 - P_LAST_OPCODE will be the highest opcode that exists (in/outband)
 - new static inlines to gate session-valid opcodes:

static inline bool
opcode_is_oob(int op)
{
    return op == P_CONTROL_OOB_V1 || op == P_CONTROL_OOB_WKC_V1;
}

/* true if op may occur on an established control-channel session */
static inline bool
opcode_valid_in_session(int op)
{
    return op >= P_FIRST_OPCODE && op <= P_LAST_OPCODE && !opcode_is_oob(op);
}

What harm in answering OOB packets inside the session?

No real harm today - a probe on a live session is dropped anyway, but
incidentally: OOB carries no ACK fields, so the session path reads SERVER_PROBE's
0x0100 as an ACK count of 1 and then TLV bytes as a packet-id and session id,
which mismatches. The payload is attacker-chosen and session ids are plaintext,
so it can be crafted into a valid ACK array - forged ACKs stall a handshake or
rekey.

Answering in-session: yes, but as a follow-up. Today's behaviour is worse than
rejected - the stateless path only runs when the address has no instance, so a
connected peer's probe is dropped, not answered (rare in practice: probe sockets
are unbound, so reconnects arrive on a fresh port). It needs dispatch before
reliable_ack_read(), a per-instance rate limit etc. I'd do it with
CONTROL_DATA_V1.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1741?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c8d302ac57c5603d622a7be14be369437388268
Gerrit-Change-Number: 1741
Gerrit-PatchSet: 9
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-Comment-Date: Tue, 28 Jul 2026 15:04:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel