[Openvpn-devel] [S] Change in openvpn[master]: ssl: Do not queue control ciphertext while a packet is still queued
"flichtenheld \(Code Review\) via Openvpn-devel" <[email protected]> Thu, 30 Jul 2026 09:03:45 +0000
| Newsgroups | net.sourceforge.lists.openvpn-devel |
|---|---|
| Message-ID | <[email protected]> |
--===============1083160401716115028==
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
Content-Type: multipart/alternative; boundary="2uRhKbVrb+w="; charset=UTF-8
--2uRhKbVrb+w=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Attention is currently required from: plaisthos=2E
Hello plaisthos,
I'd l=
ike you to do a code review=2E
Please visit
http://gerrit=2Eopenvpn=2E=
net/c/openvpn/+/1831?usp=3Demail
to review the following change=2E
Chang=
e subject: ssl: Do not queue control ciphertext while a packet is still que=
ued
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E
ssl: Do=
not queue control ciphertext while a packet is still queued
An outgoing c=
ontrol channel packet is handed to the link layer as a
buffer descriptor po=
inting into the reliable send buffer it was built
from, and the packet id s=
its in that buffer's headroom, right in front
of the payload=2E If the ent=
ry is reused before the packet has been
written out, buf_copy_n() writes th=
e new payload and
reliable_mark_active_outgoing() prepends the new packet i=
d exactly over
the packet id of the queued packet, while its opcode, ACK ar=
ray and
length stay untouched=2E The queued packet then goes out with some=
body
else's packet id=2E
Observed in a TCP p2p handshake: both peers reset=
simultaneously, the
peer's two HARD_RESET packets arrive back to back, so =
io_wait_dowork()
takes the residual data shortcut (event_set_status =3D SOC=
KET_READ) and
does not write out our already queued HARD_RESET retransmit=
=2E The ACK
in the second peer reset then purges our reset from the send w=
indow,
tls_process_state() moves to S_START and queues the ClientHello into=
the very same (now inactive) entry=2E Result on the wire: a HARD_RESET
wi=
th the ClientHello's packet id 1, followed by the ClientHello with
the same=
id 1=2E The receiver consumes the reset, advances its receive
window, and=
drops the real ClientHello as a replay - the handshake
deadlocks until it =
times out=2E
The send path in tls_process_state() and the dedicated ACK pa=
th in
tls_process() are already guarded by to_link->len, only the ciphertex=
t
queueing was not=2E Guard it as well=2E A pending to_link makes
tls_pro=
cess() report itself as active, so we are called again as soon
as the packe=
t has been written out=2E In the error path this can drop a
TLS alert that=
we would have queued, which is in line with that path
not ensuring deliver=
y anyway=2E
Change-Id: Ib0e7c9d3a2f4e6b8c1d5a9f7e3b2c4d6a8f1e5b3
Signed-of=
f-by: Frank Lichtenheld <frank@lichtenheld=2Ecom>
---
M src/openvpn/ssl=2Ec=
1 file changed, 10 insertions(+), 3 deletions(-)
git pull ssh://gerri=
t=2Eopenvpn=2Enet:29418/openvpn refs/changes/31/1831/1
diff --git a/src/op=
envpn/ssl=2Ec b/src/openvpn/ssl=2Ec
index 60df7ce=2E=2E7f2e850 100644
--- a=
/src/openvpn/ssl=2Ec
+++ b/src/openvpn/ssl=2Ec
@@ -2714,8 +2714,15 @@
st=
atic bool
check_outgoing_ciphertext(struct key_state *ks, struct tls_sessi=
on *session,
- bool *continue_tls_process)
+ =
struct buffer *to_link, bool *continue_tls_process)
{
=
+ if (to_link->len)
+ {
+ dmsg(D_TLS_DEBUG,
+ "Def=
erring outgoing ciphertext, previous packet not written out yet");
+ =
return true;
+ }
+
/* Outgoing Ciphertext to reliable buffer */
=
if (ks->state >=3D S_START)
{
@@ -2895,7 +2902,7 @@
dm=
sg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
}
}
- if (!c=
heck_outgoing_ciphertext(ks, session, &continue_tls_process))
+ if (!che=
ck_outgoing_ciphertext(ks, session, to_link, &continue_tls_process))
{=
goto error;
}
@@ -2907,7 +2914,7 @@
/* Shut down the TL=
S session but do a last read from the TLS
* object to be able to read=
potential TLS alerts */
key_state_ssl_shutdown(&ks->ks_ssl);
- che=
ck_outgoing_ciphertext(ks, session, &continue_tls_process);
+ check_outg=
oing_ciphertext(ks, session, to_link, &continue_tls_process);
/* Put=
ourselves in the pre error state that will only send out the
* contr=
ol channel packets but nothing else */
--
To view, visit http://gerrit=2E=
openvpn=2Enet/c/openvpn/+/1831?usp=3Demail
To unsubscribe, or for help writ=
ing mail filters, visit http://gerrit=2Eopenvpn=2Enet/settings?usp=3Demail
=
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: maste=
r
Gerrit-Change-Id: Ib0e7c9d3a2f4e6b8c1d5a9f7e3b2c4d6a8f1e5b3
Gerrit-Change=
-Number: 1831
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <frank@lichtenh=
eld=2Ecom>
Gerrit-Reviewer: plaisthos <arne-openvpn@rfc2549=2Eorg>
Gerrit-C=
C: openvpn-devel <openvpn-devel@lists=2Esourceforge=2Enet>
Gerrit-Attention=
: plaisthos <arne-openvpn@rfc2549=2Eorg>
--2uRhKbVrb+w=
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<!DOCTYPE html><html><head><style></style></head><body><p> Attention is cur=
rently required from: plaisthos=2E </p>
<p>flichtenheld would like plaistho=
s to <strong>review</strong> this change=2E
</p><p><a href=3D"http://gerrit=
=2Eopenvpn=2Enet/c/openvpn/+/1831?usp=3Demail">View Change</a></p><pre clas=
s=3D"blocks" style=3D"font-family: monospace,monospace; white-space: pre-wr=
ap;">ssl: Do not queue control ciphertext while a packet is still queued<br=
><br>An outgoing control channel packet is handed to the link layer as a<br=
>buffer descriptor pointing into the reliable send buffer it was built<br>f=
rom, and the packet id sits in that buffer's headroom, right in front<b=
r>of the payload=2E If the entry is reused before the packet has been<br>w=
ritten out, buf_copy_n() writes the new payload and<br>reliable_mark_active=
_outgoing() prepends the new packet id exactly over<br>the packet id of the=
queued packet, while its opcode, ACK array and<br>length stay untouched=2E=
The queued packet then goes out with somebody<br>else's packet id=2E<=
br><br>Observed in a TCP p2p handshake: both peers reset simultaneously, th=
e<br>peer's two HARD_RESET packets arrive back to back, so io_wait_dowo=
rk()<br>takes the residual data shortcut (event_set_status =3D SOCKET_READ)=
and<br>does not write out our already queued HARD_RESET retransmit=2E The=
ACK<br>in the second peer reset then purges our reset from the send window=
,<br>tls_process_state() moves to S_START and queues the ClientHello into<b=
r>the very same (now inactive) entry=2E Result on the wire: a HARD_RESET<b=
r>with the ClientHello's packet id 1, followed by the ClientHello with<=
br>the same id 1=2E The receiver consumes the reset, advances its receive<=
br>window, and drops the real ClientHello as a replay - the handshake<br>de=
adlocks until it times out=2E<br><br>The send path in tls_process_state() a=
nd the dedicated ACK path in<br>tls_process() are already guarded by to_lin=
k->len, only the ciphertext<br>queueing was not=2E Guard it as well=2E =
A pending to_link makes<br>tls_process() report itself as active, so we ar=
e called again as soon<br>as the packet has been written out=2E In the err=
or path this can drop a<br>TLS alert that we would have queued, which is in=
line with that path<br>not ensuring delivery anyway=2E<br><br>Change-Id: I=
b0e7c9d3a2f4e6b8c1d5a9f7e3b2c4d6a8f1e5b3<br>Signed-off-by: Frank Lichtenhel=
d <frank@lichtenheld=2Ecom><br>---<br>M src/openvpn/ssl=2Ec<br>1 file=
changed, 10 insertions(+), 3 deletions(-)<br><br></pre><pre class=3D"block=
s" style=3D"font-family: monospace,monospace; white-space: pre-wrap;">git p=
ull ssh://gerrit=2Eopenvpn=2Enet:29418/openvpn refs/changes/31/1831/1</pre>=
<pre style=3D"font-family: monospace,monospace; white-space: pre-wrap;"><sp=
an>diff --git a/src/openvpn/ssl=2Ec b/src/openvpn/ssl=2Ec</span><br><span>i=
ndex 60df7ce=2E=2E7f2e850 100644</span><br><span>--- a/src/openvpn/ssl=2Ec<=
/span><br><span>+++ b/src/openvpn/ssl=2Ec</span><br><span>@@ -2714,8 +2714,=
15 @@</span><br><span> </span><br><span> static bool</span><br><span> check=
_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,</sp=
an><br><span style=3D"color: hsl(0, 100%, 40%);">- =
bool *continue_tls_process)</span><br><span style=3D"color: hsl(120, 100%,=
40%);">+ struct buffer *to_link, bool *continue_t=
ls_process)</span><br><span> {</span><br><span style=3D"color: hsl(120, 100=
%, 40%);">+ if (to_link->len)</span><br><span style=3D"color: hsl(120=
, 100%, 40%);">+ {</span><br><span style=3D"color: hsl(120, 100%, 40%);"=
>+ dmsg(D_TLS_DEBUG,</span><br><span style=3D"color: hsl(120, 100%, =
40%);">+ "Deferring outgoing ciphertext, previous packet n=
ot written out yet");</span><br><span style=3D"color: hsl(120, 100%, 4=
0%);">+ return true;</span><br><span style=3D"color: hsl(120, 100%, =
40%);">+ }</span><br><span style=3D"color: hsl(120, 100%, 40%);">+</span=
><br><span> /* Outgoing Ciphertext to reliable buffer */</span><br><spa=
n> if (ks->state >=3D S_START)</span><br><span> {</span><br><=
span>@@ -2895,7 +2902,7 @@</span><br><span> dmsg(D_TLS_DEBUG, &=
quot;Outgoing Plaintext -> TLS");</span><br><span> }</span>=
<br><span> }</span><br><span style=3D"color: hsl(0, 100%, 40%);">- i=
f (!check_outgoing_ciphertext(ks, session, &continue_tls_process))</spa=
n><br><span style=3D"color: hsl(120, 100%, 40%);">+ if (!check_outgoing_=
ciphertext(ks, session, to_link, &continue_tls_process))</span><br><spa=
n> {</span><br><span> goto error;</span><br><span> }</span>=
<br><span>@@ -2907,7 +2914,7 @@</span><br><span> /* Shut down the TLS s=
ession but do a last read from the TLS</span><br><span> * object to be=
able to read potential TLS alerts */</span><br><span> key_state_ssl_sh=
utdown(&ks->ks_ssl);</span><br><span style=3D"color: hsl(0, 100%, 40=
%);">- check_outgoing_ciphertext(ks, session, &continue_tls_process)=
;</span><br><span style=3D"color: hsl(120, 100%, 40%);">+ check_outgoing=
_ciphertext(ks, session, to_link, &continue_tls_process);</span><br><sp=
an> </span><br><span> /* Put ourselves in the pre error state that will=
only send out the</span><br><span> * control channel packets but noth=
ing else */</span><br><span></span><br></pre><p>To view, visit <a href=3D"h=
ttp://gerrit=2Eopenvpn=2Enet/c/openvpn/+/1831?usp=3Demail">change 1831</a>=
=2E To unsubscribe, or for help writing mail filters, visit <a href=3D"http=
://gerrit=2Eopenvpn=2Enet/settings?usp=3Demail">settings</a>=2E</p><div ite=
mscope itemtype=3D"http://schema=2Eorg/EmailMessage"><div itemscope itempro=
p=3D"action" itemtype=3D"http://schema=2Eorg/ViewAction"><link itemprop=3D"=
url" href=3D"http://gerrit=2Eopenvpn=2Enet/c/openvpn/+/1831?usp=3Demail"/><=
meta itemprop=3D"name" content=3D"View Change"/></div></div>
<div style=3D=
"display:none"> Gerrit-MessageType: newchange </div>
<div style=3D"display:=
none"> Gerrit-Project: openvpn </div>
<div style=3D"display:none"> Gerrit-B=
ranch: master </div>
<div style=3D"display:none"> Gerrit-Change-Id: Ib0e7c9=
d3a2f4e6b8c1d5a9f7e3b2c4d6a8f1e5b3 </div>
<div style=3D"display:none"> Gerr=
it-Change-Number: 1831 </div>
<div style=3D"display:none"> Gerrit-PatchSet:=
1 </div>
<div style=3D"display:none"> Gerrit-Owner: flichtenheld <frank=
@lichtenheld=2Ecom> </div>
<div style=3D"display:none"> Gerrit-Reviewer:=
plaisthos <arne-openvpn@rfc2549=2Eorg> </div>
<div style=3D"display:=
none"> Gerrit-CC: openvpn-devel <openvpn-devel@lists=2Esourceforge=2Enet=
> </div>
<div style=3D"display:none"> Gerrit-Attention: plaisthos <ar=
ne-openvpn@rfc2549=2Eorg> </div>
</body></html>
--2uRhKbVrb+w=--
--===============1083160401716115028==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
--===============1083160401716115028==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
--===============1083160401716115028==--