openwrt / luci

LuCI - OpenWrt Configuration Interface
Apache License 2.0
6.27k stars 2.51k forks source link

Feature request to have LuCI limit the length of firewall zone names to prevent total network breakage #507

Closed TimMillerDyck closed 8 years ago

TimMillerDyck commented 8 years ago

Hello, I had a colleague use LuCI to create a firewall zone named "guestwireless", which seems harmless. Immediately after creating the zone, all network was nonfunctional on OpenWrt and safe mode (and physical access) was needed to diagnose the problem. The issue proved to be the length of the zone name. Based on experimentation, any firewall zone name 12 characters or more in length breaks the OpenWrt firewall rule generator such that all networking doesn't work any longer.

This is a request just to add a length limit to the LuCI firewall name field to prevent this in the future.

More details are here: firewall zone name length of 12 characters or more breaks all networking https://dev.openwrt.org/ticket/20380

Thanks, Tim Miller Dyck

hnyman commented 8 years ago

my thoughts: The current firewall implementation "fw3" sets the zonename limit to 14 characters. That limit is now also implemented in Luci trunk and in CC for-15.05 branch. (commit https://github.com/openwrt/luci/commit/edc58332f0bc93a637d1b44689849e9cef29596d)

But the evidence in that bug 20380 seems to indicate that actually 11 characters is the practical real-life maximum zone name length. If that is true, then the firewall itself should be patched first to enforce the 11 characters limit. Luci can then be patched later.

I see two different approaches:

I would maybe prefer the seconds option, to maintain good possibilities for zone naming. Any thoughts, @jow- ?

jow- commented 8 years ago

The problem is the user chains, e.g. postrouting_ZONE_rule, I'd love to rename it to something shorter but those chains are documented external user hooks so altering their name will likely break various existing deployments.

hnyman commented 8 years ago

Then it looks to me that firewall3 should be modified to limit the max. zone name to 11 chars. Luci can be changed separately, later.

It seems to me that the limit set in firewall3 sources (zones.h) is probably calculated wrongly using XT_TABLE_MAXNAMELEN (=32) instead of XT_EXTENSION_MAXNAMELEN (=29) as the basis when defining the limit as 14:

http://nbd.name/gitweb.cgi?p=firewall3.git;a=blob;f=zones.h;h=4205196268f75061280ac13da656ab4362245a06;hb=18a503d0125aebc3a8d62dad1c02e6bb1da92eb6#l25

/* 32 - sizeof("postrouting_") - sizeof("_rule") - sizeof("\0") */
  #define FW3_ZONE_MAXNAMELEN 14

Reference to iptables: http://git.netfilter.org/iptables/tree/iptables/iptables.c?id=482c6d3731e2681cb4baae835c294840300197e6#n381

XT_EXTENSION_MAXNAMELEN as 29: http://lxr.free-electrons.com/source/include/uapi/linux/netfilter/x_tables.h

Ps. Naturally Luci could be changed already earlier, but Luci would allow less than fw3.

TimMillerDyck commented 8 years ago

Hi hnyman and jow, I was thinking LuCI would be easier to change as a first step as I suspect more users edit firewall rules using LuCI than directly editing the configuration files, and a LuCI update would be possible to deploy as a regular package update through opkg. Thanks for looking at this issue! -Tim

hnyman commented 8 years ago

Yep, Luci could be "fixed" by changing the limit of the max zone name length 14 --> 11. But that might lead into strange situations, where a manually created config file could not be edited in Luci, or actually Luci would restrict the length more aggressively.

@jow- Any opinions? Are you going to change the zone name length limit in firewall3 ? Or could Luci be changed already before that? (I can do that)

jow- commented 8 years ago

Yes, lets restrict to 11 in LuCI for now, in many places luci is more strict than the cli system, often because init scripts etc. do not check data integrity at all.

hnyman commented 8 years ago

Change pushed to Luci master and for-15.05 branch.

TimMillerDyck commented 8 years ago

Thanks very much, guys! I added a note on the length to the wiki page documenting firewall settings at http://wiki.openwrt.org/doc/uci/firewall#zones so, for those using the CLI, there is documentation on the length limit.

Regards, Tim

hnyman commented 8 years ago

@jow- One additional issue, for which I would like to have your opinion:

One user at bug 20380 noted that there is also a similar problem with the interface name length. See https://dev.openwrt.org/ticket/20380#comment:7 onward.

There is apparently a 15 character limit to the "real" interface name, enforced both in the firewall and dnsmasq. The real interface names includes the possible prefix "br-", "6in4-" etc.

I already thought to limit the interface name length to 12 characters (15-"br-"), but I then thought about the prefix and realised that I myself have had a 6in4 tunnel, where prefix is 5 chars long "6in4-". And after further checking, the prefix "pppoe-" is even longer, 6 chars. That is the longest prefix I have found so far.

So, the interface name length limit should maybe be limited to just 9 characters to allow for a protocol-dependent prefix to be added and still fitting into the limit.

It seems waste to limit it so short, but enforcing a protocol-dependent limit setting exceeds my lua skills. https://github.com/openwrt/luci/blob/master/modules/luci-mod-admin-full/luasrc/model/cbi/admin_network/iface_add.lua

There might maybe be a 15 chars (or 12 ? thinking about "br-") limit for the name initially, and then the newproto.validate function might contain logic to check for a protocol-dependent shorter limit.

Any ideas?

This may cause hard-to-find errors for the users, so authoring a fix would be great.