| Message ID | 20200725235143.22736-1-arne@rfc2549.org |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [Openvpn-devel] Refuse PUSH_REQUEST as client/refactor process_incoming_push_request | expand |
Sanity check: On 26/07/2020 00:51, Arne Schwabe wrote: > When a server sends a client a push request, the client will reply > with a push reply. The reply is bogus and almost empty since almost > all the options that are normally set (remote ip etc) are unset. > > I checked 2.4 and master and this does not have any security implications > or other bugs but it is still better to refuse this. > > This code also refactors the method to get rid of the ret variable to > make the code flow easier to understand. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/push.c | 40 ++++++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 18 deletions(-) > > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index 1c4f2033..84193afe 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -733,13 +733,19 @@ push_remove_option(struct options *o, const char *p) > int > process_incoming_push_request(struct context *c) > { > - int ret = PUSH_MSG_ERROR; > + /* if we receive a message a client we do not want to reply to it > + * so limit this to multi server */ This sentence does not make sense, suggestion: If a client receives a push request then ignore it. Only multi server will reply to a push requests. Something like that .. > + if (c->options.mode != MODE_SERVER) > + { > + return PUSH_MSG_ERROR; > + } > + > > if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED) > { > const char *client_reason = tls_client_reason(c->c2.tls_multi); > send_auth_failed(c, client_reason); > - ret = PUSH_MSG_AUTH_FAILURE; > + return PUSH_MSG_AUTH_FAILURE; > } > else if (c->c2.context_auth == CAS_SUCCEEDED) > { > @@ -748,29 +754,27 @@ process_incoming_push_request(struct context *c) > openvpn_time(&now); > if (c->c2.sent_push_reply_expiry > now) > { > - ret = PUSH_MSG_ALREADY_REPLIED; > + return PUSH_MSG_ALREADY_REPLIED; > } > - else > - { > - /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */ > - struct push_list push_list = { 0 }; > - struct gc_arena gc = gc_new(); > > - if (prepare_push_reply(c, &gc, &push_list) > - && send_push_reply(c, &push_list)) > - { > - ret = PUSH_MSG_REQUEST; > - c->c2.sent_push_reply_expiry = now + 30; > - } > - gc_free(&gc); > + int ret = PUSH_MSG_ERROR; > + /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */ > + struct push_list push_list = { 0 }; > + struct gc_arena gc = gc_new(); > + > + if (prepare_push_reply(c, &gc, &push_list) > + && send_push_reply(c, &push_list)) > + { > + ret = PUSH_MSG_REQUEST; > + c->c2.sent_push_reply_expiry = now + 30; > } > + gc_free(&gc); > + return ret; > } > else > { > - ret = PUSH_MSG_REQUEST_DEFERRED; > + return PUSH_MSG_REQUEST_DEFERRED; > } > - > - return ret; > } > > static void >
Am 26.07.20 um 01:51 schrieb Arne Schwabe: > When a server sends a client a push request, the client will reply > with a push reply. The reply is bogus and almost empty since almost > all the options that are normally set (remote ip etc) are unset. > > I checked 2.4 and master and this does not have any security implications > or other bugs but it is still better to refuse this. > > This code also refactors the method to get rid of the ret variable to > make the code flow easier to understand. On further discussion on IRC, retract this patch. The tls-server/tls-client pair as a p2p pair with one side (does not even need to be the one with tls-server) can have multiple "push xy" in the config. Arne
diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 1c4f2033..84193afe 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -733,13 +733,19 @@ push_remove_option(struct options *o, const char *p) int process_incoming_push_request(struct context *c) { - int ret = PUSH_MSG_ERROR; + /* if we receive a message a client we do not want to reply to it + * so limit this to multi server */ + if (c->options.mode != MODE_SERVER) + { + return PUSH_MSG_ERROR; + } + if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED) { const char *client_reason = tls_client_reason(c->c2.tls_multi); send_auth_failed(c, client_reason); - ret = PUSH_MSG_AUTH_FAILURE; + return PUSH_MSG_AUTH_FAILURE; } else if (c->c2.context_auth == CAS_SUCCEEDED) { @@ -748,29 +754,27 @@ process_incoming_push_request(struct context *c) openvpn_time(&now); if (c->c2.sent_push_reply_expiry > now) { - ret = PUSH_MSG_ALREADY_REPLIED; + return PUSH_MSG_ALREADY_REPLIED; } - else - { - /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */ - struct push_list push_list = { 0 }; - struct gc_arena gc = gc_new(); - if (prepare_push_reply(c, &gc, &push_list) - && send_push_reply(c, &push_list)) - { - ret = PUSH_MSG_REQUEST; - c->c2.sent_push_reply_expiry = now + 30; - } - gc_free(&gc); + int ret = PUSH_MSG_ERROR; + /* per-client push options - peer-id, cipher, ifconfig, ipv6-ifconfig */ + struct push_list push_list = { 0 }; + struct gc_arena gc = gc_new(); + + if (prepare_push_reply(c, &gc, &push_list) + && send_push_reply(c, &push_list)) + { + ret = PUSH_MSG_REQUEST; + c->c2.sent_push_reply_expiry = now + 30; } + gc_free(&gc); + return ret; } else { - ret = PUSH_MSG_REQUEST_DEFERRED; + return PUSH_MSG_REQUEST_DEFERRED; } - - return ret; } static void
When a server sends a client a push request, the client will reply with a push reply. The reply is bogus and almost empty since almost all the options that are normally set (remote ip etc) are unset. I checked 2.4 and master and this does not have any security implications or other bugs but it is still better to refuse this. This code also refactors the method to get rid of the ret variable to make the code flow easier to understand. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/push.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)