Re: [Openvpn-devel] RFD: ssl library version numbers

[Steffan Karger]

On 04/16/2014 05:24 PM, Gert Doering wrote:
> OK, here's the full patch with this version.

One minor remark: if you add an extra * to the /* of the comment above
get_ssl_library_version(void), the comment becomes a doxygen comment and
will be included in the generated doxygen.

Otherwise, ACK.

-Steffan

Re: [Openvpn-devel] RFD: ssl library version numbers

[Gert Doering]

Hi,

On Tue, Apr 15, 2014 at 09:42:39AM +0200, Gert Doering wrote:
> Different approach:
> 
> char *
> get_ssl_library_version(void)
> {
> static char polar_version[30];
> unsigned int pv = version_get_number();
> sprintf( polar_version, "PolarSSL %d.%d.%d",
> (pv>>24)&0xff, (pv>>16)&0xff, (pv>>8)&0xff );
> return polar_version;
> }

OK, here's the full patch with this version.

gert


-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de
From 2834a400c6225833da9b9559f1be709b4e77d5e9 Mon Sep 17 00:00:00 2001
From: Gert Doering 
List-Post: openvpn-devel@lists.sourceforge.net
Date: Sun, 13 Apr 2014 17:29:32 +0200
Subject: [PATCH] Add SSL library version reporting.

Print the version of the SSL and LZO library (if any) used.

SSL library version is also sent as IV_SSL= to the server if
--push-peer-info is enabled.

Signed-off-by: Gert Doering 
---
 src/openvpn/openvpn.c  |  1 +
 src/openvpn/options.c  | 18 ++
 src/openvpn/ssl.c  |  1 +
 src/openvpn/ssl_backend.h  |  6 ++
 src/openvpn/ssl_openssl.c  |  6 ++
 src/openvpn/ssl_polarssl.c | 10 ++
 6 files changed, 42 insertions(+)

diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
index 5125eae..fd87fc1 100644
--- a/src/openvpn/openvpn.c
+++ b/src/openvpn/openvpn.c
@@ -220,6 +220,7 @@ openvpn_main (int argc, char *argv[])
 
  /* print version number */
  msg (M_INFO, "%s", title_string);
+ show_library_versions(M_INFO);
 
  /* misc stuff */
  pre_setup ();
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 18cb354..dc74b53 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3435,10 +3435,28 @@ usage_small (void)
   openvpn_exit (OPENVPN_EXIT_STATUS_USAGE); /* exit point */
 }
 
+void
+show_library_versions(const unsigned int flags)
+{
+  msg (flags, "library versions: %s%s%s",
+#ifdef ENABLE_SSL
+   get_ssl_library_version(),
+#else
+   "",
+#endif
+#ifdef ENABLE_LZO
+   ", LZO ", lzo_version_string()
+#else
+   "", ""
+#endif
+   );
+}
+
 static void
 usage_version (void)
 {
   msg (M_INFO|M_NOPREFIX, "%s", title_string);
+  show_library_versions( M_INFO|M_NOPREFIX );
   msg (M_INFO|M_NOPREFIX, "Originally developed by James Yonan");
   msg (M_INFO|M_NOPREFIX, "Copyright (C) 2002-2010 OpenVPN Technologies, Inc. 
");
 #ifndef ENABLE_SMALL
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index d4acc0f..b09e52b 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1835,6 +1835,7 @@ push_peer_info(struct buffer *buf, struct tls_session 
*session)
  get_default_gateway ();
  if (rgi.flags & RGI_HWADDR_DEFINED)
buf_printf (, "IV_HWADDR=%s\n", format_hex_ex (rgi.hwaddr, 6, 
0, 1, ":", ));
+ buf_printf (, "IV_SSL=%s\n", get_ssl_library_version() );
 }
 
   /* push env vars that begin with UV_ and IV_GUI_VER */
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a6fc3bd..b1087e1 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -466,4 +466,10 @@ void show_available_tls_ciphers (const char *tls_ciphers);
  */
 void get_highest_preference_tls_cipher (char *buf, int size);
 
+/*
+ * return a pointer to a static memory area containing the
+ * name and version number of the SSL library in use
+ */
+char * get_ssl_library_version(void);
+
 #endif /* SSL_BACKEND_H_ */
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 0b63e26..a7d7142 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1320,4 +1320,10 @@ get_highest_preference_tls_cipher (char *buf, int size)
   SSL_CTX_free (ctx);
 }
 
