Opened 3 years ago

Closed 11 months ago

Last modified 11 months ago

#163 closed Bug / Defect (fixed)

Segfault in PF

Reported by: svimik Owned by:
Priority: major Milestone:
Component: Networking Version: git master branch
Severity: Patch Queue: Merged Keywords: pf
Cc:

Description

OS


Bug reproduced on all versions and operating systems I tried:
OS: Debian (2.6.32-5-amd64 and 3.0.0-1-amd64), CentOS (2.6.38.2.domU.x86_64)
OpenVPN versions: 2.1.0, 2.1.3, 2.2.0 and openvpn-201130

CONFIG


configured as udp server with server-bridge.
with simple plugin just to enable PF:
http://backreference.org/2010/06/18/openvpns-built-in-packet-filter/

CONDITION


Shortly after starting the server, launch two clients with ~5 seconds delay.
Segfault probability on the server: >80% if proper delay between two connections were discovered.

VERSION


OpenVPN 2.x-testing-b7e0d372e3ae x86_64-unknown-linux-gnu [SSL] [LZO2] [EPOLL] [eurephia] [MH] [PF_INET6] [IPv6 payload 20110522-1 (2.2.0)] built on Sep 26 2011
Originally developed by James Yonan
Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <sales@…>

$ ./configure

Compile time defines: ENABLE_CLIENT_SERVER ENABLE_DEBUG ENABLE_EUREPHIA ENABLE_FRAGMENT ENABLE_HTTP_PROXY ENABLE_MANAGEMENT ENABLE_MULTIHOME ENABLE_PORT_SHARE ENABLE_SOCKS USE_CRYPTO USE_LIBDL USE_LZO USE_PF_INET6 USE_SSL

GDB


(gdb) run --cd /etc/openvpn --config /etc/openvpn/server_tcp.conf
Starting program: /usr/local/sbin/openvpn --cd /etc/openvpn --config /etc/openvpn/server_tcp.conf
Detaching after fork from child process 17280.
Detaching after fork from child process 17281.

Program received signal SIGSEGV, Segmentation fault.
pf_cn_test (pfs=0x0, tm=0x6cc230, type=2, prefix=0x468736 "bcast_c2c") at pf.c:414
414 if (!pfs->kill)

GDB bt


(gdb) bt
#0 pf_cn_test (pfs=0x0, tm=0x6cc230, type=2, prefix=0x468736 "bcast_c2c") at pf.c:414
#1 0x0000000000429cd3 in pf_c2c_test (m=0x7fffffffcd60, buf=<value optimized out>, sender_instance=0x6ad770, sender_addr=0x0) at pf-inline.h:38
#2 multi_bcast (m=0x7fffffffcd60, buf=<value optimized out>, sender_instance=0x6ad770, sender_addr=0x0) at multi.c:1898
#3 0x000000000042dc4d in multi_process_incoming_link (m=0x7fffffffcd60, instance=<value optimized out>, mpp_flags=5) at multi.c:2173
#4 0x0000000000428e0c in tunnel_server_udp_single_threaded (top=0x7fffffffdbb0) at mudp.c:167
#5 tunnel_server_udp (top=0x7fffffffdbb0) at mudp.c:274
#6 0x000000000042fa73 in main (argc=5, argv=0x7fffffffe9c8) at openvpn.c:211

LOG


http://svimik.com/openvpn_devel.log (815 KB)

Attachments (2)

openvpn.zip (5.9 KB) - added by dennis84 13 months ago.
openvpn configs to reproduce the segfault
openvpn_V1.zip (60.3 KB) - added by dennis84 13 months ago.
V1

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by svimik

Fixed.


