Re: [Openvpn-devel] [PATCH] Use stricter snprintf() formatting in socks_username_password_auth() (v2)

[Gert Doering]

Hi,

On Mon, Nov 15, 2010 at 09:50:46PM +0100, David Sommerseth wrote:
> +  || !creds.password || (strlen(creds.password) > 255) ) {
> +  msg (M_NONFATAL,
> +   "socks username and/or password exceeds 255 characters.  "
> +   "Authentiaction not possible.");

I'd ack this but for the spelling typo :-)

(Sorry, but, well, these things tend to stick...)

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


pgpzM_lF2BLDb.pgp
Description: PGP signature

Re: [Openvpn-devel] [PATCH 1/6] Use stricter snprintf() formatting in socks_username_password_auth()

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 15/11/10 16:05, Peter Stuge wrote:
> Gert Doering wrote:
>>> -  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", 
>>> strlen(creds.username),
>>> -creds.username, strlen(creds.password), creds.password);
>>> +  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", (int) 
>>> strlen(creds.username) & 0xff,
>>> +creds.username, (int) strlen(creds.password) & 0xff, 
>>> creds.password);
>>
>> I tend to NAK this.
> 
> I agree. My ack is only for the other patches. (Sorry, I should have
> written that in the other mail.) A better solution for this should be
> simple as Gert outlined.

I didn't think about this when I wrote this patch, but I completely
agree!  I'll improve that part.  It's also silly to truncate the %c part
and not limit the %s as %.255s in this case.

Thanks for catching that, I'll send a new patch fixing this one!


kind regards,

David Sommerseth


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

iEYEARECAAYFAkzhlJwACgkQDC186MBRfrpaPQCePSJrLAyNOysVGjG33NEGl46L
qG4An3npEuIjRry2EO+dfRnCO1BfiEuN
=dv5a
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH 1/6] Use stricter snprintf() formatting in socks_username_password_auth()

[Peter Stuge]

Gert Doering wrote:
> > -  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", 
> > strlen(creds.username),
> > -creds.username, strlen(creds.password), creds.password);
> > +  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", (int) 
> > strlen(creds.username) & 0xff,
> > +creds.username, (int) strlen(creds.password) & 0xff, 
> > creds.password);
> 
> I tend to NAK this.

I agree. My ack is only for the other patches. (Sorry, I should have
written that in the other mail.) A better solution for this should be
simple as Gert outlined.


//Peter


pgpav_Assz9cZ.pgp
Description: PGP signature

Re: [Openvpn-devel] [PATCH 1/6] Use stricter snprintf() formatting in socks_username_password_auth()

[Gert Doering]

Hi,

On Mon, Nov 15, 2010 at 09:05:43AM +0100, David Sommerseth wrote:
> -  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", 
> strlen(creds.username),
> -creds.username, strlen(creds.password), creds.password);
> +  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", (int) 
> strlen(creds.username) & 0xff,
> +creds.username, (int) strlen(creds.password) & 0xff, 
> creds.password);

I tend to NAK this.

I'm fine with the "(int)" thingie, but not with the "0xff" part - if
the creds.username/creds.password string is indeed longer than 255 bytes,
the whole message will be unparsable by the receiver, and silent truncation
of the length field is not helping to make the code more readable, nor
does it improve code quality.

So if you want to make sure that the length is <= 255, add a check + error
above the snprintf() block.

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


pgpBMc5i3TmC2.pgp
Description: PGP signature

[Openvpn-devel] [PATCH 5/6] Removed functions not being used anywhere

[David Sommerseth]

The GNU C compiler gave warnings about these functions in the patch
not being used anywhere.  Doing a git grep on the code turned out
there were no callers to these functions.  Taking these functions out,
as there is not good reason why to carry dead code.

Signed-off-by: David Sommerseth 
---
 ps.c  |   12 
 sig.c |9 -
 2 files changed, 0 insertions(+), 21 deletions(-)

