[Openvpn-devel] [PATCH] Options parsing demands unnecessary configuration if PKCS11 is used

[Arne Schwabe]

In the old patch the if incorrectly closed the outer if condition.  (closes 
ticket #231)
---
 src/openvpn/options.c |   10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 05a0f54..8717b89 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2192,13 +2192,15 @@ options_postprocess_verify_ce (const struct options 
*options, const struct conne
}
   else
 #endif
-#ifdef ENABLE_CRYPTOAPI
 #ifdef MANAGMENT_EXTERNAL_KEY
 if((options->management_flags & MF_EXTERNAL_KEY) && 
options->priv_key_file)
-   msg (M_USAGE, "--key and --management-external-key are mutually 
exclusive");
+  {
+msg (M_USAGE, "--key and --management-external-key are 
mutually exclusive");
+  } 
+else 
 #endif
-
-  if (options->cryptoapi_cert)
+#ifdef ENABLE_CRYPTOAPI
+ if (options->cryptoapi_cert)
{
  if ((!(options->ca_file)) && (!(options->ca_path)))
msg(M_USAGE, "You must define CA file (--ca) or CA path 
(--capath)");
-- 
1.7.9.5

Re: [Openvpn-devel] [PATCH] Fixed a bug where PolarSSL gave an error when using an inline file tag.

[David Sommerseth]

On 17/10/12 11:39, Adriaan de Jong wrote:
> Signed-off-by: Adriaan de Jong 
> ---
>  src/openvpn/ssl_polarssl.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK.  Applied to master and beta/2.3.

commit 2ebbe4c0b4f0f0b15b4c32180e906a545446c376 (master)
commit a4b99628d19d6130a4f53e0ed84d9d0b2fd28dc0 (beta/2.3)
Author: Adriaan de Jong 
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Wed Oct 17 11:39:25 2012 +0200

Fixed a bug where PolarSSL gave an error when using an inline file tag.

Signed-off-by: Adriaan de Jong 
Acked-by: David Sommerseth 
Message-Id: 1350466765-23301-1-git-send-email-dej...@fox-it.com
Signed-off-by: David Sommerseth 


kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] RFC - Usage of --script-security with the 'system' flag

[David Sommerseth]

On 17/10/12 11:29, Jan Just Keijser wrote:
> Hi David,
> 
> David Sommerseth wrote:
>> Hi all,
>>
>> I've been reviewing a bug reported to the v2.3 code base.  We're in the 
>> beta phase currently, and this is a bug I'd like to get fixed before 
>> we're moving on further.  The bug is related to the use of the 'system' 
>> flag in --script-security.
>>
>> 
>>
>> The use of the 'system' flag has been deprecated for a long time.  And 
>> it is really a potential security issue to use it, due to shell 
>> expansions which might happen.  The preferred (and default way) is to 
>> use execve(), which is far safer and does not do the shell expansions 
>> while executing the script or binary.
>>
>>   
> on Linux the difference is not that big, however watch out for Windows 
> servers - with the old (system) like functionality it was possible to 
> specify e.g. a VBS script directly. With the 'exec' style you need to 
> specify the vbscript.exe, the full path to the script etc etc. There's 
> even an example about this in the book you're reviewed for me ;)
> If at all possible I'd leave in the 'system' like functionality , as it 
> is very valuable for debugging scripts. We can add warnings about it 
> being deprecated and the fact that there's the risk of memory leaks , 
> but I do see value in this feature.

Hi Jan,

Thanks a lot for a good input!  Yeah, I didn't have Windows too much in
my mind when looking at this issue :)  My ugly habit as a non-Windows user.

When using the 'system' flag, this is put into the log:

   NOTE: --script-security method='system' is deprecated due to the \
   fact that passed parameters will be subject to shell expansion

I agree that it's a convenience to just "dump the script file" directly
into the configuration file for debugging, as this 'system' flag
supports.  But wouldn't it be an increase the overall security if you
also had to explicitly define the script parser as well?

In *nix the explicit parser is set by the very first '#!' line, and the
file needs the exec bit set.  In Windows, it will basically try to
execute whatever has a system registered file extension.  I actually
wonder what happens if you provide a 'openvpn.ovpn' file as an --up
script ... given that OpenVPN GUI is set to execute .ovpn files when
double-clicking them.

I'm also thinking that in Windows, you either need to have:

  --script-security 2 --up "%windir%\\system32\\cmd.exe myscript.cmd"

or

  --script-security 2 system --up myscript.cmd

Right?  While in *nix you just provide the script file directly (due the
'#!' + exec flag solving the script parser).  So it would anyway need to
be documented for Windows users.

Would kicking out 'system' be such a burden for Windows users in that
perspective?


kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature

[Openvpn-devel] [PATCH] Fixed a bug where PolarSSL gave an error when using an inline file tag.

[Adriaan de Jong]

Signed-off-by: Adriaan de Jong 
---
 src/openvpn/ssl_polarssl.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/ssl_polarssl.c b/src/openvpn/ssl_polarssl.c
index 6995958..12318b3 100644
--- a/src/openvpn/ssl_polarssl.c
+++ b/src/openvpn/ssl_polarssl.c
@@ -338,7 +338,7 @@ void tls_ctx_load_ca (struct tls_root_ctx *ctx, const char 
*ca_file,

   if (ca_file && !strcmp (ca_file, INLINE_FILE_TAG) && ca_file_inline)
 {
-  if (0 != x509parse_crt(ctx->ca_chain, ca_file_inline, 
strlen(ca_file_inline)));
+  if (0 != x509parse_crt(ctx->ca_chain, ca_file_inline, 
strlen(ca_file_inline)))
msg (M_FATAL, "Cannot load inline CA certificates");
 }
   else
-- 
1.7.9.5

Re: [Openvpn-devel] RFC - Usage of --script-security with the 'system' flag

[Jan Just Keijser]

Hi David,

David Sommerseth wrote:

Hi all,

I've been reviewing a bug reported to the v2.3 code base.  We're in the 
beta phase currently, and this is a bug I'd like to get fixed before 
we're moving on further.  The bug is related to the use of the 'system' 
flag in --script-security.




The use of the 'system' flag has been deprecated for a long time.  And 
it is really a potential security issue to use it, due to shell 
expansions which might happen.  The preferred (and default way) is to 
use execve(), which is far safer and does not do the shell expansions 
while executing the script or binary.


  
on Linux the difference is not that big, however watch out for Windows 
servers - with the old (system) like functionality it was possible to 
specify e.g. a VBS script directly. With the 'exec' style you need to 
specify the vbscript.exe, the full path to the script etc etc. There's 
even an example about this in the book you're reviewed for me ;)
If at all possible I'd leave in the 'system' like functionality , as it 
is very valuable for debugging scripts. We can add warnings about it 
being deprecated and the fact that there's the risk of memory leaks , 
but I do see value in this feature.


JM2CW,

JJK


The fix I'm currently considering is to remove support for the system() 
call completely.  This support was introduced in 2.1_rc9 (Nov 17, 2008).


Does anyone depend on --script-security where the 'system' flag is 
required?  If you need this feature, can you please elaborate why this 
support is needed and why you cannot use the preferred default with 
execve?


The main differences between execve() and system() calls are:

system() loads a shell before executing the script or binary, which
also provides all system environment variables defined by
/etc/profile etc.  On top of that, all the OpenVPN configured
environment variables are added.

execve() will load and execute the script or binary directly, which
will only provide the environment variables provided by OpenVPN by
default.  Example:

   -
   #!/bin/bash

   printenv
   -

Using this with '--script-security 2' will dump this out:

   -
   dev_type=tap
   ifconfig_local=192.168.100.1
   proto_1=udp
   tun_mtu=1500
   ifconfig_netmask=255.255.255.0
   script_type=up
   verb=4
   local_port_1=10443
   dev=tap0
   remote_port_1=10443
   PWD=/home/test/openvpn
   daemon=0
   ifconfig_broadcast=192.168.100.255
   SHLVL=1
   script_context=init
   daemon_start_time=1350465187
   daemon_pid=19045
   daemon_log_redirect=0
   link_mtu=1573
   -

If you need system variables as well with execve, you can either modify
the #!/bin/bash line to add --login, or you can source /etc/profile in
your script.

If you modify the --script-security to look like
'--script-security 2 system', printenv will dump a rather massive list
with environment variables.



* Technical details regarding a possible fix:

The reason this is hard to fix, is that it will require a massive work 
to avoid memory leaks.  An first example:


   
 char *envvar = malloc(100);

 snprintf(envvar, 30, "TESTVAL=abc");
 putenv(envvar); /* NOTE: We only do putenv() here */
 system("/bin/sh");  /* echo $TESTVAL returns 'abc' */

 snprintf(envvar, 30, "TESTVAL=123456");  /* A simple change */
 system("/bin/sh");  /* echo $TESTVAL returns '123456' */

 free(e);
 system("/bin/sh");  /* echo $TESTVAL is not set */
   

The current implementation will clean up the environment variables too 
early when using 'system'.  When using 'execve', OpenVPN provides a the 
environment variables via a completely different mechanism which cleans 
up the memory properly at the right time.


The result is that with 'system', we're seeing assertion issues as it 
is now.  Trying to fix this, leads easily to other fun memory 
corruption issues.  And fixing this requires a more massive amount of 
adding proper gc_arena garbage collection higher up in the call paths.  
This is needed to let the system() call have all the environment 
variables set by OpenVPN untouched while running and first clean them 
up afterwards.


Considering that using system() is less safer than execve(), I would 
prefer to remove support for system(); instead of passing around a lot 
more gc_arena pointers to get the memory management correct for 
system() to function properly.


My first hackish attempt (without cleaning it up) gives this change stat:

 $ git diff --stat
 src/openvpn/misc.c |   65 
++---
 src/openvpn/misc.h |6 --
 src/openvpn/platform.c |6 +++---
 src/openvpn/platform.h |2 +-
 4 f

[Openvpn-devel] RFC - Usage of --script-security with the 'system' flag

[David Sommerseth]

Hi all,

I've been reviewing a bug reported to the v2.3 code base.  We're in the 
beta phase currently, and this is a bug I'd like to get fixed before 
we're moving on further.  The bug is related to the use of the 'system' 
flag in --script-security.



The use of the 'system' flag has been deprecated for a long time.  And 
it is really a potential security issue to use it, due to shell 
expansions which might happen.  The preferred (and default way) is to 
use execve(), which is far safer and does not do the shell expansions 
while executing the script or binary.

The fix I'm currently considering is to remove support for the system() 
call completely.  This support was introduced in 2.1_rc9 (Nov 17, 2008).

Does anyone depend on --script-security where the 'system' flag is 
required?  If you need this feature, can you please elaborate why this 
support is needed and why you cannot use the preferred default with 
execve?

The main differences between execve() and system() calls are:

system() loads a shell before executing the script or binary, which
also provides all system environment variables defined by
/etc/profile etc.  On top of that, all the OpenVPN configured
environment variables are added.

execve() will load and execute the script or binary directly, which
will only provide the environment variables provided by OpenVPN by
default.  Example:

   -
   #!/bin/bash

   printenv
   -

Using this with '--script-security 2' will dump this out:

   -
   dev_type=tap
   ifconfig_local=192.168.100.1
   proto_1=udp
   tun_mtu=1500
   ifconfig_netmask=255.255.255.0
   script_type=up
   verb=4
   local_port_1=10443
   dev=tap0
   remote_port_1=10443
   PWD=/home/test/openvpn
   daemon=0
   ifconfig_broadcast=192.168.100.255
   SHLVL=1
   script_context=init
   daemon_start_time=1350465187
   daemon_pid=19045
   daemon_log_redirect=0
   link_mtu=1573
   -

If you need system variables as well with execve, you can either modify
the #!/bin/bash line to add --login, or you can source /etc/profile in
your script.

If you modify the --script-security to look like
'--script-security 2 system', printenv will dump a rather massive list
with environment variables.



* Technical details regarding a possible fix:

The reason this is hard to fix, is that it will require a massive work 
to avoid memory leaks.  An first example:

   
 char *envvar = malloc(100);

 snprintf(envvar, 30, "TESTVAL=abc");
 putenv(envvar); /* NOTE: We only do putenv() here */
 system("/bin/sh");  /* echo $TESTVAL returns 'abc' */

 snprintf(envvar, 30, "TESTVAL=123456");  /* A simple change */
 system("/bin/sh");  /* echo $TESTVAL returns '123456' */

 free(e);
 system("/bin/sh");  /* echo $TESTVAL is not set */
   

The current implementation will clean up the environment variables too 
early when using 'system'.  When using 'execve', OpenVPN provides a the 
environment variables via a completely different mechanism which cleans 
up the memory properly at the right time.

The result is that with 'system', we're seeing assertion issues as it 
is now.  Trying to fix this, leads easily to other fun memory 
corruption issues.  And fixing this requires a more massive amount of 
adding proper gc_arena garbage collection higher up in the call paths.  
This is needed to let the system() call have all the environment 
variables set by OpenVPN untouched while running and first clean them 
up afterwards.

Considering that using system() is less safer than execve(), I would 
prefer to remove support for system(); instead of passing around a lot 
more gc_arena pointers to get the memory management correct for 
system() to function properly.

My first hackish attempt (without cleaning it up) gives this change stat:

 $ git diff --stat
 src/openvpn/misc.c |   65 
++---
 src/openvpn/misc.h |6 --
 src/openvpn/platform.c |6 +++---
 src/openvpn/platform.h |2 +-
 4 files changed, 50 insertions(+), 29 deletions(-)

I personally find that a bit too much to make a deprecated feature work.
Unless there are some strong arguments why we really do need system()

Any thoughts, ideas or suggestions are most welcome!


kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature