Skip to contentSkip to conversation

push: Provide a warning if --ifconfig-push have argument mismatch with --topology

PATCH In review David Sommerseth 8 participants 16 messages Nov 30, 2016 – Dec 9, 2016

lore
Share & tools
lore

David Sommerseth

author
#0 lore raw

PATCH 1/1 push: Provide a warning if --ifconfig-push have argument mismatch with --topology

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask. The check done is to ensure the first octet is 0xff (255)

Trac: #755

Signed-off-by David Sommerseth <[email protected]...>
---
 src/openvpn/push.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
src/openvpn/push.c +10 -0
index 9953079..0819bf8 100644
@@ -333,6 +333,16 @@ prepare_push_reply (struct context *c, struct gc_arena *gc,
 		       print_in_addr_t (ifconfig_local, 0, gc),
 		       print_in_addr_t (c->c2.push_ifconfig_remote_netmask,
 					0, gc));
+
+      /* Warn if ifconfig_remote_netmask does not contain
+       * subnet mask in subnet topology */
+      if ((c->options.topology == TOP_SUBNET || c->options.topology == TOP_NET30)
+          && (c->c2.push_ifconfig_remote_netmask & 0xff000000) != 0xff000000)
+        {
+          msg(M_WARN, "WARNING: When --topology is subnet or net30, the second argument "
+              "to --ifconfig-push MUST be a subnet mask.  This client will not be able "
+              "to connect.");
+        }
     }
 
   /* Send peer-id if client supports it */
Signature · 1 line
1.8.3.1
#1 lore raw

While I admit that it is *extremely* unlikely to have a network larger than /8, such logic still looks a little clumsy. It does not cover all the valid netmasks neither it detects all possible invalid ones.

If you wish to test if the netmask is valid, this solution could be better:
http://stackoverflow.com/questions/17401067/c-code-for-valid-netmask

Quoted 46 lines

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask. The check done is to ensure the first octet is 0xff
(255)

Trac: #755

Signed-off-by David Sommerseth <[email protected]...>
---
 src/openvpn/push.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
src/openvpn/push.c +7 -0
index 9953079..0819bf8 100644
@@ -333,6 +333,16 @@ prepare_push_reply (struct context *c, struct gc_arena
 *gc,
 		       print_in_addr_t (ifconfig_local, 0, gc),
 		       print_in_addr_t (c->c2.push_ifconfig_remote_netmask,
 					0, gc));
+
+      /* Warn if ifconfig_remote_netmask does not contain
+       * subnet mask in subnet topology */
+      if ((c->options.topology == TOP_SUBNET || c->options.topology ==
 TOP_NET30)
+          && (c->c2.push_ifconfig_remote_netmask & 0xff000000) != 0xff000000)
+        {
+          msg(M_WARN, "WARNING: When --topology is subnet or net30, the

second argument "
+ "to --ifconfig-push MUST be a subnet mask. This client will
not be able "

src/openvpn/push.c +2 -0
+              "to connect.");
+        }
     }
 
   /* Send peer-id if client supports it */
Signature · 8 lines
1.8.3.1


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

Hi,

Quoted 1 line · SviMik

While I admit that it is *extremely* unlikely to have a network larger than /8, such logic still looks a little clumsy. It does not cover all the valid netmasks neither it detects all possible invalid ones.

This is true, but not really relevant. Right now, it will just do funky things, and there is no indication in the logs where to look.

Nobody uses non-contiguous netmasks these days (like "240.0.255.0"), so everything *normal* starts with a string of 1-bits, and a valid IPv4 address never starts with 1111xxxx, so checking for "255." at the start of something that could be "a netmask or a remote IPv4 address" will get it right in about all cases we care about.

If someone insists on doing a /7 on their tun interface, they better know really well what they are doing.

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#3 lore raw
In reply to David Sommerseth

Hi,

Some nitpicking on the eleventh hour...

The comment does not agree with the code:

