Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

miniupnpd: fw3 integration places MINIUPNPD chain at the top #286

Closed
f00b4r0 opened this issue Mar 4, 2017 · 8 comments
Closed

miniupnpd: fw3 integration places MINIUPNPD chain at the top #286

f00b4r0 opened this issue Mar 4, 2017 · 8 comments

Comments

@f00b4r0
Copy link

f00b4r0 commented Mar 4, 2017

As illustrated below, the miniupnpd firewall3 integration brings the MINIUPNPD chains at the very top of the parent chain (for prerouting, postrouting and forward).

This has the potential of enabling UPnP clients to override locally set port redirections, as illustrated in the following example (for prerouting).

I believe the local port redirections should always have priority and MINIUPNPD chains always be last, as also evidenced by the "iptables_init.sh" script provided by miniupnpd upstream which appends (-A) these chains instead of inserting them at the top (-I 1).

HTH

Chain zone_wan_prerouting (1 references)
 pkts bytes target     prot opt in     out     source               destination         
   14  1386 MINIUPNPD  all  --  any    any     anywhere             anywhere            
   14  1386 prerouting_wan_rule  all  --  any    any     anywhere             anywhere             /* !fw3: user chain for prerouting */
    0     0 DNAT       tcp  --  any    any     anywhere             anywhere             tcp dpt:10100 /* !fw3: HTTP */ to:192.168.0.100:80
@ldir-EDB0
Copy link
Contributor

Did you have any thoughts on how to attack this problem?

@f00b4r0
Copy link
Author

f00b4r0 commented Apr 30, 2018

@ldir-EDB0 other than fixing the code so that the MINIUPNPD chain ends at the bottom? No 😛

As far as I'm concerned this is a security issue and I'm surprised nobody seems to care...

@ldir-EDB0
Copy link
Contributor

I understand your concern ;-) jow & I are looking at it.

@pprindeville
Copy link
Member

So what's the downside of adding the hooks at the end of the chains?

@jow-
Copy link
Contributor

jow- commented May 3, 2018

So what's the downside of adding the hooks at the end of the chains?

It complicates the ruleset even further, causes increased cpu usage and this particular issue can be solved without adding extra hooks.

@ldir-EDB0
Copy link
Contributor

"So what's the downside of adding the hooks at the end of the chains?" I take it you mean putting the user pre_routing_wan rule at the end of the chain? Yes if fw3 added a user 'penultimate' before its generate DNAT rule then miniupnpd could live there, but that requires a change to fw3 and all of jow's comments apply.

@ldir-EDB0
Copy link
Contributor

This was addressed by openwrt/packages#6001

@f00b4r0
Copy link
Author

f00b4r0 commented Jul 5, 2018

Haven't tested the updated package but I'll take your word that it solves this issue 😉

@f00b4r0 f00b4r0 closed this as completed Jul 5, 2018
lynxis pushed a commit to lynxis/packages that referenced this issue Jan 3, 2019
Import miniupnpd from routing repo and bump to 20180422.

Drop 102-ipv6-ext-port.patch as this looks upstreamed in the pinhole
code to me.
Consolidate all other patches & update with a view to sending upstream.

Add support for runtime IGDv1 mode switch (default to IGDv2)

(not extensively) Tested-on: ar71xx Archer C7 v2 in IGDv1 compatibility
mode.  A variety of devices/applications appear to be able to create
mappings.

Have an attempt at resolving openwrt/routing#286
TL;DR miniupnpd rules get processed before fw3 rules and thus can
override existing/intended redirects.  Ideally the miniupnpd rules would
be last in the relevant chains, unfortunately fw3 can sometimes use the
last rule as a REJECT.  Put miniupnpd rules as penultimate.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants