Skip to contentSkip to conversation

Fix edge case with clients failing to set up cipher on empty PUSH_REPLY.

PATCH In review Gert Doering 2 participants 3 messages Jun 18, 2017

lore
Share & tools
lore

Gert Doering

author
#0 lore raw

PATCH 1/1 Fix edge case with clients failing to set up cipher on empty PUSH_REPLY.

The NCP (data channel crypto negotiation) code on the client side waits for an incoming PUSH_REPLY before setting up the data channel crypto parameters, because the PUSH_REPLY could contain a "cipher xxx" setting.

In the particular case of a empty PUSH_REPLY message, the relevant code bits was not called because "we have not received any options, do not bother to look into it in more detail" - so, ciphers were not set up, resulting in an error message like this:

Key [AF_INET]... [0] not initialized (yet), dropping packet.

Remove that check, always init the crypto layer on PUSH_REPLY.

Trac: #903

Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/openvpn/init.c +1 -1
index 2532cf8..91ab28e 100644
@@ -1925,7 +1925,7 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found)
     {
         reset_coarse_timers(c);
 
-        if (pulled_options && option_types_found)
+        if (pulled_options)
         {
             if (!do_deferred_options(c, option_types_found))
             {
Signature · 1 line
2.10.2
#1 lore raw
Quoted 26 lines · Gert Doering

The NCP (data channel crypto negotiation) code on the client side waits for an incoming PUSH_REPLY before setting up the data channel crypto parameters, because the PUSH_REPLY could contain a "cipher xxx" setting.

In the particular case of a empty PUSH_REPLY message, the relevant code bits was not called because "we have not received any options, do not bother to look into it in more detail" - so, ciphers were not set up, resulting in an error message like this:

Key [AF_INET]... [0] not initialized (yet), dropping packet.

Remove that check, always init the crypto layer on PUSH_REPLY.

Trac: #903

Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/openvpn/init.c +0 -0
index 2532cf8..91ab28e 100644

src/openvpn/init.c do_up(struct context *c, bool pulled_options, unsigned int option_types_found) · line 1931

Quoted 8 lines
src/openvpn/init.c +1 -1
@@ -1925,7 +1925,7 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found)
     {
         reset_coarse_timers(c);
 
-        if (pulled_options && option_types_found)
+        if (pulled_options)
         {
             if (!do_deferred_options(c, option_types_found))
             {

ACK - this allows the NCP code in do_deferred_options() to do what it wanted to.

-Steffan

author
#2 lore raw
In reply to Gert Doering

Patch has been applied to the master and release/2.4 branch.

commit bd230079d98bfe6aec70b7aedefdffcdbd0e56da (master) commit e82f7005256f77a63a3191ab7fef67e0cf0a9d02 (release/2.4)

Author: Gert Doering
Date:   Sun Jun 18 11:22:44 2017 +0200
     Fix edge case with clients failing to set up cipher on empty PUSH_REPLY.

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14856.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering