openwrt
miniupnpd: Update to 2.3.7 and enable regex filter
UPnP rules now may have an optional regex filter on requester's descriptions. This is a countermeasure against some UPnP exploiters without shutting down UPnP service completely, albeit they can bypass it by reporting innocent's descriptions maliciously.
Since the filter specifier is optional, existing valid config files will still work.
This increases the executable's size by 1.3 kB from original 147.7 kB on i386.
Maintainer: Compile tested: i386 snapshot Run tested: i386 snapshot
Hilarious.
So several days ago, I got an email with a similar changeset that ended up in my spam folder for whatever reason. My suspicion is someone does not want to use a real name.
Interesting.
https://github.com/openwrt/packages/blob/10644cacbeb2523c30a4c54cf3cee5c7ce92b376/net/i2pd/Makefile#L21
Why my name suddenly becomes 'not my real name'.
Commit description is missing for both commits according to https://openwrt.org/submitting-patches. Would be good to know why do we need to enable regex filter and also the size difference of that package.
@yangfl: Thanks!
Linked to:
- https://github.com/openwrt/packages/issues/23948
- https://github.com/openwrt/packages/pull/23176
Hi @yangfl
A few minor comments on the implemented PR that I consider important:
Since the filter specifier is optional, existing valid config files will still work.
- Have you checked how the daemon config file is generated in init? Isn't there a conflict with the comment field?
- Have you checked whether the option works with OpenWrt at all after implementation, when using a custom config file? My quick test showed that the added option does not work.
Actually, it works - by not working. It's an upstream bug and should be fixed there.
Current opened tickets about miniupnp:
- https://github.com/search?q=repo%3Aopenwrt%2Fpackages+miniupnp+is%3Aissue+state%3Aopen&type=issues&s=updated&o=desc
- https://github.com/search?q=repo%3Aopenwrt%2Fopenwrt+miniupnp+is%3Aissue+state%3Aopen&type=issues&s=updated&o=desc
Actually, it works - by not working. It's an upstream bug and should be fixed there.
But if it works, we have a new issue. With the comment in place of the regular expressions. Correct?
~~Yes but as long as upstream does not fix it, everything would be fine.~~ I think this could be fixed quickly
--- a/net/miniupnpd/files/miniupnpd.init
+++ b/net/miniupnpd/files/miniupnpd.init
@@ -39,7 +39,8 @@ conf_rule_add() {
# Make a single IP IP/32 so that miniupnpd.conf can use it.
[ "${int_addr%/*}" = "$int_addr" ] && int_addr="$int_addr/32"
- echo "$action $ext_start${ext_end:+-}$ext_end $int_addr $int_start${int_end:+-}$int_end #$comment"
+ echo "#$comment"
+ echo "$action $ext_start${ext_end:+-}$ext_end $int_addr $int_start${int_end:+-}$int_end"
}
upnpd_write_bool() {
Since you are suggesting an upstream fix, why not just ignore the regular expression if the fifth token starts with a #? I think this is the better option, as I find the ACL entry comments in the daemon config file are useful.
Making '#' an exception introduces inconsistency. What if more features are add later?
Alternatively if separating comments is not feasible (which I could't get the point here), simply provide an empty token here ("" or '') will work.
Making '#' an exception introduces inconsistency. What if more features are add later?
Considering that the daemon is more than two decades old and the ACL entries have not changed, I don't think this will happen. Even when using #, it can be distinguished from a regular expression that starts with "#....
With the current package PR in preparation, there are even more ACL comments where adding an extra line would bloat the file. If you would agree to the suggestion, I could rework the daemon config generation so that if the ACL comment starts with a ", a # is not generated. This would also make your "half" implemented regular expression filter option accessible through the LuCI UI.
Ok, let's make '#' a terminator.
But I don't think embed reg filter, or anything meaningful into ACL comment is a good idea. Eventually it should get its own option someday.
Ok, let's make '#' a terminator.
Cool, thanks.
But I don't think embed reg filter, or anything meaningful into ACL comment is a good idea. Eventually it should get its own option someday.
I've reconsidered. I have created a separate UCI (and LuCI UI) option, for this (Description filter descr_filter). However, I don't think the workaround offered by the option is complete yet, as it would also be helpful to filter by the client's User-Agent SOAP header as the current filter can only be used with IPv4 and UPnP IGD.
Thanks for your downstream PR. If I understand your PR correctly, this will then be accepted:
allow 1024-65535 0.0.0.0/0 1024-65535 "regex" # comment
allow 1024-65535 0.0.0.0/0 1024-65535 "" # comment
allow 1024-65535 0.0.0.0/0 1024-65535 # comment
If any of you would like to test/use the implementation of the Description filter option in OpenWrt, including LuCI, please get in touch directly via the linked PR. I have also added the required suggested daemon fix as patch.
https://redirect.github.com/openwrt/packages/pull/24988