On Mon, Dec 07, 2020 at 08:57:05PM +0100, Hauke Mehrtens wrote: > On 12/7/20 7:14 PM, Daniel Golle wrote: > > Granting capabilities CAP_NET_ADMIN and CAP_NET_RAW allows running > > hostapd and wpa_supplicant without root priviledges. > > Add ubus acl allowing the necessary ubus interactions for the 'network' > > user running hostapd/wpa_supplicant. > > To still allow netifd to acquire the PID of wpa_supplicant and hostapd > > introduce a new ubus method 'getpid' for both of them and make use of > > that instead of querying procd (which will not work, as procd will > > return the PID of ujail) > > > > Signed-off-by: Daniel Golle <dan...@makrotopia.org> > > --- > > package/network/services/hostapd/Makefile | 9 ++++++-- > > .../network/services/hostapd/files/hostapd.sh | 2 +- > > .../network/services/hostapd/files/wpad.init | 16 ++++++++++++++ > > .../network/services/hostapd/files/wpad.json | 22 +++++++++++++++++++ > > .../services/hostapd/files/wpad_acl.json | 10 +++++++++ > > .../services/hostapd/src/src/ap/ubus.c | 16 ++++++++++++++ > > .../hostapd/src/wpa_supplicant/ubus.c | 16 ++++++++++++++ > > 7 files changed, 88 insertions(+), 3 deletions(-) > > create mode 100644 package/network/services/hostapd/files/wpad.json > > create mode 100644 package/network/services/hostapd/files/wpad_acl.json > > > > diff --git a/package/network/services/hostapd/Makefile > > b/package/network/services/hostapd/Makefile > > index 61b2a548ef..386c17f99d 100644 > > --- a/package/network/services/hostapd/Makefile > > +++ b/package/network/services/hostapd/Makefile > > @@ -7,7 +7,7 @@ > > include $(TOPDIR)/rules.mk > > PKG_NAME:=hostapd > > -PKG_RELEASE:=18 > > +PKG_RELEASE:=19 > > PKG_SOURCE_URL:=http://w1.fi/hostap.git > > PKG_SOURCE_PROTO:=git > > @@ -149,6 +149,7 @@ define Package/hostapd/Default > > TITLE:=IEEE 802.1x Authenticator > > URL:=http://hostap.epitest.fi/ > > DEPENDS:=$(DRV_DEPENDS) +hostapd-common +libubus > > + USERID:=network=101:network=101 > > Do we have some central uid assignment or at least a list of all used uids?
I grep for 'USERID:=' in all feeds to find out existing assignments and try to use the same UID/GID values as assigned in FreeBSD (which does have a central uid/gid allocation scheme) I vaguely remember a discussion about that some years ago and I'm not sure what we have concluded back then... > We should prevent that someone uses 101 on some other package. Actually UID and GID 101 are defined for the user 'network' in the base-files package (among with user 'daemon' and user 'ftp', why ever). > Do you plan to add more services under the network user? I must admit the choice to use the existing 'network' user was kinda arbitrary. Unless it would make things really much much easier while not loosing a lot of security I'd always have a seperate user for each service. Potential candidates are: netifd: in it's current form netifd is not easily restricted, as it touches a lot of sysctl which isn't covered by capabilities nor can we chown files in /proc/sys, it launches protocol handler processes which need priviledges, ... pppd: would traditionally run as the 'uucp' user, giving it access to modem chardevs. As it is lauched by netifd rather than being a procd service we'd have to think of a way that netifd can leverage procd's jail feautres. The same applies for odhcpd and odhcp6c, both would be easy to contain (eg. using seccomp in addition to running non-root with only the necessary capabilities), but currently netifd's built-in process management can't do that for us. > > > PROVIDES:=hostapd > > CONFLICTS:=$(HOSTAPD_PROVIDERS) > > HOSTAPD_PROVIDERS+=$(1) > > @@ -232,6 +233,7 @@ define Package/wpad/Default > > SUBMENU:=WirelessAPD > > TITLE:=IEEE 802.1x Auth/Supplicant > > DEPENDS:=$(DRV_DEPENDS) +hostapd-common +libubus > > + USERID:=network=101:network=101 > > URL:=http://hostap.epitest.fi/ > > PROVIDES:=hostapd wpa-supplicant > > CONFLICTS:=$(HOSTAPD_PROVIDERS) $(SUPPLICANT_PROVIDERS) > > @@ -346,6 +348,7 @@ define Package/wpa-supplicant/Default > > TITLE:=WPA Supplicant > > URL:=http://hostap.epitest.fi/wpa_supplicant/ > > DEPENDS:=$(DRV_DEPENDS) +hostapd-common +libubus > > + USERID:=network=101:network=101 > > PROVIDES:=wpa-supplicant > > CONFLICTS:=$(SUPPLICANT_PROVIDERS) > > SUPPLICANT_PROVIDERS+=$(1) > > @@ -597,10 +600,12 @@ define Install/supplicant > > endef > > define Package/hostapd-common/install > > - $(INSTALL_DIR) $(1)/lib/netifd $(1)/etc/rc.button > > $(1)/etc/hotplug.d/ieee80211 $(1)/etc/init.d > > + $(INSTALL_DIR) $(1)/etc/capabilities $(1)/etc/rc.button > > $(1)/etc/hotplug.d/ieee80211 $(1)/etc/init.d $(1)/lib/netifd > > $(1)/usr/share/acl.d > > $(INSTALL_DATA) ./files/hostapd.sh $(1)/lib/netifd/hostapd.sh > > $(INSTALL_BIN) ./files/wpad.init $(1)/etc/init.d/wpad > > $(INSTALL_BIN) ./files/wps-hotplug.sh $(1)/etc/rc.button/wps > > + $(INSTALL_DATA) ./files/wpad_acl.json $(1)/usr/share/acl.d > > + $(INSTALL_DATA) ./files/wpad.json $(1)/etc/capabilities > > endef > > define Package/hostapd/install > > diff --git a/package/network/services/hostapd/files/hostapd.sh > > b/package/network/services/hostapd/files/hostapd.sh > > index 41b04e6029..781a34028a 100644 > > --- a/package/network/services/hostapd/files/hostapd.sh > > +++ b/package/network/services/hostapd/files/hostapd.sh > > @@ -1365,6 +1365,7 @@ wpa_supplicant_run() { > > _wpa_supplicant_common "$ifname" > > ubus wait_for wpa_supplicant > > + local supplicant_pid=$(ubus call wpa_supplicant getpid | jsonfilter -l > > 1 -e @.pid) > > ubus call wpa_supplicant config_add "{ \ > > \"driver\": \"${_w_driver:-wext}\", \"ctrl\": \"$_rpath\", \ > > \"iface\": \"$ifname\", \"config\": \"$_config\" \ > > @@ -1376,7 +1377,6 @@ wpa_supplicant_run() { > > [ "$ret" != 0 ] && wireless_setup_vif_failed WPA_SUPPLICANT_FAILED > > - local supplicant_pid=$(ubus call service list '{"name": "wpad"}' | > > jsonfilter -l 1 -e "@['wpad'].instances['supplicant'].pid") > > wireless_add_process "$supplicant_pid" "/usr/sbin/wpa_supplicant" 1 > > return $ret > > diff --git a/package/network/services/hostapd/files/wpad.init > > b/package/network/services/hostapd/files/wpad.init > > index 3198e9801f..6c60d539a2 100644 > > --- a/package/network/services/hostapd/files/wpad.init > > +++ b/package/network/services/hostapd/files/wpad.init > > @@ -9,17 +9,33 @@ NAME=wpad > > start_service() { > > if [ -x "/usr/sbin/hostapd" ]; then > > mkdir -p /var/run/hostapd > > + chown network:network /var/run/hostapd > > procd_open_instance hostapd > > procd_set_param command /usr/sbin/hostapd -s -g > > /var/run/hostapd/global > > procd_set_param respawn > > + [ -x /sbin/ujail -a -e /etc/capabilities/wpad.json ] && { > > + procd_add_jail netifd > > + procd_set_param capabilities /etc/capabilities/wpad.json > > + procd_set_param user network > > + procd_set_param group network > > + procd_set_param no_new_privs 1 > > + } > > procd_close_instance > > fi > > if [ -x "/usr/sbin/wpa_supplicant" ]; then > > mkdir -p /var/run/wpa_supplicant > > + chown network:network /var/run/wpa_supplicant > > procd_open_instance supplicant > > procd_set_param command /usr/sbin/wpa_supplicant -n -s -g > > /var/run/wpa_supplicant/global > > procd_set_param respawn > > + [ -x /sbin/ujail -a -e /etc/capabilities/wpad.json ] && { > > + procd_add_jail netifd > > + procd_set_param capabilities /etc/capabilities/wpad.json > > + procd_set_param user network > > + procd_set_param group network > > + procd_set_param no_new_privs 1 > > + } > > procd_close_instance > > fi > > } > > diff --git a/package/network/services/hostapd/files/wpad.json > > b/package/network/services/hostapd/files/wpad.json > > new file mode 100644 > > index 0000000000..ae3edd4fea > > --- /dev/null > > +++ b/package/network/services/hostapd/files/wpad.json > > @@ -0,0 +1,22 @@ > > +{ > > + "bounding": [ > > + "CAP_NET_RAW", > > + "CAP_NET_ADMIN" > > + ], > > + "effective": [ > > + "CAP_NET_RAW", > > + "CAP_NET_ADMIN" > > + ], > > + "ambient": [ > > + "CAP_NET_RAW", > > + "CAP_NET_ADMIN" > > + ], > > + "permitted": [ > > + "CAP_NET_RAW", > > + "CAP_NET_ADMIN" > > + ], > > + "inheritable": [ > > + "CAP_NET_RAW", > > + "CAP_NET_ADMIN" > > + ] > > +} > > diff --git a/package/network/services/hostapd/files/wpad_acl.json > > b/package/network/services/hostapd/files/wpad_acl.json > > new file mode 100644 > > index 0000000000..c77ccd8ea0 > > --- /dev/null > > +++ b/package/network/services/hostapd/files/wpad_acl.json > > @@ -0,0 +1,10 @@ > > +{ > > + "user": "network", > > + "access": { > > + "service": { > > + "methods": [ "event" ] > > + } > > + }, > > + "publish": [ "hostapd", "hostapd.*", "wpa_supplicant", > > "wpa_supplicant.*" ], > > + "send": [ "bss.*", "wps_credentials" ] > > +} > > diff --git a/package/network/services/hostapd/src/src/ap/ubus.c > > b/package/network/services/hostapd/src/src/ap/ubus.c > > index 8546d2ce69..e59db00bd3 100644 > > --- a/package/network/services/hostapd/src/src/ap/ubus.c > > +++ b/package/network/services/hostapd/src/src/ap/ubus.c > > @@ -690,6 +690,21 @@ hostapd_config_remove(struct ubus_context *ctx, struct > > ubus_object *obj, > > return UBUS_STATUS_OK; > > } > > +static int > > +hostapd_getpid(struct ubus_context *ctx, struct ubus_object *obj, > > + struct ubus_request_data *req, const char *method, > > + struct blob_attr *msg) > > +{ > > + That newline is not supposed to be here now that I see it. Fixed. > > + blob_buf_init(&b, 0); > > + > > + blobmsg_add_u32(&b, "pid", getpid()); > > + > > + ubus_send_reply(ctx, req, b.head); > > + > > + return UBUS_STATUS_OK; > > +} > > + > > enum { > > CSA_FREQ, > > CSA_BCN_COUNT, > > @@ -1363,6 +1378,7 @@ void hostapd_ubus_free_bss(struct hostapd_data *hapd) > > static const struct ubus_method daemon_methods[] = { > > UBUS_METHOD("config_add", hostapd_config_add, config_add_policy), > > UBUS_METHOD("config_remove", hostapd_config_remove, > > config_remove_policy), > > + UBUS_METHOD_NOARG("getpid", hostapd_getpid), > > }; > > static struct ubus_object_type daemon_object_type = > > diff --git a/package/network/services/hostapd/src/wpa_supplicant/ubus.c > > b/package/network/services/hostapd/src/wpa_supplicant/ubus.c > > index 4bb92a7b66..f9fa1645d7 100644 > > --- a/package/network/services/hostapd/src/wpa_supplicant/ubus.c > > +++ b/package/network/services/hostapd/src/wpa_supplicant/ubus.c > > @@ -311,9 +311,25 @@ wpas_config_remove(struct ubus_context *ctx, struct > > ubus_object *obj, > > return UBUS_STATUS_OK; > > } > > +static int > > +wpas_getpid(struct ubus_context *ctx, struct ubus_object *obj, > > + struct ubus_request_data *req, const char *method, > > + struct blob_attr *msg) > > +{ > > + Removed that newline as well. > > + blob_buf_init(&b, 0); > > + > > + blobmsg_add_u32(&b, "pid", getpid()); > > + > > + ubus_send_reply(ctx, req, b.head); > > + > > + return UBUS_STATUS_OK; > > +} > > + > > Can we unify these two functions wpas_getpid() and hostapd_getpid()? They do > the same thing and when we use wpad they should be both in the same binary. > > I think this is not needed now, but would be nice. > I agree, but well, you know the code, it wouldn't exactly be trivial to do that. As that function is really really small I didn't bother diving down that rabbit hole... > > static const struct ubus_method wpas_daemon_methods[] = { > > UBUS_METHOD("config_add", wpas_config_add, wpas_config_add_policy), > > UBUS_METHOD("config_remove", wpas_config_remove, > > wpas_config_remove_policy), > > + UBUS_METHOD_NOARG("getpid", wpas_getpid), > > }; > > static struct ubus_object_type wpas_daemon_object_type = > > _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel