cppcheck finding: add "ASSERT( maxoutput > 0 || separator != NULL )" to prevent possible null pointer derefence

PATCH In review Ilya Shipitsin 5 participants 9 messages Sep 18, 2016 – Nov 28, 2016

Share & tools

Ilya Shipitsin

author

2016-09-18 14:19 UTC #0 lore raw

PATCH 1/1 cppcheck finding: add "ASSERT( maxoutput > 0 || separator != NULL )" to prevent possible null pointer derefence

---
 src/openvpn/buffer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

src/openvpn/buffer.c +4 -2

index 52c6ab9..57bded9 100644
@@ -438,10 +438,12 @@ format_hex_ex (const uint8_t *data, int size, int maxoutput,
 	       unsigned int space_break_flags, const char* separator,
 	       struct gc_arena *gc)
 {
-  struct buffer out = alloc_buf_gc (maxoutput ? maxoutput :
+  int i;
+  struct buffer out;
+  ASSERT( maxoutput > 0 || separator != NULL );
+  out = alloc_buf_gc (maxoutput ? maxoutput :
 				    ((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2),
 				    gc);
-  int i;
   for (i = 0; i < size; ++i)
     {
       if (separator && i && !(i % (space_break_flags & FHE_SPACE_BREAK_MASK)))

Signature · 1 line

2.5.5

David Sommerseth

2016-09-18 15:14 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 7 lines · Ilya Shipitsin

--- src/openvpn/buffer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
src/openvpn/buffer.c index renamed from src/openvpn/buffer.c +0 -0
52c6ab9..57bded9 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -438,10 +438,12 @@ format_hex_ex (const uint8_t *data, int size, int maxoutput, unsigned int

Quoted 9 lines

space_break_flags, const char* separator, struct gc_arena *gc) { - struct buffer out = alloc_buf_gc (maxoutput ? maxoutput : + int i;
+ struct buffer out; + ASSERT( maxoutput > 0 || separator != NULL ); + out = alloc_buf_gc (maxoutput ? maxoutput : ((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2), gc); - int i; for (i = 0; i < size; ++i) { if (separator && i && !(i % (space_break_flags & FHE_SPACE_BREAK_MASK)))

This looks dangerous and somewhat wrong. This can actually stop a server completely if the ASSERT() check fails. I'm not sure we want to do that.

The ASSERT() also expects maxoutput to be higher than 0, while the alloc_buf_gc() supports/allows maxoutput to be 0, which again confirms my bad feeling about this. If maxoutput == 0, alloc_buf_gc() will actually allocate a reasonable buffer regardless.

I do see that there might be an issue if separator == NULL (strlen(NULL) will segfault), so this needs to be checked far more carefully and compared against all the callers of format_hex_ex().

The question which pops up in my head is: Is this patch purely targeted to silence a code analyser warning? If so, this is most likely not the right fix for the OpenVPN code base.

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX3q9BAAoJEIbPlEyWcf3yGkwQAKKZbI2idz6a7h5ZkbQJCHMk JDNIpdR6XTFmG16jBrycYeew+9IRvfwpF4vh2/QPaRjDNLnKvPHP+9uEetqYmUQi rWzJyg3nzXSEUhFRSbdJcOK7zEWoiUUsSTiBKIYP6D5qD+QBQOZaQseCCzafrt5b IKrUesWMWPXAjQBYbUvcc801+JR5S/gSaSbk9TuO8Nemiry/GW/pSt0OVTggPdcm LiUyutQ8HXeXS9uAL32FLqFH3qq7QTHjxdVoKGGx9w/lmznqp+EaKCuANsWnt/PW ZTzVYM5YG5jI8XNM/wyCklu5n3yqJ37Noxd3ZRSD6ATTL/MExVgWINOOGA7f+Pj3 P6g4ZSc//zaWUCRpUKg4O8h6jaO+0stDS5eKDpyUCHYRHRdMI4IGLuwzIzKmxWq9 LpNGGY3Ikpf5IM+i2R04HctnNp+izEeW3vBCKnBBUHSyegRQGQ191Z5QC3SPk6VP HwxkHCZiOsie8WsP0Sl2dur1GjV9uyslVTkmt1GBVuoOGxpmZLJRJRayO70goZ17 ToavpadDjt193g79j1/dpE7OCPcCitso/+egxVzDWPTtLj+7rS/gfdJBNdCHr7xK Pf8KKbpD7yEbEln0K7wzsjVG749qZe103KmrhZulvu3H5+SrbT5ALhroaYidhRpg lLcx4g4A+nZjM6kabBGy
=QvxV
-----END PGP SIGNATURE-----

Gert Doering

2016-09-18 15:44 UTC #2 lore raw

Hi,

Quoted 3 lines · David Sommerseth

This looks dangerous and somewhat wrong. This can actually stop a server completely if the ASSERT() check fails. I'm not sure we want to do that.

This was discussed in a github PR, but I do not have the number right now.

Basically, we *know* that we always call this like this, but if we should ever call it differently, the alloc_buf_gc() call will lead to funnies
- thus, better ASSERT() in that case, so it's clear the calling contract has been violated.

Quoted 3 lines

I do see that there might be an issue if separator == NULL (strlen(NULL) will segfault), so this needs to be checked far more carefully and compared against all the callers of format_hex_ex().

See PR :)

(And yes, if things are discussed in a PR and patches get then sent to the list months(!) later, it would be useful to actually mention the PR in the commit message - "commit --amend" before sending)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Илья Шипицин

author

2016-09-18 16:07 UTC #3 lore raw

In reply to David Sommerseth

Hello,

this defect was found by cppcheck, however cppcheck still complains. so, we did not make it happy yet.

I think, the best would be split this function into 2 separate functions (with either null argument) or leave it like that.

2016-09-18 20:14 GMT+05:00 David Sommerseth <
[email protected]...>:

Quoted 63 lines

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 7 lines · Ilya Shipitsin
--- src/openvpn/buffer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
src/openvpn/buffer.c index renamed from src/openvpn/buffer.c +0 -0
52c6ab9..57bded9 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -438,10 +438,12 @@ format_hex_ex (const uint8_t *data, int size, int maxoutput, unsigned int
Quoted 9 lines
space_break_flags, const char* separator, struct gc_arena *gc) { - struct buffer out = alloc_buf_gc (maxoutput ? maxoutput : + int i;
+ struct buffer out; + ASSERT( maxoutput > 0 || separator != NULL ); + out = alloc_buf_gc (maxoutput ? maxoutput : ((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2), gc); - int i; for (i = 0; i < size; ++i) { if (separator && i && !(i % (space_break_flags & FHE_SPACE_BREAK_MASK)))
This looks dangerous and somewhat wrong. This can actually stop a server completely if the ASSERT() check fails. I'm not sure we want to do that.
The ASSERT() also expects maxoutput to be higher than 0, while the alloc_buf_gc() supports/allows maxoutput to be 0, which again confirms my bad feeling about this. If maxoutput == 0, alloc_buf_gc() will actually allocate a reasonable buffer regardless.
I do see that there might be an issue if separator == NULL (strlen(NULL) will segfault), so this needs to be checked far more carefully and compared against all the callers of format_hex_ex().
The question which pops up in my head is: Is this patch purely targeted to silence a code analyser warning? If so, this is most likely not the right fix for the OpenVPN code base.
- --
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJX3q9BAAoJEIbPlEyWcf3yGkwQAKKZbI2idz6a7h5ZkbQJCHMk JDNIpdR6XTFmG16jBrycYeew+9IRvfwpF4vh2/QPaRjDNLnKvPHP+9uEetqYmUQi rWzJyg3nzXSEUhFRSbdJcOK7zEWoiUUsSTiBKIYP6D5qD+QBQOZaQseCCzafrt5b IKrUesWMWPXAjQBYbUvcc801+JR5S/gSaSbk9TuO8Nemiry/GW/pSt0OVTggPdcm LiUyutQ8HXeXS9uAL32FLqFH3qq7QTHjxdVoKGGx9w/lmznqp+EaKCuANsWnt/PW ZTzVYM5YG5jI8XNM/wyCklu5n3yqJ37Noxd3ZRSD6ATTL/MExVgWINOOGA7f+Pj3 P6g4ZSc//zaWUCRpUKg4O8h6jaO+0stDS5eKDpyUCHYRHRdMI4IGLuwzIzKmxWq9 LpNGGY3Ikpf5IM+i2R04HctnNp+izEeW3vBCKnBBUHSyegRQGQ191Z5QC3SPk6VP HwxkHCZiOsie8WsP0Sl2dur1GjV9uyslVTkmt1GBVuoOGxpmZLJRJRayO70goZ17 ToavpadDjt193g79j1/dpE7OCPcCitso/+egxVzDWPTtLj+7rS/gfdJBNdCHr7xK Pf8KKbpD7yEbEln0K7wzsjVG749qZe103KmrhZulvu3H5+SrbT5ALhroaYidhRpg lLcx4g4A+nZjM6kabBGy
=QvxV
-----END PGP SIGNATURE-----

Jens Neuhalfen

2016-09-18 16:34 UTC #4 lore raw

Quoted 8 lines

Hello,
this defect was found by cppcheck, however cppcheck still complains. so, we did not make it happy yet.
I think, the best would be split this function into 2 separate functions (with either null argument) or leave it like that.

Unrelated to an actual fix, IMHO we should at least break up

out = alloc_buf_gc (maxoutput ? maxoutput :

				    ((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2),
				    gc);

and create a variable with a semantic variable for the result of "((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2)”

Honestly the calculation looks very arcane and I would better understand it, if i knew what it means :-)

Jens

Steffan Karger

2016-11-28 14:26 UTC #5 lore raw

PATCH 1/1 Clean up format_hex_ex()

Fix a potential null-pointer dereference, and make the code a bit more readable while doing so.

The NULL dereference could not be triggered, because the current code never called format_hex_ex() with maxouput == 0 and separator == NULL. But it's nicer to not depend on that.

Our use of int vs size_t for lengths needs some attention too, but I'm not pulling that into this patch. Instead I decided to just make the (previously existing) assumption that INT_MAX <= SIZE_MAX explicit by adding a static_assert().

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

src/openvpn/buffer.c +9 -6

index 84d2669..6af8dbb 100644
@@ -440,13 +440,16 @@ format_hex_ex (const uint8_t *data, int size, int maxoutput,
 	       unsigned int space_break_flags, const char* separator,
 	       struct gc_arena *gc)
 {
-  struct buffer out = alloc_buf_gc (maxoutput ? maxoutput :
-				    ((size * 2) + (size / (space_break_flags & FHE_SPACE_BREAK_MASK)) * (int) strlen (separator) + 2),
-				    gc);
-  int i;
-  for (i = 0; i < size; ++i)
+  const size_t bytes_per_hexblock = space_break_flags & FHE_SPACE_BREAK_MASK;
+  const size_t separator_len = separator ? strlen (separator) : 0;
+  static_assert (INT_MAX <= SIZE_MAX, "Code assumes INT_MAX <= SIZE_MAX");
+  const size_t out_len = maxoutput > 0 ? maxoutput :
+	    ((size * 2) + ((size / bytes_per_hexblock) * separator_len) + 2);
+
+  struct buffer out = alloc_buf_gc (out_len, gc);
+  for (int i = 0; i < size; ++i)
     {
-      if (separator && i && !(i % (space_break_flags & FHE_SPACE_BREAK_MASK)))
+      if (separator && i && !(i % bytes_per_hexblock))
 	buf_printf (&out, "%s", separator);
       if (space_break_flags & FHE_CAPS)
 	buf_printf (&out, "%02X", data[i]);

Signature · 1 line

2.7.4

Gert Doering

2016-11-28 16:31 UTC #6 lore raw

[Openvpn-devel] [PATCH applied] Re: Clean up format_hex_ex()

ACK. Long-standing eye-hurting code. This is better - and (staring at "openvpn --verb 11" output) seems to do the same thing. At least the code now looks like one could understand what it wants to do :-)

Your patch has been applied to the master branch.

commit 294040102fbceb4be320a45ba3c340ace1804a49

Author: Steffan Karger
Date:   Mon Nov 28 15:26:40 2016 +0100

     Clean up format_hex_ex()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13259.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Steffan Karger

2016-11-28 16:55 UTC #7 lore raw

PATCH 1/1 Clean up format_hex_ex()

Cherry-pick of commit 29404010 from master, slightly reworked to match the 2.3 codebase (no flags inside space_break here), and pulled in the new static_assert() fallback we also have in master now.

Fix a potential null-pointer dereference, and make the code a bit more readable while doing so.

The NULL dereference could not be triggered, because the current code never called format_hex_ex() with maxouput == 0 and separator == NULL. But it's nicer to not depend on that.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/buffer.c | 10 +++++++---
 src/openvpn/error.h  |  8 ++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

src/openvpn/buffer.c +7 -3

index fb3b52d..edbdd66 100644
@@ -400,9 +400,13 @@ format_hex_ex (const uint8_t *data, int size, int maxoutput,
 	       int space_break, const char* separator,
 	       struct gc_arena *gc)
 {
-  struct buffer out = alloc_buf_gc (maxoutput ? maxoutput :
-				    ((size * 2) + (size / space_break) * (int) strlen (separator) + 2),
-				    gc);
+  const size_t separator_len = separator ? strlen (separator) : 0;
+  static_assert (INT_MAX <= SIZE_MAX, "Code assumes INT_MAX <= SIZE_MAX");
+  const size_t out_len = maxoutput > 0 ? maxoutput :
+	    ((size * 2) + ((size / space_break) * separator_len) + 2);
+
+  struct buffer out = alloc_buf_gc (out_len, gc);
+
   int i;
   for (i = 0; i < size; ++i)
     {

src/openvpn/error.h +8 -0

index 36819e0..4d7fef9 100644
@@ -217,6 +217,14 @@ FILE *msg_fp(const unsigned int flags);
 void assert_failed (const char *filename, int line, const char *condition)
   __attribute__((__noreturn__));
 
+/* Poor-man's static_assert() for when not supplied by assert.h, taken from
+ * Linux's sys/cdefs.h under GPLv2 */
+#ifndef static_assert
+#define static_assert(expr, diagnostic) \
+    extern int (*__OpenVPN_static_assert_function (void)) \
+      [!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]
+#endif
+
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */
 #endif

Signature · 1 line

2.7.4

Gert Doering

2016-11-28 18:35 UTC #8 lore raw

[Openvpn-devel] [PATCH applied] Re: Clean up format_hex_ex()

ACK, thanks.

Same actual code change as in the master patch, just static_assert() added as backport.

Your patch has been applied to the release/2.3 branch.

commit 384302478f967e4fbb5e4d813eb30a5c15d00a14 (release/2.3)

Author: Steffan Karger
Date:   Mon Nov 28 17:55:07 2016 +0100

     Clean up format_hex_ex()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13269.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering