Re: [Openvpn-devel] [PATCHv2 1/2] Get NTLMv1 and NTLMv2 up and running

[Steffan Karger]

Hi Holgert,

Apologies for taking so long to respond.  This patch came up again at 
the hackathon last weekend.  Since we can not thoroughly test this 
ourselves, but the patch is coming from a known source and you report 
successful tests, we decided that 'stare at code' should suffice. 
Hereby the results of my staring.


First, ntlm.c was a type mess before your patch, and still is after. 
I'll not comment on that any further, but will send a follow-up patch to 
fix it later on.  It would be highly appreciated if you could give that 
patch a test spin for me.


On 04-07-14 09:35, Holger Kummert wrote:

+  const char trailingBytesForUTF8[256] = {
+  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+  1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1, 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,
+  2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2, 3,3,3,3,3,3,3,3,4,4,4,4,5,5,5,5
+  };


Not sure if the compiler will actually put this on the stack, but this 
can be static const.



+if (ch >= UNI_SUR_HIGH_START && ch <= UNI_SUR_LOW_END) {
+  msg (M_INFO, "Warning: Illegal character value: %x is in reserved area of 
UTF-16 [%x, %x]", UNI_SUR_HIGH_START, UNI_SUR_LOW_END);
+  return -1;
+}


This msg() call is missing an argument (I think 'ch').


  static void
-add_security_buffer(int sb_offset, void *data, int length, unsigned char 
*msg_buf, int *msg_bufpos)
+add_security_buffer(int sb_offset, void *data, int length, unsigned char 
*msg_buf, int *msg_bufpos, int msg_buflen)
  {
+  if ((*msg_bufpos - *msg_buf) + length > msg_buflen) {
+msg (M_INFO, "Warning: Not enough space in destination buffer");
+return;
+  }
+


I don't think this is what you intended, or does *msg_buf contain some 
magic length field?  You probably want something like:


  ASSERT (length > 0 && *msg_bufpos > 0 && msg_buflen > 0);
  if (msg_buflen - length < msg_bufpos) { /* error */ }

That should be an integer-overflow safe overflow check.

Since we're also writing to sb_offset to sb_offset+5, we should also add 
a check on sb_offset:


  if (msg_buflen - 5 >= sb_offset) { /* error */ }

(And wow, this function was/is absolutely terrifying.  It seems like a 
miracle that this code ever worked...)


-Steffan

[Openvpn-devel] [PATCH v3] Notify clients about server's exit/restart

[Lev Stipakov]

When server exits / restarts (gets SIGUSR1, SIGTERM, SIGHUP, SIGINT) and
explicit-exit-notify is set, server sends RESTART control channel command to
all clients and reschedules received signal in 2 secs.

When client receives RESTART command, it either reconnects to the same server or
advances to the new one, depends on parameter comes with RESTART
command - behavior is controlled by explicit-exit-notify in the server config.

v3:
 - Use control channel "RESTART" command instead of new OCC code to
notify clients
 - Configure on the server side (by value of explicit-exit-notify) if
client should reconnect to the same server or advance to the next one
 - Fix compilation when OCC is disabled (--enable-small)
 - Update man page

v2:
 - Take into use explicit-exit-notify on the server side
 - OCC_SHUTTING_DOWN renamed to OCC_SERVER_EXIT
 - Code prettifying

Signed-off-by: Lev Stipakov 
---
 doc/openvpn.8 | 15 ++--
 src/openvpn/multi.c   | 68 ---
 src/openvpn/multi.h   |  9 +++
 src/openvpn/options.c |  9 +++
 src/openvpn/push.c|  6 +
 5 files changed, 96 insertions(+), 11 deletions(-)

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index e213f5a..0a00dec 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3829,8 +3829,19 @@ option will tell the server to immediately close its 
client instance object
 rather than waiting for a timeout.  The
 .B n
 parameter (default=1) controls the maximum number of attempts that the client
-will try to resend the exit notification message.  OpenVPN will not send any 
exit
-notifications unless this option is enabled.
+will try to resend the exit notification message. 
+
+In UDP server mode, send RESTART control channel command to connected clients. 
The
+.B n
+parameter (default=1) controls client behavior. With
+.B n
+= 1 client will attempt to reconnect
+to the same server, with
+.B n
+= 2 client will advance to the next server.
+
+OpenVPN will not send any exit
+notifications unless this option is enabled. 
 .\"*
 .SS Data Channel Encryption Options:
 These options are meaningful for both Static & TLS-negotiated key modes
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 902c4dc..d16b145 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -396,6 +396,8 @@ multi_init (struct multi_context *m, struct context *t, 
bool tcp_mode, int threa
 t->options.stale_routes_check_interval, 
t->options.stale_routes_ageing_time);
   event_timeout_init (>stale_routes_check_et, 
t->options.stale_routes_check_interval, 0);
 }
+
+  m->deferred_shutdown_signal.signal_received = 0;
 }

 const char *
@@ -2572,10 +2574,18 @@ multi_process_timeout (struct multi_context *m, const 
unsigned int mpp_flags)
   /* instance marked for wakeup? */
   if (m->earliest_wakeup)
 {
-  set_prefix (m->earliest_wakeup);
-  ret = multi_process_post (m, m->earliest_wakeup, mpp_flags);
+  if (m->earliest_wakeup == (struct 
multi_instance*)>deferred_shutdown_signal)
+   {
+ schedule_remove_entry(m->schedule, (struct schedule_entry*) 
>deferred_shutdown_signal);
+ throw_signal(m->deferred_shutdown_signal.signal_received);
+   }
+  else
+   {
+ set_prefix (m->earliest_wakeup);
+ ret = multi_process_post (m, m->earliest_wakeup, mpp_flags);
+ clear_prefix ();
+   }
   m->earliest_wakeup = NULL;
-  clear_prefix ();
 }
   return ret;
 }
@@ -2700,6 +2710,48 @@ multi_top_free (struct multi_context *m)
   free_context_buffers (m->top.c2.buffers);
 }

+static bool
+is_exit_restart(int sig)
+{
+  return (sig == SIGUSR1 || sig == SIGTERM || sig == SIGHUP || sig == SIGINT);
+}
+
+static void
+multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
+{
+  struct hash_iterator hi;
+  struct hash_element *he;
+  struct timeval tv;
+
+  /* tell all clients to restart */
+  hash_iterator_init (m->iter, );
+  while ((he = hash_iterator_next ()))
+{
+  struct multi_instance *mi = (struct multi_instance *) he->value;
+  if (!mi->halt)
+{
+ send_control_channel_string (>context, next_server ? 
"RESTART,[N]" : "RESTART", D_PUSH);
+ multi_schedule_context_wakeup(m, mi);
+}
+}
+  hash_iterator_free ();
+
+  /* reschedule signal */
+  ASSERT (!openvpn_gettimeofday (>deferred_shutdown_signal.wakeup, NULL));
+  tv.tv_sec = 2;
+  tv.tv_usec = 0;
+  tv_add (>deferred_shutdown_signal.wakeup, );
+
+  m->deferred_shutdown_signal.signal_received = m->top.sig->signal_received;
+
+  schedule_add_entry (m->schedule,
+ (struct schedule_entry *) >deferred_shutdown_signal,
+ >deferred_shutdown_signal.wakeup,
+ compute_wakeup_sigma 
(>deferred_shutdown_signal.wakeup));
+
+  m->top.sig->signal_received = 0;
+}
+
 /*
  * Return true if event loop should break,