+char *
+get_ssl_library_version(void)
+{
+return  SSLeay_version(SSLEAY_VERSION);
+}
+
 #endif /* defined(ENABLE_SSL) && defined(ENABLE_CRYPTO_OPENSSL) */
diff --git a/src/openvpn/ssl_polarssl.c b/src/openvpn/ssl_polarssl.c
index 9dc4e87..844c04e 100644
--- a/src/openvpn/ssl_polarssl.c
+++ b/src/openvpn/ssl_polarssl.c
@@ -1079,4 +1079,14 @@ get_highest_preference_tls_cipher (char *buf, int size)
   strncpynt (buf, cipher_name, size);
 }
 
+char *
+get_ssl_library_version(void)
+{
+static char polar_version[30];
+unsigned int pv = version_get_number();
+sprintf( polar_version, "PolarSSL %d.%d.%d",
+   (pv>>24)&0xff, (pv>>16)&0xff, (pv>>8)&0xff );
+return polar_version;
+}
+
 #endif /* defined(ENABLE_SSL) && defined(ENABLE_CRYPTO_POLARSSL) */
-- 
1.8.3.2



pgpoIHLXI96xd.pgp
Description: PGP signature

Re: [Openvpn-devel] RFD: ssl library version numbers

[Gert Doering]

Hi,

On Tue, Apr 15, 2014 at 11:12:29AM +0200, M. Braun wrote:
> Am 15.04.2014 09:42, schrieb Gert Doering:> char *
> > get_ssl_library_version(void)
> > {
> > static char polar_version[30];
> > unsigned int pv = version_get_number();
> > sprintf( polar_version, "PolarSSL %d.%d.%d",
> > (pv>>24)&0xff, (pv>>16)&0xff, (pv>>8)&0xff );
> > return polar_version;
> > }
> >
> > this is well-defined (polarssl/version.h), and guaranteed to not overflow.
> 
> wouldn't be using snprintf with length sizeof(polar_version) instead of
> just sprintf be better as to make sure this will not be broken by
> accident in future?

How can this ever be broken?  0xff bounds the numbers to 3 digits, 
there are only 3 numbers in there, which adds up to 21 bytes, at
maximum.

Now if someone changes that code to add more text, it might...

(but I actually tend to go back to the previous version, as PolarSSL
guarantees that they won't overrun these "magic 18 bytes", as that is
"2 digits for each number", and if they ever add more text to the
string, they will introduce a new API which is more sane than this - had
a side-track discussion with Paul and David on it, of which David missed
most due to his MTA bouncing all mails...)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpsypimxX66R.pgp
Description: PGP signature

Re: [Openvpn-devel] RFD: ssl library version numbers

[Gert Doering]

HI,

On Mon, Apr 14, 2014 at 10:18:51PM +0200, David Sommerseth wrote:
> Regarding your patch - the PolarSSL stuff
> 
> +char *
> +get_ssl_library_version(void)
> +{
> +static char polar_version[30];   /* "at least 18 bytes in size" */
> +version_get_string_full( polar_version );
> +return polar_version;
> +}
> 
> version_get_string_full() is crappy due to the "at least 18 bytes".
> That's ugly.  Can we please ensure that we at least adds a NULL
> termination in polar_version[29] after version_get_string_full() ...
> just to reduce the risk somewhat, in case it suddenly gets longer.  Then
> it should (hopefully) be somewhat harder to get a buffer information leak.