diff --git a/pf.c b/pf.c
index 6b4cba4..311495a 100644
--- a/pf.c
+++ b/pf.c
@@ -411,7 +411,7 @@ lookup_cn_rule (struct hash *h, const char *cn, const uint32
 bool
 pf_cn_test (struct pf_set *pfs, const struct tls_multi *tm, const int type, con
 {
-  if (!pfs->kill)
+  if (pfs && !pfs->kill)
     {
       const char *cn;
       uint32_t cn_hash;

comment:2 Changed 20 months ago by dazo

  • Component changed from Generic / unclassified to Networking
  • Priority changed from blocker to major
  • Version set to git master branch

The patch suggestion will circumvent this crash, indeed. But it would be good to understand why the *pfs pointer is NULL before committing to this solution.

comment:3 follow-up: Changed 14 months ago by NickPadilla

Hello,

Has anyone made any more progress on this issue? We will need to use this functionality for a project. We wouldn't mind flushing out the bug if we could get a hint on where to start, assuming pf.c. Please let know if there is anything else I should know before I start troubleshooting this issue.

Thanks!

Nick

comment:4 Changed 14 months ago by dennis84

i also would like to know, in which release this patch will be included.

Thanks for your Support Guys!

comment:5 Changed 13 months ago by dazo

Please see comment #2 ... we need to understand why the *pfs pointer is NULL first, to see if the suggested fix in comment #1 is the proper one. So if anyone are willing to dig into this and come with a thesis, that would be highly appreciated.

comment:6 in reply to: ↑ 3 Changed 13 months ago by dazo

Replying to NickPadilla:

Please let know if there is anything else I should know before I start troubleshooting this issue.

pf.c is one place to start looking. But tracing down the whole code path where pf_cn_test() is called and down to the level where *pfc is set to NULL needs to be figured out.

comment:7 Changed 13 months ago by dazo

  • Keywords pf added; segfault packet filter removed

Changed 13 months ago by dennis84

openvpn configs to reproduce the segfault

comment:8 Changed 13 months ago by dennis84

Changed 13 months ago by dennis84

V1

comment:9 Changed 11 months ago by Waffelman

Hello,

I've tried to tackle this problem.

The patch posted by svimik indeed works, and it should be accepted as soon as possible. This little change fixed the segfaults for me.

I've also spent some time understanding what happened. There are two possible problem configurations:

  • Using a plugin with the mask OPENVPN_PLUGIN_ENABLE_PF
  • Starting the OpenVPN server using the "management-client-pf" configuration

The results are of course equal, but they are handled differently in the code.
The problem in both cases is that the pointer pfs in the struct pf_context is null when it is dereferenced in pf_cn_test.

Let me explain how this happens in both cases.

OPENVPN_PLUGIN_ENABLE_PF

dennis84's link describes how to create a rudimentary packet filter plugin for OpenVPN. By enabling it in the configuration, the call to plugin_defined in pf.c returns true. This is essentially all the plugin has to do: Provide the correct type mask.

The following two lines create the temporary file for the packet filter instructions. Afterwards the plugin func is called, which, if it returns true, actually enables packet filtering:

The filename and enabled flag are set in the context. Note that the created temporary file is empty (except if the plugin func has written something into it, I will return to that later).

Now let's step into the pf_init function, which is the culprit that should return the defined filters as a pf_set.

Since the packet filter file is empty, there is nothing in the buffer list passed into the function. There are two possibilities (I did not care to figure out which one it actually is):

If the first case is true, n_errors is increased. If the second case is true, n_errors is increased. So it doesn't really matter :) What does matter, though, is that, because n_errors is now greater than 0, this if block is entered, where pfs is destroyed and set to NULL. Afterwards, this NULL value is returned.

This is why the pfs pointer is NULL. There is however one workaround for this: Since the packet filter plugin func is executed before packet filtering is actually enabled, one can perform the following steps to ensure that the pf_init function does return something:

  • Locate the "pf_file=..." entry in the array of environment variables (envp). ... denotes the actual path to the packet filter file.
  • Open this file in write mode.
  • Write the following content into the file: "[CLIENTS DROP]\n[SUBNETS ACCEPT]\n[END]".
  • Close the file.

This way, you have ensured that there is something meaningful in the file from the beginning. Using this technique, I could stop the segfaults.

management-client-pf

In case of the management interface instruction client-pf it is much simpler.

Packet filtering is enabled in the pf_init_context function as soon as OpenVPN is started with the management-client-pf option. This means that the pfs pointer is set to NULL.

But the pointer is only changed when the user executes the client-pf command over the management interface. This function pointer is set to management_client_pf right here.

As you can see, the packet filters are only changed when management interface lines are parsed.

Last edited 11 months ago by Waffelman (previous) (diff)

comment:10 Changed 11 months ago by dazo

  • Severity changed from Not set (if unsure, select this one) to Patch Queue: In progress

Thank you very much for your thorough review of this issue, Waffelman. Now it is fully possible to understand why pfs is NULL, which definitely was the most important thing here.

That it is NULL due to no parsed pf contents (file or management interface), makes a lot of sense. So I agree, the patch can now be included. The other approach and writing to this file, to give it some valid contents seems like the wrong solution to me.

And I checked quickly the call paths for pf_cn_test(), which always is called via pf_c2c_test(). So it seems to be something like this:

       -> multi_process_incoming_link()      [multi.c: 2179, 2241]
          -> multi_bcast()                   [multi.c:1939]
             -> pf_c2c_test()                [pf-inline.h:35]
                -> pf_cn_test()              [pf.c:418]

  OR   -> multi_prcocess_incoming_tun()      [multi.c:2361]
          -> multi_bcast()                   [multi.c:1939]
             -> pf_c2c_test()                [pf-inline.h:35]
                -> pf_cn_test()              [pf.c:418]

  OR   -> multi_process_incoming_link()      [multi.c:2106]
          -> pf_c2c_test()                   [pf-inline.h:35]
             -> pf_cn_test()                 [pf.c:418]

  OR   -> multi_process_incoming_link()      [multi.c:2251]
          -> pf_c2c_test()                   [pf-inline.h:35]
             -> pf_cn_test()                 [pf.c:418]

And all these multi_*() functions do handle tunnel packets. Of course, a fix could be added to pf_c2c_test() instead, not calling pf_cn_test() if pfs == NULL. But I see no particular reason why that would be a better approach. So the patch in comment 1, gets an ACK from me!

Last edited 11 months ago by dazo (previous) (diff)

comment:11 Changed 11 months ago by dazo

  • Resolution set to fixed
  • Severity changed from Patch Queue: In progress to Patch Queue: Merged
  • Status changed from new to closed

Patch applied to master and release/2.3 branches.

commit 31e5f34f3c6cf3aa6f120d22c415ac74a5ba1639  master
commit 9b9e33cc36f9a6d40244cf6ea5547cc5eae7d79c  release/2.3
Author: svimik <svimik@mail.ru>
Date:   Thu Sep 29 13:41:34 2011 +0200

    Fix segfault when enabling pf plug-ins
    
    This fixes an issue where a segfault happens in pf_cn_test() if no
    packet filtering rules have been parsed.  See the trac ticket for
    more details.
    
    Trac: 163
    Signed-off-by: David Sommerseth <davids@redhat.com>
    Acked-by: David Sommerseth <davids@redhat.com>

comment:12 Changed 11 months ago by Dennis84

great Guys!

Thanks for your support! :)

Note: See TracTickets for help on using tickets.