Opened 6 years ago

Closed 4 months ago

#586 closed Patch submission (fixed)

Possible Segmentation Fault issue in OpenVPN 2.3.x

Reported by: dogbert2 Owned by: David Sommerseth
Priority: major Milestone: release 2.5.2
Component: Generic / unclassified Version: OpenVPN 2.3.5 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords: Possible Segmentation Violation/Fault
Cc: David Sommerseth, tct

Description

Hello All,

In reviewing code in OpenVPN 2.3.6, I found in file 'buffer.c'

if DMALLOC is defined, at line 553, where ret = openvpn_dmalloc
(file, line, n) which is a call to malloc() in the memdbg.h file
where it appears to reference malloc(). In the case where NULL
is returned to variable 'ret', in this case, a call to memset()
with variable 'ret' equal to NULL, which will result in a
segmentation violation/fault.

The patch file below should address this issue:

--- buffer.c.orig 2015-07-31 23:03:56.265948284 -0400
+++ buffer.c 2015-07-31 23:05:21.439948284 -0400
@@ -554,6 +554,7 @@

*/

#ifdef DMALLOC

ret = openvpn_dmalloc (file, line, n);

+ check_malloc_return(ret);

memset(ret, 0, n);

#else

ret = calloc(1, n);


Comments, Questions, Suggestions, Complaints? :)

I am attaching the patch file to this bug report...

Bill Parker (wp02855 at gmail dot com)

Attachments (1)

buffer.c.patch (292 bytes) - added by dogbert2 6 years ago.
Patch file for ticket 586

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by dogbert2

Attachment: buffer.c.patch added

Patch file for ticket 586

comment:1 Changed 6 years ago by Gert Döring

Cc: David Sommerseth added
Milestone: release 2.3.8

It's a somewhat unlikely case and would need an actual DMALLOC build to be risky - but still, as long as we're not sure that dmalloc_malloc() (which openvpn_dmalloc() maps to) never returns NULL, needs fixing...

The problematic code was introduced in commit dc7be6d0, part of a series of ambitious but not well-understood-enough cleanup changes.

comment:2 Changed 6 years ago by Gert Döring

Milestone: release 2.3.8release 2.3.9

comment:3 Changed 5 years ago by Samuli Seppänen

Milestone: release 2.3.9release 2.3.12

comment:4 Changed 5 years ago by Gert Döring

Milestone: release 2.3.12release 2.3.14
Owner: set to David Sommerseth
Status: newassigned

either we rip out DMALLOC or we fix it :)

comment:5 Changed 3 years ago by Playgame123

My computer is the best platform to given the commend easily in your desktop connection by the users you see the file the explorer in http://mycomputerwindows10.com and save i must be most people have the concoct this latest version.

Version 0, edited 3 years ago by Playgame123 (next)

comment:6 Changed 20 months ago by tct

Cc: tct added

comment:7 Changed 16 months ago by nomozine

Thank you for this information

Very helpful

comment:8 Changed 4 months ago by Gert Döring

Milestone: release 2.3.14release 2.5.3

Not sure why this was never closed.

Checked buffer.c, and while there's check_malloc_return() all over the place, the problematic code is still there

#ifdef DMALLOC
            ret = openvpn_dmalloc(file, line, n);
            memset(ret, 0, n);
#else
            ret = calloc(1, n);
#endif
            check_malloc_return(ret);

so it seems we should really clean this up one day...

comment:9 Changed 4 months ago by Gert Döring

Milestone: release 2.5.3release 2.5.2
Resolution: fixed
Status: assignedclosed

commit e2acfad40c0d79ce7fd431c380d7466d383bcefa (master)
commit acf52dda9f4cb117e9d020dd06fccd7ecb90d303 (release/2.5)
commit a7263a125199c6d11710ecf50f9a07424369fdbc (release/2.4)
Author: Gert Doering
Date: Fri Apr 2 19:34:14 2021 +0200

Fix potential NULL ptr crash if compiled with DMALLOC

Note: See TracTickets for help on using tickets.