diff --git a/ps.c b/ps.c
index 003fb45..0aaef62 100644
--- a/ps.c
+++ b/ps.c
@@ -234,18 +234,6 @@ port_share_sendmsg (const socket_descriptor_t sd,
 }
 }

-static int
-pc_list_len (struct proxy_connection *pc)
-{
-  int count = 0;
-  while (pc)
-{
-  ++count;
-  pc = pc->next;
-}
-  return count;
-}
-
 static void
 proxy_entry_close_sd (struct proxy_connection *pc, struct event_set *es)
 {
diff --git a/sig.c b/sig.c
index 4dd6b09..438f4e6 100644
--- a/sig.c
+++ b/sig.c
@@ -185,15 +185,6 @@ signal_handler (const int signum)
   signal (signum, signal_handler);
 }

-/* temporary signal handler, before we are fully initialized */
-static void
-signal_handler_exit (const int signum)
-{
-  msg (M_FATAL,
-   "Signal %d (%s) received during initialization, exiting",
-   signum, signal_description (signum, NULL));
-}
-
 #endif

 /* set handlers for unix signals */
-- 
1.7.2.3

[Openvpn-devel] [PATCH 0/6] GNU C compiler warning clean-up

[David Sommerseth]

The GNU C compiler gives a lot of different warnings when compiling
with -Wall set.  This patch-set tries to clean up all these issues,
hopefully without breaking anything for other compilers - especially
on the Windows platform.


David Sommerseth (6):
  Use stricter snprintf() formatting in socks_username_password_auth()
  Fix compiler warnings about not used dummy() functions
  Fixed potential misinterpretation of boolean logic
  Only add some functions when really needed
  Removed functions not being used anywhere
  Merged add_bypass_address() and add_host_route_if_nonlocal()

 cryptoapi.c |2 ++
 ieproxy.c   |3 ++-
 options.c   |2 ++
 perf.c  |2 ++
 pkcs11.c|2 ++
 ps.c|   12 
 reliable.c  |2 ++
 route.c |   34 --
 sig.c   |9 -
 socks.c |4 ++--
 ssl.c   |2 +-
 11 files changed, 27 insertions(+), 47 deletions(-)

-- 
1.7.2.3

[Openvpn-devel] [PATCH 1/6] Use stricter snprintf() formatting in socks_username_password_auth()

[David Sommerseth]

commit fc1fa9ffc7e3356458ec3 added a new function which needs to have a
stricter string formatting.  This was detected due to a compiler warning.

This patch makes sure that the %c parts in the string only gets 8 bits of data,
which seems to be the intension.  strlen() is used to determine the values 
passed
to the %c places in, but it was not taken into consideration that size_t which
strlen() returns might not be the same as int or char.

Signed-off-by: David Sommerseth 
---
 socks.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/socks.c b/socks.c
index 58b3648..219accd 100644
--- a/socks.c
+++ b/socks.c
@@ -114,8 +114,8 @@ socks_username_password_auth (struct socks_proxy_info *p,
   creds.defined = 0;

   get_user_pass (, p->authfile, UP_TYPE_SOCKS, GET_USER_PASS_MANAGEMENT);
-  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", strlen(creds.username),
-creds.username, strlen(creds.password), creds.password);
+  snprintf (to_send, sizeof (to_send), "\x01%c%s%c%s", (int) 
strlen(creds.username) & 0xff,
+creds.username, (int) strlen(creds.password) & 0xff, 
creds.password);
   size = send (sd, to_send, strlen(to_send), MSG_NOSIGNAL);

   if (size != strlen (to_send))
-- 
1.7.2.3

[Openvpn-devel] [PATCH 3/6] Fixed potential misinterpretation of boolean logic

[David Sommerseth]

The GNU C compiler warned about a potential issue with an if()
expression missing an extra set of parentheses.

Signed-off-by: David Sommerseth 
---
 ssl.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ssl.c b/ssl.c
index c05d34f..8644ae4 100644
--- a/ssl.c
+++ b/ssl.c
@@ -940,7 +940,7 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
   if (opt->verify_export_cert)
 {
   gc = gc_new();
-  if (tmp_file=get_peer_cert(ctx, opt->verify_export_cert,))
+  if ((tmp_file=get_peer_cert(ctx, opt->verify_export_cert,)))
{
  setenv_str(opt->es, "peer_cert", tmp_file);
}
-- 
1.7.2.3

[Openvpn-devel] [PATCH 2/6] Fix compiler warnings about not used dummy() functions

[David Sommerseth]

It has been reported that the Microsoft Visual C compiler complains if
a .c file do not contain any compilable code, which can happen if the
code has been #ifdef'ed out.  To avoid this, these #ifdef sections have
a #else section which adds a static dummy() function which does nothing.

On the other hand, the GNU C compiler complains about unused functions when
it discovers this situation.

This patch tries to only add these dummy() functions if the Microsoft Visual C
compiler is detected, via the _MSC_VER macro.

Signed-off-by: David Sommerseth 
---
 cryptoapi.c |2 ++
 ieproxy.c   |3 ++-
 perf.c  |2 ++
 pkcs11.c|2 ++
 4 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/cryptoapi.c b/cryptoapi.c
index 8fb5387..3365cd7 100644
--- a/cryptoapi.c
+++ b/cryptoapi.c
@@ -470,5 +470,7 @@ int SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, 
const char *cert_prop)
 }

 #else
+#ifdef _MSC_VER  /* Dummy function needed to avoid empty file compiler warning 
in Microsoft VC */
 static void dummy (void) {}
+#endif
 #endif /* WIN32 */
diff --git a/ieproxy.c b/ieproxy.c
index 89977a8..3099870 100644
--- a/ieproxy.c
+++ b/ieproxy.c
@@ -139,7 +139,8 @@ LPCTSTR getIeHttpProxy()
 return(NULL);
   }
 }
-
 #else
+#ifdef _MSC_VER  /* Dummy function needed to avoid empty file compiler warning 
in Microsoft VC */
 static void dummy (void) {}
+#endif
 #endif /* WIN32 */
diff --git a/perf.c b/perf.c
index a149c07..67ea958 100644
--- a/perf.c
+++ b/perf.c
@@ -287,5 +287,7 @@ perf_print_state (int lev)
 }

 #else
+#ifdef _MSC_VER  /* Dummy function needed to avoid empty file compiler warning 
in Microsoft VC */
 static void dummy(void) {}
 #endif
+#endif
diff --git a/pkcs11.c b/pkcs11.c
index e06a2ed..d90ac96 100644
--- a/pkcs11.c
+++ b/pkcs11.c
@@ -982,5 +982,7 @@ cleanup:
 }

 #else
+#ifdef _MSC_VER  /* Dummy function needed to avoid empty file compiler warning 
in Microsoft VC */
 static void dummy (void) {}
+#endif
 #endif /* ENABLE_PKCS11 */
-- 
1.7.2.3

[Openvpn-devel] [PATCH 0/6] *** SUBJECT HERE ***

[David Sommerseth]

*** BLURB HERE ***

David Sommerseth (6):
  Use stricter snprintf() formatting in socks_username_password_auth()
  Fix compiler warnings about not used dummy() functions
  Fixed potential misinterpretation of boolean logic
  Only add some functions when really needed
  Removed functions not being used anywhere
  Merged add_bypass_address() and add_host_route_if_nonlocal()

 cryptoapi.c |2 ++
 ieproxy.c   |3 ++-
 options.c   |2 ++
 perf.c  |2 ++
 pkcs11.c|2 ++
 ps.c|   12 
 reliable.c  |2 ++
 route.c |   34 --
 sig.c   |9 -
 socks.c |4 ++--
 ssl.c   |2 +-
 11 files changed, 27 insertions(+), 47 deletions(-)

