| Message ID | 20200305084912.14659-1-ynezz@true.cz |
|---|---|
| State | Accepted |
| Delegated to: | Petr Štetiar |
| Headers | show |
| Series | [OpenWrt-Devel] rpcd: fix respawn settings | expand |
Petr Štetiar <ynezz@true.cz> wrote: > Commit 432ec292ccc8 ("rpcd: add respawn param") has introduced > infinite restarting of the service which could be reached over > network. Didn't we already decide that this wasn't the case? This is not recommended security practice as it might > give potential adversary infinite number of tries in case there > might be some issue in the rpcd or its surrounding stack. Sure, now it's a DoS instead :) It's always a tradeoff, but I think you're glossing over the tradeoff here. > > So lets remove the currently bogus `respawn_retry` variable (it > wasn't possible to override it anyway), reverting to the > previous default max. of 5 service restarts which could be now > overriden via system's UCI settings if desired. > > Cc: Jo-Philip Wich <jow@mein.io> > Cc: Florian Eckert <fe@dev.tdt.de> > Cc: Hauke Mehrtens <hauke@hauke-m.de> > Fixes: 432ec292ccc8 ("rpcd: add respawn param") > Signed-off-by: Petr Štetiar <ynezz@true.cz> > --- > package/system/rpcd/files/rpcd.init | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/system/rpcd/files/rpcd.init > b/package/system/rpcd/files/rpcd.init index > 3e9ea5bbf329..f75d0e0f0eea 100755 > --- a/package/system/rpcd/files/rpcd.init > +++ b/package/system/rpcd/files/rpcd.init > @@ -12,7 +12,7 @@ start_service() { > > procd_open_instance > procd_set_param command "$PROG" ${socket:+-s "$socket"} ${timeout:+-t "$timeout"} > - procd_set_param respawn ${respawn_retry:-0} > + procd_set_param respawn > procd_close_instance > } > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Karl Palsson <karlp@tweak.net.au> [2020-03-05 11:18:02]: > > Commit 432ec292ccc8 ("rpcd: add respawn param") has introduced > > infinite restarting of the service which could be reached over > > network. > > Didn't we already decide that this wasn't the case? < jow> ubus itself has no network transport < jow> it is reachable via http://.../ubus in case uhttpd-mod-ubus is installed (not the default) or via http://.../cgi-bin/luci/admin/ubus (default) < jow> the latter emulates uhttpd-mob-ubus in Lua code < jow> it takes incoming http requests, parses the body json and invokes ubus via libubus I understand this as Yes, it is available over network. > Sure, now it's a DoS instead :) It's always a tradeoff, but I > think you're glossing over the tradeoff here. Secure by default. -- ynezz
On Thu, Mar 5, 2020 at 5:35 AM Petr Štetiar <ynezz@true.cz> wrote: > Karl Palsson <karlp@tweak.net.au> [2020-03-05 11:18:02]: > > > > Commit 432ec292ccc8 ("rpcd: add respawn param") has introduced > > > infinite restarting of the service which could be reached over > > > network. > > > > Didn't we already decide that this wasn't the case? > > < jow> ubus itself has no network transport > < jow> it is reachable via http://.../ubus in case uhttpd-mod-ubus is > installed (not the default) or via http://.../cgi-bin/luci/admin/ubus > (default) > < jow> the latter emulates uhttpd-mob-ubus in Lua code > < jow> it takes incoming http requests, parses the body json and invokes > ubus via libubus > > I understand this as Yes, it is available over network. > > > Sure, now it's a DoS instead :) It's always a tradeoff, but I > > think you're glossing over the tradeoff here. > > Secure by default. > > -- ynezz > > The flip side here is that rpcd likes to crash a lot. By preventing automatic restarts, you're all but ensuring that users will experience denial-of-service, even in the absence of malicious traffic. Is rpcd subject to fuzz testing, to discover potential security issues that makes limiting the restarts attractive?
Mar 5, 2020 19:54:49 Michael Jones : > The flip side here is that rpcd likes to crash a lot. 0 (zero) bugs found https://bugs.openwrt.org/index.php?string=rpcd > By preventing automatic restarts, you're all but ensuring that users will experience denial-of-service, even in the absence of malicious traffic. Default respawn retry value was 5, now is infinite and this patch restores it back to 5 respawns. > Is rpcd subject to fuzz testing, to discover potential security issues Not yet, it's planed. It's just one of the methods, you'll never be 100% sure anyway. > that makes limiting the restarts attractive? "Any remote service which crashes is potentially exploitable; we have to assume so, until we see the specific way it fails." -- Theo, OpenBSD Recent real-world example from 36c3 in my previous email http://lists.infradead.org/pipermail/openwrt-devel/2020-March/022014.html
On Thu, Mar 5, 2020 at 1:41 PM Petr Štetiar <ynezz@true.cz> wrote: > > > Mar 5, 2020 19:54:49 Michael Jones : > > > The flip side here is that rpcd likes to crash a lot. > > 0 (zero) bugs found https://bugs.openwrt.org/index.php?string=rpcd Saying there are zero bugs on a bug tracker where issues go to be ignored is not a convincing argument. rpcd crashes for me daily, to the point where i have a script that restarts it every 5 minutes. It also gets hung a lot without crashing, and stops serving responses to ubus traffic. This is *only* with the UCI plugin, mind you. I don't use any of the other ones. If I create a bug report on flyspray, will it actually be looked at? Or will I be talking to myself? OpenWRT has a well-deserved reputation for user originated bug reports and requests for help going ignored. I've asked dozens of questions over the years on the forums that received no answer, and I've filed bugs that were still open with no feedback from anyone, last I bothered to check (Note: Not many of them have this email associated. I've worked many jobs that involved openwrt in some way) Note: I don't have any animosity about this. Volunteers are volunteers, I'm not expecting anyone to do anything. I'm just saying that that's not a valid argument unless or until the OpenWRT community engagement improves to the point where the bug tracker and forum stop being echo chambers. Will that happen? I don't know. Should it happen? I don't know. > By preventing automatic restarts, you're all but ensuring that users will > experience denial-of-service, even in the absence of malicious traffic. > > Default respawn retry value was 5, now is infinite and this patch restores > it back to 5 respawns. > Right, which means that you're re-introducing the denial-of-service-in-the-absence-of-traffic problem. I'm not saying that's the wrong thing to do. > > > Is rpcd subject to fuzz testing, to discover potential security issues > > Not yet, it's planed. It's just one of the methods, you'll never be 100% > sure anyway. > How can I help? I don't accept that you can't be 100% certain. Tools like https://klee.github.io/ can get you so close to 100% certainty that it's effectively 100%.
Hi, > rpcd crashes for me daily, to the point where i have a script that restarts it > every 5 minutes. > > It also gets hung a lot without crashing, and stops serving responses to ubus > traffic. I've never heard about anything like that until now, not even in the forum or IRC chatter. Getting some details here would be interesting. ~ Jo
On Fri, Mar 6, 2020 at 1:19 AM Jo-Philipp Wich <jo@mein.io> wrote: > Hi, > > > rpcd crashes for me daily, to the point where i have a script that > restarts it > > every 5 minutes. > > > > It also gets hung a lot without crashing, and stops serving responses to > ubus > > traffic. > > I've never heard about anything like that until now, not even in the forum > or > IRC chatter. Getting some details here would be interesting. > > ~ Jo > I've reviewed the mailing list for the previous year, and found this post: http://lists.infradead.org/pipermail/openwrt-devel/2019-October/019592.html Which appears to have been merged into rpcd with this commit : https://git.openwrt.org/?p=project/rpcd.git;a=commit;h=bd0ed2521476c3e5b6c1a0e0bd2c386ea809d74b This post / commit appears to identify the same crash that my scripts cause. However, the commit for the OpenWRT 18.06 branch (Still receiving security fixes, as far as I can tell), has this commit for RPCD https://github.com/openwrt/openwrt/blob/openwrt-18.06/package/system/rpcd/Makefile commit 3aa81d0dfae167eccc26203bd0c96f3e3450f253 Author: Jo-Philipp Wich <jo@mein.io> Date: Wed Nov 28 12:12:04 2018 +0100 file: access exec timeout via daemon ops structure Since the plugin is not linked, but dlopen()'d with RTLD_LOCAL, we cannot access global rpcd variables but need to access them via the common ops structure symbol. Signed-off-by: Jo-Philipp Wich <jo@mein.io> To see if that really fixed the issue, I will update my build of rpcd from 3aa81d0dfae167eccc26203bd0c96f3e3450f253 to bd0ed2521476c3e5b6c1a0e0bd2c386ea809d74b (or git head, perhaps) to see if the crash gets resolved.
diff --git a/package/system/rpcd/files/rpcd.init b/package/system/rpcd/files/rpcd.init index 3e9ea5bbf329..f75d0e0f0eea 100755 --- a/package/system/rpcd/files/rpcd.init +++ b/package/system/rpcd/files/rpcd.init @@ -12,7 +12,7 @@ start_service() { procd_open_instance procd_set_param command "$PROG" ${socket:+-s "$socket"} ${timeout:+-t "$timeout"} - procd_set_param respawn ${respawn_retry:-0} + procd_set_param respawn procd_close_instance }
Commit 432ec292ccc8 ("rpcd: add respawn param") has introduced infinite restarting of the service which could be reached over network. This is not recommended security practice as it might give potential adversary infinite number of tries in case there might be some issue in the rpcd or its surrounding stack. So lets remove the currently bogus `respawn_retry` variable (it wasn't possible to override it anyway), reverting to the previous default max. of 5 service restarts which could be now overriden via system's UCI settings if desired. Cc: Jo-Philip Wich <jow@mein.io> Cc: Florian Eckert <fe@dev.tdt.de> Cc: Hauke Mehrtens <hauke@hauke-m.de> Fixes: 432ec292ccc8 ("rpcd: add respawn param") Signed-off-by: Petr Štetiar <ynezz@true.cz> --- package/system/rpcd/files/rpcd.init | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)