Different approach:

char *
get_ssl_library_version(void)
{
static char polar_version[30];
unsigned int pv = version_get_number();
sprintf( polar_version, "PolarSSL %d.%d.%d",
(pv>>24)&0xff, (pv>>16)&0xff, (pv>>8)&0xff );
return polar_version;
}

this is well-defined (polarssl/version.h), and guaranteed to not overflow.

> Or maybe even slightly more hardened.  Put version_get_string_full()
> into a much bigger temporary buffer, which then uses a strncpy() or
> memcpy() into the polar_version[] buffer.  The strncpy()/memcpy() should
> restrict it to 30 bytes, including NULL terminator.

"much bigger", right :-)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpZITQqy8ZSy.pgp
Description: PGP signature

Re: [Openvpn-devel] RFD: ssl library version numbers

[David Sommerseth]

On 14/04/14 21:34, Gert Doering wrote:
> Hi,
> 
> On Sun, Apr 13, 2014 at 05:26:13PM +0200, Gert Doering wrote:
>> OpenVPN does not currently report the version of the SSL library it is
>> using - which I'm not sure whether it's by design or just because nobody
>> ever added it.  Anyway, right now I think we need it, to help future
>> cases.
> 
> OK, here's a "continue the discussion" patch.
> 
> This is actually surprisingly messy, as the "first line" OpenVPN prints
> is a static string that is referenced all over the place, sometimes sent
> to file descriptors, sometimes used with msg(), so extending this to
> include the "library version numbers" would be a fairly big change.  So
> I've decided to just print a second line:

Makes sense!

> 
> If --push-peer-info is set (and only then), this is sent as part of the
> peer-info handshake towards the server:
> 
> Apr 14 21:23:49 gentoo openvpn[17804]: 2001:608:4:0:222:68ff:fe7f:7420 peer 
> info: IV_SSL=OpenSSL_1.0.1g_7_Apr_2014

Looks good.

Regarding your patch - the PolarSSL stuff

+char *
+get_ssl_library_version(void)
+{
+static char polar_version[30]; /* "at least 18 bytes in size" */
+version_get_string_full( polar_version );
+return polar_version;
+}

version_get_string_full() is crappy due to the "at least 18 bytes".
That's ugly.  Can we please ensure that we at least adds a NULL
termination in polar_version[29] after version_get_string_full() ...
just to reduce the risk somewhat, in case it suddenly gets longer.  Then
it should (hopefully) be somewhat harder to get a buffer information leak.

Or maybe even slightly more hardened.  Put version_get_string_full()
into a much bigger temporary buffer, which then uses a strncpy() or
memcpy() into the polar_version[] buffer.  The strncpy()/memcpy() should
restrict it to 30 bytes, including NULL terminator.

I presume get_ssl_library_version() will be treated as a normal NULL
terminated string elsewhere in the code, also later on.


-- 
kind regards,

David Sommerseth



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] RFD: ssl library version numbers

[Gert Doering]

Hi,

On Mon, Apr 14, 2014 at 09:12:05AM +0200, Jan Just Keijser wrote:
> >- how do I get the library version for PolarSSL?
> void version_get_string_full( char *string )

I really wonder what they smoked when designing this API...

/**
 * Get the full version string ("PolarSSL x.y.z").
 *
 * \param stringThe string that will receive the value.
 *  (Should be at least 18 bytes in size)
 */
void version_get_string_full( char *string );


"pass me a pointer to a string, don't pass me a length field, and there's
no real way you can be sure whatever you use is long enough"...

Anyway.  Patch coming.

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpZ4ptm_0GSN.pgp
Description: PGP signature

Re: [Openvpn-devel] RFD: ssl library version numbers

[Steffan Karger]

Hi,