-- 
1.7.2.3

[Openvpn-devel] [PATCH 6/6] Merged add_bypass_address() and add_host_route_if_nonlocal()

[David Sommerseth]

The add_host_route_if_nonlocal() function is too simple to really
benefit from calling add_bypass_address() when this function is the
only caller to this function.

Signed-off-by: David Sommerseth 
---
 route.c |   34 --
 1 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/route.c b/route.c
index 612bcbe..f78893e 100644
--- a/route.c
+++ b/route.c
@@ -59,26 +59,6 @@ print_bypass_addresses (const struct route_bypass *rb)

 #endif

-static bool
-add_bypass_address (struct route_bypass *rb, const in_addr_t a)
-{
-  int i;
-  for (i = 0; i < rb->n_bypass; ++i)
-{
-  if (a == rb->bypass[i]) /* avoid duplicates */
-   return true;
-}
-  if (rb->n_bypass < N_ROUTE_BYPASS)
-{
-  rb->bypass[rb->n_bypass++] = a;
-  return true;
-}
-  else
-{
-  return false;
-}
-}
-
 struct route_option_list *
 new_route_option_list (const int max_routes, struct gc_arena *a)
 {
@@ -2124,8 +2104,18 @@ netmask_to_netbits (const in_addr_t network, const 
in_addr_t netmask, int *netbi
 static void
 add_host_route_if_nonlocal (struct route_bypass *rb, const in_addr_t addr)
 {
-  if (test_local_addr(addr) == TLA_NONLOCAL && addr != 0 && addr != ~0)
-add_bypass_address (rb, addr);
+  if (test_local_addr(addr) == TLA_NONLOCAL && addr != 0 && addr != ~0) {
+int i;
+for (i = 0; i < rb->n_bypass; ++i)
+  {
+if (addr == rb->bypass[i]) /* avoid duplicates */
+  return;
+  }
+if (rb->n_bypass < N_ROUTE_BYPASS)
+  {
+rb->bypass[rb->n_bypass++] = addr;
+  }
+  }
 }

 static void
-- 
1.7.2.3

[Openvpn-devel] [PATCH 4/6] Only add some functions when really needed

[David Sommerseth]

The GNU C compiler gave warnings about some functions not being used.
These functions where only used if certian #ifdef sections was enabled.

This patch encapsulates these function declarations with matching #ifdef's
to make it more clear when these functions are needed.

Signed-off-by: David Sommerseth 
---
 options.c  |2 ++
 reliable.c |2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/options.c b/options.c
index f0b9310..d0cec7d 100644
--- a/options.c
+++ b/options.c
@@ -2785,6 +2785,7 @@ positive_atoi (const char *str)
   return i < 0 ? 0 : i;
 }

+#ifdef WIN32  /* This function is only used when compiling on Windows */
 static unsigned int
 atou (const char *str)
 {
@@ -2792,6 +2793,7 @@ atou (const char *str)
   sscanf (str, "%u", );
   return val;
 }
+#endif

 static inline bool
 space (unsigned char c)
diff --git a/reliable.c b/reliable.c
index a41f2bf..e68c236 100644
--- a/reliable.c
+++ b/reliable.c
@@ -516,6 +516,7 @@ reliable_can_send (const struct reliable *rel)
   return n_current > 0 && !rel->hold;
 }

+#ifdef EXPONENTIAL_BACKOFF
 /* return a unique point-in-time to trigger retry */
 static time_t
 reliable_unique_retry (struct reliable *rel, time_t retry)
@@ -535,6 +536,7 @@ reliable_unique_retry (struct reliable *rel, time_t retry)
 }
   return retry;
 }
+#endif

 /* return next buffer to send to remote */
 struct buffer *
-- 
1.7.2.3