Opened 2 years ago

Closed 2 years ago

#1072 closed Bug / Defect (fixed)

--push-remove is not removing "ifconfig"

Reported by: Gert Döring Owned by: Gert Döring
Priority: major Milestone: release 2.5
Component: Generic / unclassified Version: OpenVPN git master branch (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc: Antonio

Description

push-remove "ifconfig-ipv6"

works perfectly fine, whereas

push-remove "ifconfig "

does not have any effect (which might not be surprising since ifconfig/ifconfig-ipv6 are special cases in push.c, and I might have only looked at ifconfig-ipv6).

As a side-note, push-remove ifconfig-ipv6 is the only match that needs to be exact, unlike all the other options that do substring matching (push-remove route matches "route" and "route-ipv6").

Change History (4)

comment:1 Changed 2 years ago by tincantech

thanks

comment:2 Changed 2 years ago by Gert Döring

void
push_remove_option(struct options *o, const char *p)
{
    msg(D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
    /* ifconfig-ipv6 is special, as not part of the push list */
    if (streq( p, "ifconfig-ipv6" ))
    {
        o->push_ifconfig_ipv6_blocked = true;
        return;
    }

and

prepare_push_reply(struct context *c, struct gc_arena *gc,
                   struct push_list *push_list)
{
...
    /* ipv6 */
    if (c->c2.push_ifconfig_ipv6_defined && !o->push_ifconfig_ipv6_blocked)
    {
        push_option_fmt(gc, push_list, M_USAGE, "ifconfig-ipv6 %s/%d %s",

take care of the ifconfig-ipv6 (and it's obvious why it's an exact match), while

...
    /* ipv4 */
    if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local
        && c->c2.push_ifconfig_remote_netmask)
    {
        in_addr_t ifconfig_local = c->c2.push_ifconfig_local;
        if (c->c2.push_ifconfig_local_alias)
        {
            ifconfig_local = c->c2.push_ifconfig_local_alias;
        }
        push_option_fmt(gc, push_list, M_USAGE, "ifconfig %s %s",
...

just doesn't do the "shall we block it?" check anywhere.

It's actually not straightforward to get the semantics right - the "it only works for exact matches for ifconfig and ifconfig-ipv6" approach is easy but different from all other options. Doing proper substring matching when the actual matching string is only constructed later is more complicated.

Yes, we could do the "is there anything to remove?" check in push_option_ex(), but that implies "store the push_remove options list somewhere", turning around the logic. So maybe it will be the easy approach first :-)

comment:3 Changed 2 years ago by Gert Döring

Patch on the list, quick and simple ("in style" :) ).

https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17160.html

comment:4 Changed 2 years ago by Gert Döring

Resolution: fixed
Status: assignedclosed

commit 6ae2f19d891e8cedccffdb1760b9774b9feff140
Author: Gert Doering
Date: Sun Jul 1 21:59:38 2018 +0200

Extend push-remove to also handle 'ifconfig'.

Signed-off-by: Gert Doering <gert@…>
Acked-by: Antonio Quartulli <antonio@…>
Message-Id: <20180701195938.2541-1-gert@…>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17169.html

Note: See TracTickets for help on using tickets.