diff mbox series

[Openvpn-devel,v1] Change --reneg-bytes and --reneg-packets to 64 bit counters

Message ID 20241111094033.16073-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel,v1] Change --reneg-bytes and --reneg-packets to 64 bit counters |

Commit Message

Gert Doering Nov. 11, 2024, 9:40 a.m. UTC 
  From: Arne Schwabe <arne@rfc2549.org>

reneg-bytes can currently only specify up to a maximum of 2GB.
This makes it even problematic to use without extended counters.

Change-Id: I993e7fc5609955d271e74370affc2eea340a1e2d
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/795
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Antonio Quartulli Nov. 11, 2024, 1:31 p.m. UTC | #1 
On 11/11/2024 10:40, Gert Doering wrote:
> From: Arne Schwabe <arne@rfc2549.org>
> 
> reneg-bytes can currently only specify up to a maximum of 2GB.
> This makes it even problematic to use without extended counters.
> 
> Change-Id: I993e7fc5609955d271e74370affc2eea340a1e2d
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> Acked-by: Gert Doering <gert@greenie.muc.de>
> ---
> 
> This change was reviewed on Gerrit and approved by at least one
> developer. I request to merge it to master.
> 
> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/795
> This mail reflects revision 1 of this Change.
> 
> Acked-by according to Gerrit (reflected above):
> Gert Doering <gert@greenie.muc.de>
> 
>          
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 1beb0ee..10ee9f6 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -2032,8 +2032,8 @@
>   
>       SHOW_INT(tls_timeout);
>   
> -    SHOW_INT(renegotiate_bytes);
> -    SHOW_INT(renegotiate_packets);
> +    SHOW_INT64(renegotiate_bytes);
> +    SHOW_INT64(renegotiate_packets);
>       SHOW_INT(renegotiate_seconds);
>   
>       SHOW_INT(handshake_window);
> @@ -9187,12 +9187,26 @@
>       else if (streq(p[0], "reneg-bytes") && p[1] && !p[2])
>       {
>           VERIFY_PERMISSION(OPT_P_TLS_PARMS);
> -        options->renegotiate_bytes = positive_atoi(p[1]);
> +        char *end;
> +        long long reneg_bytes = strtoll(p[1], &end, 10);

why int64_t in options.h and long long here?
I think they should always match in size, but why not use it consistently?

> +        if (*end != '\0' || reneg_bytes < 0)

Shouldn't also check for "reneg_bytes != LLONG_MAX" ? LLONG_MAX is 
returned in case of overflow.

Regards,

> +        {
> +            msg(msglevel, "--reneg-bytes parameter must be an integer and >= 0");
> +            goto err;
> +        }
> +        options->renegotiate_bytes = reneg_bytes;
>       }
>       else if (streq(p[0], "reneg-pkts") && p[1] && !p[2])
>       {
>           VERIFY_PERMISSION(OPT_P_TLS_PARMS);
> -        options->renegotiate_packets = positive_atoi(p[1]);
> +        char *end;
> +        long long pkt_max = strtoll(p[1], &end, 10);
> +        if (*end != '\0' || pkt_max < 0)

same here.

> +        {
> +            msg(msglevel, "--reneg-pkts parameter must be an integer and >= 0");
> +            goto err;
> +        }
> +        options->renegotiate_packets = pkt_max;
>       }
>       else if (streq(p[0], "reneg-sec") && p[1] && !p[3])
>       {
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index ee39dbb..6ab92e2 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -626,8 +626,8 @@
>       int tls_timeout;
>   
>       /* Data channel key renegotiation parameters */
> -    int renegotiate_bytes;
> -    int renegotiate_packets;
> +    int64_t renegotiate_bytes;
> +    int64_t renegotiate_packets;
>       int renegotiate_seconds;
>       int renegotiate_seconds_min;
>   
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index c48a85c..ab55365 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -118,7 +118,7 @@
>    *                      May *not* be NULL.
>    */
>   static void
> -tls_limit_reneg_bytes(const char *ciphername, int *reneg_bytes)
> +tls_limit_reneg_bytes(const char *ciphername, int64_t *reneg_bytes)
>   {
>       if (cipher_kt_insecure(ciphername))
>       {
> @@ -3028,7 +3028,7 @@
>           && should_trigger_renegotiation(session, ks))
>      {
>           msg(D_TLS_DEBUG_LOW, "TLS: soft reset sec=%d/%d bytes=" counter_format
> -            "/%d pkts=" counter_format "/%d",
> +            "/%" PRIi64 " pkts=" counter_format "/%" PRIi64,
>               (int) (now - ks->established), session->opt->renegotiate_seconds,
>               ks->n_bytes, session->opt->renegotiate_bytes,
>               ks->n_packets, session->opt->renegotiate_packets);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 5bc2f2a..5840e2d 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -331,8 +331,8 @@
>       int transition_window;
>       int handshake_window;
>       interval_t packet_timeout;
> -    int renegotiate_bytes;
> -    int renegotiate_packets;
> +    int64_t renegotiate_bytes;
> +    int64_t renegotiate_packets;
>       interval_t renegotiate_seconds;
>   
>       /* cert verification parms */
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering Nov. 11, 2024, 1:38 p.m. UTC | #2 
Hi,

On Mon, Nov 11, 2024 at 02:31:48PM +0100, Antonio Quartulli wrote:
> > -        options->renegotiate_bytes = positive_atoi(p[1]);
> > +        char *end;
> > +        long long reneg_bytes = strtoll(p[1], &end, 10);
> 
> why int64_t in options.h and long long here?
> I think they should always match in size, but why not use it consistently?

Because strtoll() is documented to return "long long"...

gert
Gert Doering Nov. 11, 2024, 2:05 p.m. UTC | #3 
Stared-at-code, and gave it a quick test with "--reneg-bytes 20000" again
(on FreeBSD/amd64).  BB has also tested this on NetBSD/i386, so 32bit 
systems are also fine.  Testing with 2^65 leads to

2024-11-11 15:03:54 TLS: soft reset sec=30/30 bytes=578818/9223372036854775807 pkts=550/0

.. which should be good enough in terms of error handling, I think
(it's truncating to 2^63-1).  We can revisit this if this is a relevant
distinction for "number of packets until we need to renegotiate".

Your patch has been applied to the master branch.

commit d52ea247d9dec5262a09f3891db83b79b2ca403e
Author: Arne Schwabe
Date:   Mon Nov 11 10:40:33 2024 +0100

     Change --reneg-bytes and --reneg-packets to 64 bit counters

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20241111094033.16073-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg29744.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 1beb0ee..10ee9f6 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2032,8 +2032,8 @@ 
 
     SHOW_INT(tls_timeout);
 
-    SHOW_INT(renegotiate_bytes);
-    SHOW_INT(renegotiate_packets);
+    SHOW_INT64(renegotiate_bytes);
+    SHOW_INT64(renegotiate_packets);
     SHOW_INT(renegotiate_seconds);
 
     SHOW_INT(handshake_window);
@@ -9187,12 +9187,26 @@ 
     else if (streq(p[0], "reneg-bytes") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_TLS_PARMS);
-        options->renegotiate_bytes = positive_atoi(p[1]);
+        char *end;
+        long long reneg_bytes = strtoll(p[1], &end, 10);
+        if (*end != '\0' || reneg_bytes < 0)
+        {
+            msg(msglevel, "--reneg-bytes parameter must be an integer and >= 0");
+            goto err;
+        }
+        options->renegotiate_bytes = reneg_bytes;
     }
     else if (streq(p[0], "reneg-pkts") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_TLS_PARMS);
-        options->renegotiate_packets = positive_atoi(p[1]);
+        char *end;
+        long long pkt_max = strtoll(p[1], &end, 10);
+        if (*end != '\0' || pkt_max < 0)
+        {
+            msg(msglevel, "--reneg-pkts parameter must be an integer and >= 0");
+            goto err;
+        }
+        options->renegotiate_packets = pkt_max;
     }
     else if (streq(p[0], "reneg-sec") && p[1] && !p[3])
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index ee39dbb..6ab92e2 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -626,8 +626,8 @@ 
     int tls_timeout;
 
     /* Data channel key renegotiation parameters */
-    int renegotiate_bytes;
-    int renegotiate_packets;
+    int64_t renegotiate_bytes;
+    int64_t renegotiate_packets;
     int renegotiate_seconds;
     int renegotiate_seconds_min;
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index c48a85c..ab55365 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -118,7 +118,7 @@ 
  *                      May *not* be NULL.
  */
 static void
-tls_limit_reneg_bytes(const char *ciphername, int *reneg_bytes)
+tls_limit_reneg_bytes(const char *ciphername, int64_t *reneg_bytes)
 {
     if (cipher_kt_insecure(ciphername))
     {
@@ -3028,7 +3028,7 @@ 
         && should_trigger_renegotiation(session, ks))
    {
         msg(D_TLS_DEBUG_LOW, "TLS: soft reset sec=%d/%d bytes=" counter_format
-            "/%d pkts=" counter_format "/%d",
+            "/%" PRIi64 " pkts=" counter_format "/%" PRIi64,
             (int) (now - ks->established), session->opt->renegotiate_seconds,
             ks->n_bytes, session->opt->renegotiate_bytes,
             ks->n_packets, session->opt->renegotiate_packets);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 5bc2f2a..5840e2d 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -331,8 +331,8 @@ 
     int transition_window;
     int handshake_window;
     interval_t packet_timeout;
-    int renegotiate_bytes;
-    int renegotiate_packets;
+    int64_t renegotiate_bytes;
+    int64_t renegotiate_packets;
     interval_t renegotiate_seconds;
 
     /* cert verification parms */