src/openvpn/push.c prepare_push_reply (struct context *c, struct · line 341

Quoted 11 lines

+++ b/src/openvpn/push.c

src/openvpn/push.c +3 -0
@@ -333,6 +333,16 @@ prepare_push_reply (struct context *c, struct
 gc_arena *gc,
                       print_in_addr_t (ifconfig_local, 0, gc),
                       print_in_addr_t (c->c2.push_ifconfig_remote_
 netmask,
                                        0, gc));
+
+      /* Warn if ifconfig_remote_netmask does not contain
+       * subnet mask in subnet topology */

If we go by the code this should be subnet and net30 topology. By the way, style alert -- we want the ending */ on a line by itself, don't we?

Quoted 10 lines
(quoted diff) +1 -0
+      if ((c->options.topology == TOP_SUBNET || c->options.topology ==

TOP_NET30)
+ && (c->c2.push_ifconfig_remote_netmask & 0xff000000) !=
0xff000000)

(quoted diff) +2 -0
+        {
+          msg(M_WARN, "WARNING: When --topology is subnet or net30, the

second argument "
+ "to --ifconfig-push MUST be a subnet mask. This client
will not be able "
+ "to connect.");

+ }

Quoted 2 lines
     }

I don't know the code path well enough, so cant say anything technical...

Selva

#4 lore raw
In reply to David Sommerseth

Hi,

src/openvpn/push.c prepare_push_reply (struct context *c, struct gc_arena *gc, · line 340

Quoted 2 lines · David Sommerseth
src/openvpn/push.c +2 -0
+      if ((c->options.topology == TOP_SUBNET || c->options.topology == TOP_NET30)
+          && (c->c2.push_ifconfig_remote_netmask & 0xff000000) != 0xff000000)

Are you sure of that? I would assume that this is stored in network byte order (as it's an in_addr_t) so you can't directly compare it to a number but need to run through ntohl() first.

On *intel* it might end up being the same, but code dealing with IPv4 addresses and ports always needs to be checked on both endiannesses.

Some random code I just found in a BSD header file does it that way:

#define INADDR_ALLHOSTS_GROUP (u_int32_t)0xe0000001 /* 224.0.0.1 */
#define in_allhosts(x) ((x).s_addr == htonl(INADDR_ALLHOSTS_GROUP))

... so it certainly needs the htonl()

(What you can do is "peek at byte 0", which will always be the same part of the netmask [network byte order!], and which might actually make this easier to read .-) )

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#5 lore raw
Quoted 6 lines · Gert Doering

Hi,

src/openvpn/push.c prepare_push_reply (struct context *c, struct gc_arena *gc, · line 340

Quoted 2 lines · David Sommerseth
src/openvpn/push.c +2 -0
+      if ((c->options.topology == TOP_SUBNET || c->options.topology == TOP_NET30)
+          && (c->c2.push_ifconfig_remote_netmask & 0xff000000) != 0xff000000)
Quoted 6 lines

Are you sure of that? I would assume that this is stored in network byte order (as it's an in_addr_t) so you can't directly compare it to a number but need to run through ntohl() first.

On *intel* it might end up being the same, but code dealing with IPv4 addresses and ports always needs to be checked on both endiannesses.

Good point! I didn't consider endianess too careful, I just looked at what we do in print_in_addr_t(), realised that we always call it with 0 in the flags argument in same prepare_push_reply() function.

      ia.s_addr = (flags & IA_NET_ORDER) ? addr : htonl (addr);

And now (when I have slightly more brainpower) I see I misread the logic, so ....

Quoted 6 lines

Some random code I just found in a BSD header file does it that way:

#define INADDR_ALLHOSTS_GROUP (u_int32_t)0xe0000001 /* 224.0.0.1 */
#define in_allhosts(x) ((x).s_addr == htonl(INADDR_ALLHOSTS_GROUP))

... so it certainly needs the htonl()

... Yes!

Quoted 3 lines

(What you can do is "peek at byte 0", which will always be the same part of the netmask [network byte order!], and which might actually make this easier to read .-) )

You mean like this?

    in_addr_t nmask = htonl(c->c2.push_ifconfig_remote_netmask);
    if ( nmask[0] == 0xff )
      {
         ...
Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
#6 lore raw

Hi,

Quoted 11 lines · David Sommerseth
Quoted 3 lines

(What you can do is "peek at byte 0", which will always be the same part of the netmask [network byte order!], and which might actually make this easier to read .-) )

You mean like this?

    in_addr_t nmask = htonl(c->c2.push_ifconfig_remote_netmask);
    if ( nmask[0] == 0xff )
      {
         ...

No :-) - if you htonl() it, you need to work with full words and 0xff000000 etc. - but if you just peek at

((uchar *)&c->c2.push_ifconfig_remote_netmask)[0]

you should get the first byte of the netmask, no matter what the host byte order is. (I'm not absolutely sure it's [0], could be [3], but in any case, since this is "network byte order", location *in memory* will not be host byte order dependent)

But right now I'm a bit too busy to properly think about this, and would just postpone to 2.4.1 - it's really minor.

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#7 lore raw
Quoted 1 line · Gert Doering
   ((uchar *)&c->c2.push_ifconfig_remote_netmask)[0]

Looks like dereferencing a type-punned pointer to me ;)

-Steffan

#8 lore raw

Hi,

Quoted 4 lines · Steffan Karger
Quoted 1 line · Gert Doering
   ((uchar *)&c->c2.push_ifconfig_remote_netmask)[0]

Looks like dereferencing a type-punned pointer to me ;)

