Re: [PATCH] nm-pptp-service: Grant proto GRE by firewalld

[Lubomir Rintel <lkundrak v3 sk>]

Taking this off-list, since it's not constructive.

My actual recomendation is to get Thomas Woerner's opinion; as none of
us seem to be familiar enough with firewalld.

We should eventually fix this; thanks for pointing it out. I just feel
uncomfortable about the current patch and feel that it needs
improvement.

Thanks
Lubo

On Fri, 2017-03-03 at 10:24 +0100, poma wrote:
> On 02.03.2017 20:32, Lubomir Rintel wrote:
> > On Wed, 2017-03-01 at 08:07 +0100, poma wrote:
> > > From 28b7713cda1deba1b54bd9e52b0d62716e356b66 Mon Sep 17 00:00:00
> > > 2001
> > > > From: poma <poma gmail com>
> > >
> > > Date: Wed, 1 Mar 2017 07:05:40 +0100
> > > Subject: [PATCH] nm-pptp-service: Grant proto GRE by firewalld.
> > >
> > > With recent kernels, the Poptop - The PPTP Server for Linux
> > > (pptpd) requires
> > > explicit load of nf_conntrack_pptp kernel module to achieve the
> > > operating state of the service itself.
> > > However this is not the case with the PPTP Client (pptp) on a
> > > Linux based platform.
> > > What is needed is to apply directly, rule within the firewalld,
> > > to grant proto gre,
> > > to achieve the operating state of the client itself.
> > >
> > > Ref.
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1187328
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1214643
> > > ---
> > >  src/nm-pptp-service.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/nm-pptp-service.c b/src/nm-pptp-service.c
> > > index 1710fd9..6a66386 100644
> > > --- a/src/nm-pptp-service.c
> > > +++ b/src/nm-pptp-service.c
> > > @@ -1113,7 +1113,7 @@ main (int argc, char *argv[])
> > > >         GMainLoop *main_loop;
> > > >         gboolean persist = FALSE;
> > > >         GOptionContext *opt_ctx = NULL;
> > > > -       char *conntrack_module[] = { "/sbin/modprobe",
> > > > "nf_conntrack_pptp", NULL };
> > > > +       char *firewalld_grant_proto_gre[] = { "/bin/firewall-
> > > > cmd", "--direct", "--add-rule", "ipv4", "filter", "INPUT", "0",
> > > > "-p", "gre", "-j", "ACCEPT", NULL };
> > > > ...
> > > > +       if (!g_spawn_sync (NULL, firewalld_grant_proto_gre,
> > > > NULL, 0, NULL, NULL, NULL, NULL, NULL, &error)) {
> > > > +               _LOGW ("granting proto gre by firewalld
> > > > failed: %s", error->message);
> > > >                 g_error_free (error);
> > > >         }
> >
> > As Thomas Haller already suggested; we probably should not be
> > removing
> > the module load. It doesn't seem to have anything to do with a
> > missing
> > firewall rule.
>
> With only loaded modules it will not work if:
> $ sysctl net.netfilter.nf_conntrack_helper
> net.netfilter.nf_conntrack_helper = 0
> OR
> $ cat /sys/module/nf_conntrack/parameters/nf_conntrack_helper
> N
>
> The correct firewall(s) rule(s) should replace both.
>
> > I'm not sure either whether we should be punching holes in the
> > firewall
> > automatically or if this is the proper way to do that. I'm
> > especially
> > not sure if we should be calling firewall-cmd instead of talking D-
> > Bus
> > and if it's all right that we don't clean up the rule when we tear
> > down
> >  all PPTP connections.
> >
> > Adding Thomas Woerner to the loop; hopefully he'll be able to
> > provide
> > some help.
> >
> > Lubo
>
> I wrote about firewalld because I use it.
> No one's stopping NM to talk via D-BUS with any implementation of a
> firewall that is used.
>
> So, what is your actual recommendation, in this particular case?
>
> Ref.
> - Secure use of iptables and connection tracking helpers
>   https://home.regit.org/netfilter-en/secure-use-of-helpers
> - iptables connection tracking helpers
>   http://www.odi.ch/weblog/posting.php?posting=663
> - Automatic Helper Assignment
>   http://www.firewalld.org/2016/10/automatic-helper-assignment
> - Netfilter Helpers
>   http://shorewall.org/Helpers.html