On 14-04-14 09:12, Jan Just Keijser wrote:
> Gert Doering wrote:
>> - if we report it, do we want to report it always (as IV_VER) or only
>>   if --push-peer-info is set?
>>   
> we're reporting the openvpn version info anyway, so adding the SSL lib 
> version would not change much; if it is only returned when 
> --push-peer-info is set then there shouldn't be any privacy/security 
> concerns, esp if the info is given *AFTER* the initial connection is 
> made (i.e. after the first certificate handshake).

My thoughts exactly. I think this is useful, but I do not want to tell
an eavesdropper whether I'm running a vulnerable SSL library. So, this
should really happen *after* the peer authentication, over a secure channel.

-Steffan

Re: [Openvpn-devel] RFD: ssl library version numbers

[David Sommerseth]

- Opprinnelig melding -
> Fra: "Gert Doering" <g...@greenie.muc.de>
> Til: openvpn-devel@lists.sourceforge.net
> Sendt: 13. april 2014 17:26:13
> Emne: [Openvpn-devel] RFD: ssl library version numbers
> 
> Hi,
> 
> OpenVPN does not currently report the version of the SSL library it is
> using - which I'm not sure whether it's by design or just because nobody
> ever added it.  Anyway, right now I think we need it, to help future
> cases.
> 
> There are a few questions that go along with that, which I want to discuss
> here :-)
> 
> - shall we report compile-time versions as well, or only run-time version?

I'd say we should return *run-time* versions.  For RPM based distros at
least, the OpenSSL library can be upgraded without a rebuild of OpenVPN
(as long as the ABI doesn't change, which most enterprise level distros
will beware of).  In that perspective there is not much value which
compile-time version OpenVPN was compiled against.

The only time OpenSSL compile-time versions is interesting is when there
are #define constants and macros which would directly change the OpenVPN
runtime behaviour.  But such changes is actually breaking the OpenSSL ABI,
which again most enterprise level distros will beware of and avoid.  And
I even believe, at least for OpenSSL, that such ABI breaking changes goes
into their major releases, not the minor updates.


--
kind regards,

David Sommerseth

Re: [Openvpn-devel] RFD: ssl library version numbers

[Jan Just Keijser]

Hi Gert,

Gert Doering wrote:

Hi,

OpenVPN does not currently report the version of the SSL library it is
using - which I'm not sure whether it's by design or just because nobody
ever added it.  Anyway, right now I think we need it, to help future
cases.

There are a few questions that go along with that, which I want to discuss
here :-)

- shall we report compile-time versions as well, or only run-time version?

  Like:

OpenSSL compile version='OpenSSL 1.0.1f 6 Jan 2014'
library version='OpenSSL 1.0.1g 7 Apr 2014'

  (this is on one of my test systems where I discovered an old OpenSSL
  installation, and upgraded *after* I built the OpenVPN binary)

  While I always like seeing numbers, I think the compile-time version is
  not actually that useful - if the ABI is not compatible, it will break,
  and if it is, the library version is what is relevant.

  
+1 for this; I'd go for runtime versions only , although it might be an 
interesting debug flag to get the compile time version as well - it 
might aid in debugging cases where the ABI is supposed to be compatible, 
but isn't .

- how do I get the library version for PolarSSL?

  

void version_get_string_full( char *string )



- shall we report the library version to the server, e.g. in the form of

   IV_SSL=OpenSSL 1.0.1f
   IV_SSL=PolarSSL 1.2.8

  as a sysadmin on the server side, I'd welcome this ("show me what my
  users are running").  From a security geek side, I'm not sure whether
  there is potential for abuse, so "please give me your input"

  



- if we report it, do we want to report it always (as IV_VER) or only
  if --push-peer-info is set?

  
we're reporting the openvpn version info anyway, so adding the SSL lib 
version would not change much; if it is only returned when 
--push-peer-info is set then there shouldn't be any privacy/security 
concerns, esp if the info is given *AFTER* the initial connection is 
made (i.e. after the first certificate handshake).