I was waiting for this :-)

(...but I thought "char *" is allowed by the rules?)

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#9 lore raw
Quoted 9 lines · Gert Doering
Quoted 4 lines · Steffan Karger
Quoted 1 line · Gert Doering
   ((uchar *)&c->c2.push_ifconfig_remote_netmask)[0]

Looks like dereferencing a type-punned pointer to me ;)

I was waiting for this :-)

(...but I thought "char *" is allowed by the rules?)

Heh, and I was worried that you had already though of this, so I must have been wrong ;)

Indeed, "character types" (including unsigned char) are exempt of the strict aliasing rules. C99 6.5:

An object shall have its stored value accessed only by an lvalue expression that has one of the following types:
— [...], or
— a character type.

Apologies for the noise.

-Steffan

#10 lore raw
In reply to David Sommerseth

Am 30.11.16 um 23:41 schrieb David Sommerseth:

Quoted 5 lines

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask. The check done is to ensure the first octet is 0xff (255)

But way you actually want to test is

if topology == subnet or net30:
if gateway not in net(ip, mask):
print ("Your gw and ip and netmask disagree!)

right? And isn't there code that gets executed for the client side that can be disabled by ifconfig-nowarn. Can we reuse that code?

Arne

#11 lore raw

Hi,

Quoted 5 lines · Arne Schwabe

Am 30.11.16 um 23:41 schrieb David Sommerseth:

Quoted 3 lines

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask. The check done is to ensure the first octet is 0xff (255)

Quoted 8 lines

But way you actually want to test is

if topology == subnet or net30:
if gateway not in net(ip, mask):
print ("Your gw and ip and netmask disagree!)

right? And isn't there code that gets executed for the client side that can be disabled by ifconfig-nowarn. Can we reuse that code?

That is certainly something we should test as well, but the particular case I had in mind was

- someone uses --topology p2p
- they have ccd/ files with "ifconfig-push 10.4.0.14 10.4.0.13" (for some clients that need to get a static address)
- they change their server.conf to --topology subnet
- everything works for "dynamic clients", but the static client now receives "10.4.0.13" and interprets it as a "netmask", which will cause the most interesting things, depending on platform code

so the idea was to help this case by looking at only the "netmask" thing, and see if it makes sense (simplified sense: starts with 255.) - and if not, log this on the server side right when reading the ccd/ file so the admin in question can see "oh, I overlooked *that* special client".

(Since that happened to a friend of mine based on my advice, I know that this is not a purely theoretical possibility :-) )

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#12 lore raw

Am 01.12.16 um 13:37 schrieb Gert Doering:

Quoted 26 lines

Hi,

Quoted 4 lines · Arne Schwabe

Am 30.11.16 um 23:41 schrieb David Sommerseth:

Quoted 3 lines

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask. The check done is to ensure the first octet is 0xff (255)

Quoted 8 lines

But way you actually want to test is

if topology == subnet or net30:
if gateway not in net(ip, mask):
print ("Your gw and ip and netmask disagree!)

right? And isn't there code that gets executed for the client side that can be disabled by ifconfig-nowarn. Can we reuse that code?

That is certainly something we should test as well, but the particular case I had in mind was

- someone uses --topology p2p
- they have ccd/ files with "ifconfig-push 10.4.0.14 10.4.0.13" (for some clients that need to get a static address)
- they change their server.conf to --topology subnet
- everything works for "dynamic clients", but the static client now receives "10.4.0.13" and interprets it as a "netmask", which will cause the most interesting things, depending on platform code

Quoted 8 lines

so the idea was to help this case by looking at only the "netmask" thing, and see if it makes sense (simplified sense: starts with 255.) - and if not, log this on the server side right when reading the ccd/ file so the admin in question can see "oh, I overlooked *that* special client".

(Since that happened to a friend of mine based on my advice, I know that this is not a purely theoretical possibility :-) )

You are still very likely to catch this case If you check if in net30/subnet the netmask is indead valid (e.g. CIDR)

Arne

author
#13 lore raw

PATCH 1/1 push: Provide a warning if --ifconfig-push have argument mismatch with --topology

In reply to David Sommerseth

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask.

v2 - Make use of ifconfig_sanity_check() in tun.c instead of doing the exact
same check and warning in prepare_push_reply(). Also improve documentation of ifconfig_sanity_check() while at it.

Trac: #755

Signed-off-by David Sommerseth <[email protected]...>
---
 src/openvpn/push.c |  8 ++++++++
 src/openvpn/tun.c  | 20 ++++++++++++++------
 src/openvpn/tun.h  |  2 ++
 3 files changed, 24 insertions(+), 6 deletions(-)
src/openvpn/push.c +8 -0
index 9953079..5292b06 100644
@@ -333,6 +333,14 @@ prepare_push_reply (struct context *c, struct gc_arena *gc,
 		       print_in_addr_t (ifconfig_local, 0, gc),
 		       print_in_addr_t (c->c2.push_ifconfig_remote_netmask,
 					0, gc));
+
+      /* Warn if ifconfig_remote_netmask contains an unexpected value
+       * when checking configuration up against TUN/TAP device and
+       * network topology
+       */
+      ifconfig_sanity_check(c->c1.tuntap->type == DEV_TYPE_TUN,
+                            c->c2.push_ifconfig_remote_netmask,
+                            c->options.topology);
     }
 
   /* Send peer-id if client supports it */
