Re: [Openvpn-devel] OpenVPN Pf plugin/small status patch

[David S. <ope...@to...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 26/06/09 17:00, Arne Schwabe wrote:
> Hi,
> 
> I have written a simple plugin for packet filtering that looks up fw rules
> in the order
> 
> Commonname.pf
> IP_Port.pf
> IP.pf
> default.pf
> 
> If one of this files is found the file is used as PF configuration. Maybe
> this plugin is useful for someone else.

Hi!

Thank you for your patches.  I've been looking at both patches, and I
have some questions in regards to this plug-in.

How does this packet filtering further work?  I do not completely
understand how you imagine this should work.  I see that it tries to
open a number of files with different filename criteria , and if it
finds a file it copies it somewhere.

But how does that itself really provide packet filtering?

The code itself is not bad, I'd like to have it cleaned up a little bit
before inclusion.  Things I'm picking on are consequent spacing,
removing unneeded blank lines, improved comments (explaining *what* is
happening).

In addition the copypffile() function can be written a bit more clever,
avoiding using stack space for an array of file names.  As you already
have a for() loop, it would be better, IMHO, to build up the filename
string in this loop, using a simple select() for the filename template.

You also have a potential buffer overflow bug with your snprintf().  If
common name (the most likely one to attack) is longer than 256 bytes,
the filename will consist of those 256 bytes without a NULL terminator,
as snprintf() expect the NULL terminator to be included in the
size/length provided.

I'd recommend something like:

   char str[258];  // 256 bytes + 2 bytes extra

   memset(&str, 0, 258);  // Make sure it's NULL
   snprintf(str, 256, "%.253s.pf%c", cn, '\0');

or:

   char *str = calloc(1, 258); // 256 + 2 extra
   snprintf(str, 256, "%.253s.pf%c", cn, '\0');

   free(str);

This way str will never exceed 256 bytes, including the extension, and
it will be NULL terminated for sure.  In addition you are safer against
buffer overflows.  If using calloc(), the memory can be expected to be
zeroed as well (one of the advantages of calloc() compared to malloc()).

Using the str pointer above in a for loop, you can just call snprintf()
several times to change the contents, and free it after the for() loop.

I would also recommend to be stricter to the allowed string length when
processing IP addresses, port numbers and device name.  F.ex. the
filename template for port number could be: "%.5s.pf", as a port number
cannot be bigger than 65535, which is 5 digits.

And the last important thing, you have no error checking if copypffile()
fails or not.  OpenVPN will believe that this function call will always
work.  Granted, you if no files exist to be copied that might not be a
failure at all.  But it could be a failure if it could not write to the
destination file when it does the copy.

You also have two functions which might not be needed as well,
openvpn_plugin_client_contstructor_v1() and
openvpn_plugin_client_destructor_v1().  They don't seem to do anything
useful and OpenVPN don't require them, AFAIK.

The printf() messages could also specify the plug-in name instead of
just generic function names ... especially important for users who use
several plug-ins in parallel.


> Add I would like to ask to include the patch for multi.c.
> 
> There are commands in the management interface which require the cid. The
> only way at the moment to get the cid of connected clients is to have
> always a management connection established. The patch adds the CID to the
> status output.

This patch is sent for review by more developers.  It will need an
official ACK before inclusion, so I hope that will come soon.

Thank you once again for your contributions!


kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuKbggACgkQDC186MBRfrrG6ACfWIuNyCPpufx1DwRmv2RGCqug
8TEAnjRy8EPYVfrlCXw6xdSNg0UPaJl2
=oaxR
-----END PGP SIGNATURE-----