while we're at it : if we're printing out (not reporting) the version 
number of the SSL lib, why not also print out the version numbers of the 
other libs

- zlib
- pkcs11-helper
- snappy?

this is what 'curl' does and it has saved me debugging time in some 
strange cases.


JM2CW,

JJK

Re: [Openvpn-devel] RFD: ssl library version numbers

[Eric Crist]

Love it. Report always 

> On Apr 13, 2014, at 10:26 AM, Gert Doering  wrote:
> 
> Hi,
> 
> OpenVPN does not currently report the version of the SSL library it is
> using - which I'm not sure whether it's by design or just because nobody
> ever added it.  Anyway, right now I think we need it, to help future
> cases.
> 
> There are a few questions that go along with that, which I want to discuss
> here :-)
> 
> - shall we report compile-time versions as well, or only run-time version?
> 
>  Like:
> 
>OpenSSL compile version='OpenSSL 1.0.1f 6 Jan 2014'
>library version='OpenSSL 1.0.1g 7 Apr 2014'
> 
>  (this is on one of my test systems where I discovered an old OpenSSL
>  installation, and upgraded *after* I built the OpenVPN binary)
> 
>  While I always like seeing numbers, I think the compile-time version is
>  not actually that useful - if the ABI is not compatible, it will break,
>  and if it is, the library version is what is relevant.
> 
> - how do I get the library version for PolarSSL?
> 
> - shall we report the library version to the server, e.g. in the form of
> 
>   IV_SSL=OpenSSL 1.0.1f
>   IV_SSL=PolarSSL 1.2.8
> 
>  as a sysadmin on the server side, I'd welcome this ("show me what my
>  users are running").  From a security geek side, I'm not sure whether
>  there is potential for abuse, so "please give me your input"
> 
> - if we report it, do we want to report it always (as IV_VER) or only
>  if --push-peer-info is set?
> 
> feedback, please! :-)
> 
> gert
> -- 
> USENET is *not* the non-clickable part of WWW!
>   //www.muc.de/~gert/
> Gert Doering - Munich, Germany g...@greenie.muc.de
> fax: +49-89-35655025g...@net.informatik.tu-muenchen.de
> --
> Put Bad Developers to Shame
> Dominate Development with Jenkins Continuous Integration
> Continuously Automate Build, Test & Deployment 
> Start a new project now. Try Jenkins in the cloud.
> http://p.sf.net/sfu/13600_Cloudbees
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] RFD: ssl library version numbers

[Gert Doering]

Hi,

OpenVPN does not currently report the version of the SSL library it is
using - which I'm not sure whether it's by design or just because nobody
ever added it.  Anyway, right now I think we need it, to help future
cases.

There are a few questions that go along with that, which I want to discuss
here :-)

- shall we report compile-time versions as well, or only run-time version?

  Like:

OpenSSL compile version='OpenSSL 1.0.1f 6 Jan 2014'
library version='OpenSSL 1.0.1g 7 Apr 2014'

  (this is on one of my test systems where I discovered an old OpenSSL
  installation, and upgraded *after* I built the OpenVPN binary)

  While I always like seeing numbers, I think the compile-time version is
  not actually that useful - if the ABI is not compatible, it will break,
  and if it is, the library version is what is relevant.

- how do I get the library version for PolarSSL?

- shall we report the library version to the server, e.g. in the form of

   IV_SSL=OpenSSL 1.0.1f
   IV_SSL=PolarSSL 1.2.8

  as a sysadmin on the server side, I'd welcome this ("show me what my
  users are running").  From a security geek side, I'm not sure whether
  there is potential for abuse, so "please give me your input"

- if we report it, do we want to report it always (as IV_VER) or only
  if --push-peer-info is set?

feedback, please! :-)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpeQ1Fh95jRc.pgp
Description: PGP signature