[Openvpn-devel] Avoid overflow in wakeup time computation

Message ID 1520316568-8983-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series
  • [Openvpn-devel] Avoid overflow in wakeup time computation
Related show

Commit Message

Selva Nair March 6, 2018, 6:09 a.m.
From: Selva Nair <selva.nair@gmail.com>

Time interval arithmetic can overflow especially when user
defined intervals are involved. E.g., see Trac #922.

Avoid this by reordering the arithmetic operation in
event_timeout_trigger(). Also avoid unnecessary casting of time
variable to int.

Time until wakeup is now calculated like:

time_t wakeup = (last - now) + delay

Here delay is of type int, but is +ve by construction. Time backtrack
protection in OpenVPN ensures (last - now) <= 0. Then the above
expression cannot overflow (provided time_t is at least as large
as int).

A similar expression in interval.h is also changed.

(This patch grew out of patch 168 by Steffan Karger.)

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/interval.c | 8 +++++---
 src/openvpn/interval.h | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

Comments

Steffan Karger March 6, 2018, 8:27 p.m. | #1
Hi,

On 06-03-18 07:09, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Time interval arithmetic can overflow especially when user
> defined intervals are involved. E.g., see Trac #922.
> 
> Avoid this by reordering the arithmetic operation in
> event_timeout_trigger(). Also avoid unnecessary casting of time
> variable to int.
> 
> Time until wakeup is now calculated like:
> 
> time_t wakeup = (last - now) + delay
> 
> Here delay is of type int, but is +ve by construction. Time backtrack
> protection in OpenVPN ensures (last - now) <= 0. Then the above
> expression cannot overflow (provided time_t is at least as large
> as int).
> 
> A similar expression in interval.h is also changed.
> 
> (This patch grew out of patch 168 by Steffan Karger.)
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>  src/openvpn/interval.c | 8 +++++---
>  src/openvpn/interval.h | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/openvpn/interval.c b/src/openvpn/interval.c
> index 00ee627..b728560 100644
> --- a/src/openvpn/interval.c
> +++ b/src/openvpn/interval.c
> @@ -51,11 +51,12 @@ event_timeout_trigger(struct event_timeout *et,
>  
>      if (et->defined)
>      {
> -        int wakeup = (int) et->last + et->n - local_now;
> +        time_t wakeup = et->last - local_now + et->n;

I would have preferred braces to not have any doubt about associativity,
but can live with this.  (I always forget, so had to look up what C did
again.)

>          if (wakeup <= 0)
>          {
>  #if INTERVAL_DEBUG
> -            dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n, et_const_retry);
> +            dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n,
> +                 et_const_retry);
>  #endif
>              if (et_const_retry < 0)
>              {
> @@ -72,7 +73,8 @@ event_timeout_trigger(struct event_timeout *et,
>          if (tv && wakeup < tv->tv_sec)
>          {
>  #if INTERVAL_DEBUG
> -            dmsg(D_INTERVAL, "EVENT event_timeout_wakeup (%d/%d) etcr=%d", wakeup, et->n, et_const_retry);
> +            dmsg(D_INTERVAL, "EVENT event_timeout_wakeup (%d/%d) etcr=%d",
> +                 (int) wakeup, et->n, et_const_retry);
>  #endif
>              tv->tv_sec = wakeup;
>              tv->tv_usec = 0;
> diff --git a/src/openvpn/interval.h b/src/openvpn/interval.h
> index 826a08b..5623f3a 100644
> --- a/src/openvpn/interval.h
> +++ b/src/openvpn/interval.h
> @@ -196,7 +196,7 @@ event_timeout_modify_wakeup(struct event_timeout *et, interval_t n)
>  static inline interval_t
>  event_timeout_remaining(struct event_timeout *et)
>  {
> -    return (int) et->last + et->n - now;
> +    return (interval_t) (et->last - now + et->n);
>  }
>  
>  /*
> 

As discussed, an elegant way of solving the issue (verified manually
with -fsanitize=undefined).

Acked-by: Steffan Karger <steffan@karger.me>

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering March 24, 2018, 4:45 p.m. | #2
Your patch has been applied to the master and release/2.4 branch (bugfix).

Tested by running with the config from #922, which leads to

./../../openvpn/src/openvpn/interval.c:54:37: runtime error: signed integer overflow: 1521909676 + 1000000000 cannot be represented in type 'int'

without the patch, and no error with it (Linux/amd64).

commit f158c0e1df13ae1b697cdc7f189ddd1575a0c1aa (master)
commit 6d16c87de38610b05ec768af28a76a9389791134 (release/2.4)
Author: Selva Nair
Date:   Tue Mar 6 01:09:28 2018 -0500

     Avoid overflow in wakeup time computation

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Steffan Karger <steffan.karger@fox-it.com>
     Message-Id: <1520316568-8983-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16634.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/src/openvpn/interval.c b/src/openvpn/interval.c
index 00ee627..b728560 100644
--- a/src/openvpn/interval.c
+++ b/src/openvpn/interval.c
@@ -51,11 +51,12 @@  event_timeout_trigger(struct event_timeout *et,
 
     if (et->defined)
     {
-        int wakeup = (int) et->last + et->n - local_now;
+        time_t wakeup = et->last - local_now + et->n;
         if (wakeup <= 0)
         {
 #if INTERVAL_DEBUG
-            dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n, et_const_retry);
+            dmsg(D_INTERVAL, "EVENT event_timeout_trigger (%d) etcr=%d", et->n,
+                 et_const_retry);
 #endif
             if (et_const_retry < 0)
             {
@@ -72,7 +73,8 @@  event_timeout_trigger(struct event_timeout *et,
         if (tv && wakeup < tv->tv_sec)
         {
 #if INTERVAL_DEBUG
-            dmsg(D_INTERVAL, "EVENT event_timeout_wakeup (%d/%d) etcr=%d", wakeup, et->n, et_const_retry);
+            dmsg(D_INTERVAL, "EVENT event_timeout_wakeup (%d/%d) etcr=%d",
+                 (int) wakeup, et->n, et_const_retry);
 #endif
             tv->tv_sec = wakeup;
             tv->tv_usec = 0;
diff --git a/src/openvpn/interval.h b/src/openvpn/interval.h
index 826a08b..5623f3a 100644
--- a/src/openvpn/interval.h
+++ b/src/openvpn/interval.h
@@ -196,7 +196,7 @@  event_timeout_modify_wakeup(struct event_timeout *et, interval_t n)
 static inline interval_t
 event_timeout_remaining(struct event_timeout *et)
 {
-    return (int) et->last + et->n - now;
+    return (interval_t) (et->last - now + et->n);
 }
 
 /*