src/openvpn/tun.c +14 -6
index 572e168..8df3489 100644
@@ -285,14 +285,22 @@ guess_tuntap_dev (const char *dev,
 /* --ifconfig-nowarn disables some options sanity checking */
 static const char ifconfig_warn_how_to_silence[] = "(silence this warning with --ifconfig-nowarn)";
 
-/*
- * If !tun, make sure ifconfig_remote_netmask looks
- *  like a netmask.
+/**
+ * If not a tun device, make sure ifconfig_remote_netmask looks
+ * like a netmask.
+ *
+ * If a tun device, make sure ifconfig_remote_netmask looks
+ * like an IPv4 address if topology is also TOP_NET30 or TOP_P2P.
+ *
+ * The result of this check is only reported to the log file as a warning
+ * when issues are found.
+ *
+ * @param tun       Boolean; if true device is a tun device, otherwise tap
+ * @param addr      Address to do sanity check on
+ * @param topology  Expected to be TOP_NET30, TOP_P2P, TOP_SUBNET
  *
- * If tun, make sure ifconfig_remote_netmask looks
- *  like an IPv4 address.
  */
-static void
+void
 ifconfig_sanity_check (bool tun, in_addr_t addr, int topology)
 {
   struct gc_arena gc = gc_new ();
src/openvpn/tun.h +2 -0
index 9b5a1b7..1a1f0b2 100644
@@ -229,6 +229,8 @@ const char *guess_tuntap_dev (const char *dev,
 			      const char *dev_node,
 			      struct gc_arena *gc);
 
+void ifconfig_sanity_check (bool tun, in_addr_t addr, int topology);
+
 struct tuntap *init_tun (const char *dev,       /* --dev option */
 			 const char *dev_type,  /* --dev-type option */
 			 int topology,          /* one of the TOP_x values */
Signature · 1 line
1.8.3.1
#14 lore raw

Hi,

Quoted 7 lines · David Sommerseth

This adds a warning to the log file if --topology is configured to use subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option is not an subnet mask.

v2 - Make use of ifconfig_sanity_check() in tun.c instead of doing the exact
same check and warning in prepare_push_reply(). Also improve documentation of ifconfig_sanity_check() while at it.

Not being the one to complain about your code all the time... but... since trac #755 is about "topology subnet"...

static void
ifconfig_sanity_check (bool tun, in_addr_t addr, int topology) {
struct gc_arena gc = gc_new ();
const bool looks_like_netmask = ((addr & 0xFF000000) == 0xFF000000); if (tun)

    {
      if (looks_like_netmask && (topology == TOP_NET30 || topology == TOP_P2P))
        msg (M_WARN, "WARNING: Since you are using --dev tun with a point-to-poi
nt topology, the second argument to --ifconfig must be an IP address.  You are u
sing something (%s) that looks more like a netmask. %s",
             print_in_addr_t (addr, 0, &gc),
             ifconfig_warn_how_to_silence);
    }

else /* tap */

... ifconfig_sanity_check() does *nothing* for TOP_SUBNET (and the code disagrees with your commit message for "net30").

So the v3 version of that patch would need to add the (missing today, but actually important diagnostic aid - and the point of #755) clause

    if (!looks_like_netmask && topology == TOP_SUBNET)
    {
	msg (M_WARN, "topology subnet needs a netmask as second argument bla bla");
    }

Also we might to re-think the warning message printed - if called from an ifconfig-push context, the text "the second argument to --ifconfig must be an IP address" is less than clear ("my --ifconfig settings are just fine, what is openvpn warning about?").

Plus, it needs to be actually tested on a non-intel architecture - the code has been like that forever, but I would not trust it to be endian- safe (though our use of "in_addr_t" is not actually consistent with what *should* be in one - parts of the code treat it as "network byte order", other parts as "host byte order", and the point of using "in_addr_t" as well defined *type* and not "a bag of 32bits, unsigned" should have been "as defined by the socket API", which wants network byte order).

But since this is an enhancement and not a bug, it should not be rushed - it does not have to be in 2.4.0. Let's do it properly, test it, then see if we want to have it in 2.4.1 (and while at it, we should teach check_addr_clash() that networks do have a netmask for a reason, and "local/remote and --ifconfig are in the same /24" could be perfectly fine if the pushed netmask is *not* a /24).

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#15 lore raw

Hi,

Quoted 1 line · Gert Doering

... ifconfig_sanity_check() does *nothing* for TOP_SUBNET

Overlooked the second patch (since it wasn't threaded). So with the other patch, that argument is no longer valid, of course. Apologies.

[..]

Quoted 4 lines

Also we might to re-think the warning message printed - if called from an ifconfig-push context, the text "the second argument to --ifconfig must be an IP address" is less than clear ("my --ifconfig settings are just fine, what is openvpn warning about?").

That one still holds... technically it's more "for the other patch", but since *this* patch is calling ifconfig_sanity_check() from a new context, it should tackle the now-misleading warning text, I think.

(So maybe apply the other one first, after verifying endian correctness on sparc or mips/be architecture, and this one then removes the static, adds a flag for "ifconfig or ifconfig-push context?" and adapts the messages accordingly)

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...