Re: git: 2c24ad3377a6 - main - ifconfig: abort if loading a module fails other than for ENOENT
Date: Fri, 20 Jan 2023 13:24:26 UTC
On Fri, Jan 20, 2023 at 11:04:33AM +0000, Alexander Chernikov wrote:
>
>
> > On 19 Jan 2023, at 17:11, Alan Somers <asomers@freebsd.org> wrote:
> >
> > On Thu, Jan 19, 2023 at 7:03 AM Danilo G. Baio <dbaio@freebsd.org> wrote:
> >>
> >>
> >>
> >> On Mon, Jan 9, 2023, at 15:57, Alan Somers wrote:
> >>> The branch main has been updated by asomers:
> >>>
> >>> URL:
> >>> https://cgit.FreeBSD.org/src/commit/?id=2c24ad3377a6f584e484656db8390e4eb7cfc119
> >>>
> >>> commit 2c24ad3377a6f584e484656db8390e4eb7cfc119
> >>> Author: Alan Somers <asomers@FreeBSD.org>
> >>> AuthorDate: 2022-12-26 02:06:21 +0000
> >>> Commit: Alan Somers <asomers@FreeBSD.org>
> >>> CommitDate: 2023-01-10 02:56:18 +0000
> >>>
> >>> ifconfig: abort if loading a module fails other than for ENOENT
> >>>
> >>> If "ifconfig create" tries to load a kernel module, and the module
> >>> exists but can't be loaded, fail the command with a useful error
> >>> message. This is helpful, for example, when trying to create a cloned
> >>> interface in a vnet jail. But ignore ENOENT, because sometimes ifconfig
> >>> can't correctly guess the name of the required kernel module.
> >>>
> >>> MFC after: 2 weeks
> >>> Reviewed by: jhb
> >>> Differential Revision: https://reviews.freebsd.org/D37873
> >>> ---
> >>> sbin/ifconfig/ifconfig.c | 18 +++++++++++++-----
> >>> 1 file changed, 13 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/sbin/ifconfig/ifconfig.c b/sbin/ifconfig/ifconfig.c
> >>> index 462d543125c4..120207a6927e 100644
> >>> --- a/sbin/ifconfig/ifconfig.c
> >>> +++ b/sbin/ifconfig/ifconfig.c
> >>> @@ -1719,11 +1719,19 @@ ifmaybeload(const char *name)
> >>> }
> >>> }
> >>>
> >>> - /*
> >>> - * Try to load the module. But ignore failures, because ifconfig can't
> >>> - * infer the names of all drivers (eg mlx4en(4)).
> >>> - */
> >>> - (void) kldload(ifkind);
> >>> + /* Try to load the module. */
> >>> + if (kldload(ifkind) < 0) {
> >>> + switch (errno) {
> >>> + case ENOENT:
> >>> + /*
> >>> + * Ignore ENOENT, because ifconfig can't infer the
> >>> + * names of all drivers (eg mlx4en(4)).
> >>> + */
> >>> + break;
> >>> + default:
> >>> + err(1, "kldload(%s)", ifkind);
> >>> + }
> >>> + }
> >>> }
> >>>
> >>> static struct cmd basic_cmds[] = {
> >>
> >>
> >> Hi.
> >>
> >> I have a jail with vnet where the interface is renamed that stopped working after this.
> >>
> >> from inside the jail:
> >>
> >> root@test:/ # ifconfig
> >> lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
> >> options=680003<RXCSUM,TXCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
> >> inet6 ::1 prefixlen 128
> >> inet6 fe80::1%lo0 prefixlen 64 scopeid 0x10
> >> inet 127.0.0.1 netmask 0xff000000
> >> groups: lo
> >> nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
> >> vnet0b_test: flags=8862<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
> >> options=8<VLAN_MTU>
> >> ether 02:27:72:a7:28:0b
> >> groups: epair
> >> media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
> >> status: active
> >> nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
> >>
> >> root@test:/ # ifconfig vnet0b_test
> >> ifconfig: kldload(if_vnet): Operation not permitted
> >>
> >>
> >> If I don't rename the interface, that works.
> >>
> >> jail.conf:
> >>
> >> test {
> >> vnet;
> >> $index = "0";
> >> vnet.interface = "vnet${index}b_${name}";
> >> exec.prestart += "ifconfig epair${index} create";
> >> exec.prestart += "ifconfig ${bridge} addm epair${index}a";
> >> exec.prestart += "ifconfig epair${index}a up name vnet${index}a_${name}";
> >> exec.prestart += "ifconfig epair${index}b up name vnet${index}b_${name}";
> >> exec.poststop += "ifconfig ${bridge} deletem vnet${index}a_${name}";
> >> exec.poststop += "ifconfig vnet${index}a_${name} destroy";
> >> devfs_ruleset = "11"; # add path 'bpf*' unhide (devfs.rules)
> >> }
> >>
> >> That's a bit odd, I know, could be using description instead.
> >>
> >> Just reporting.
> >>
> >> Regards.
> >> --
> >> Danilo G. Baio
> >
> > Ugh, it looks like kldload(2) is doing the privilege check before the
> > file existence check. I'm not sure of the best solution:
> > * Change kern_kldload to check for file existence first. This would
> > ring some alarm bells among security folks, and it isn't totally easy
> > to do, either.
> > * Change ifconfig(8) to do an existence check of its own. This would be ugly.
> > * Change ifconfig(8) so that it doesn't attempt to load modules when
> > just listing an interface. This might be incomplete, but is probably
> > worth doing anyway.
> I think another question is that if if should be done by ifconfig(8) at all. Kernel can take care of trying to load the required modules, checking the privileges.
> I’m considering adding such code for the netlink-based interface creation.
An interesting problem unique to HardenedBSD is that since the kld*
syscalls are hardened such that unprivileged users cannot use them at
all (so kldfind(2)/kldstat(8) are completely nonfunctional), this
breaks even read-only operations with ifconfig when specifying the
interface. Meaning, `ifconfig` works, but `ifconfig em0` does not,
when run as an unprivileged user.
I'm of the opinion that read-only operations (like `ifconfig em0`)
should be read-only in every sense. Kernel state should be preserved
unmodified.
The change I made in HardenedBSD is rather simple: force -n to be
enabled by default for all cases. Though, I don't think that's likely
the right solution for FreeBSD. It seems natural that FreeBSD would
want to take a more permissive route.
https://git.hardenedbsd.org/hardenedbsd/HardenedBSD/-/commit/671eb92efc2c9eef485194e443f7fa8102b2fe97
Thanks,
--
Shawn Webb
Cofounder / Security Engineer
HardenedBSD
https://git.hardenedbsd.org/hardenedbsd/pubkeys/-/raw/master/Shawn_Webb/03A4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc