Possible null deref on pf.c
Hi, This was reported on CID 1501718, ifp starts as NULL and then might be deref'ed. The question is does the below make any sense to solve it since I don't know what I'm doing? :) What do you net gurus say? Index: pf.c === RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.1108 diff -u -p -u -r1.1108 pf.c --- pf.c4 Feb 2021 00:55:41 - 1.1108 +++ pf.c12 Feb 2021 11:52:31 - @@ -6156,6 +6156,10 @@ pf_route6(struct pf_pdesc *pd, struct pf dst->sin6_addr = s->rt_addr.v6; rtableid = m0->m_pkthdr.ph_rtableid; + ifp = if_get(rt->rt_ifidx); + if (ifp == NULL) + goto bad; + if (IN6_IS_SCOPE_EMBED(>sin6_addr)) dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index); rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid); @@ -6168,10 +6172,6 @@ pf_route6(struct pf_pdesc *pd, struct pf ip6stat_inc(ip6s_noroute); goto bad; } - - ifp = if_get(rt->rt_ifidx); - if (ifp == NULL) - goto bad; /* A locally generated packet may have invalid source address. */ if (IN6_IS_ADDR_LOOPBACK(>ip6_src) &&
Add missing break statement on if_rge.c
Hi, Add missing break statement. OK? CID 1501710 Index: if_rge.c === RCS file: /cvs/src/sys/dev/pci/if_rge.c,v retrieving revision 1.11 diff -u -p -c -u -r1.11 if_rge.c --- if_rge.c24 Dec 2020 06:34:03 - 1.11 +++ if_rge.c11 Feb 2021 12:21:33 - @@ -311,6 +311,7 @@ rge_activate(struct device *self, int ac #ifndef SMALL_KERNEL rge_wol_power(sc); #endif + break; default: rv = config_activate_children(self, act); break;
Uninitialized var in dev/pv/vmt.c
Hi, Uninitialized var and it's used in a condition != NULL a little bit afterwards. CID 1501713 OK? Index: vmt.c === RCS file: /cvs/src/sys/dev/pv/vmt.c,v retrieving revision 1.22 diff -u -p -u -r1.22 vmt.c --- vmt.c 15 Jan 2021 06:14:41 - 1.22 +++ vmt.c 11 Feb 2021 11:35:41 - @@ -1289,7 +1289,7 @@ vmt_xdr_nic_info(char *data) struct ifnet *iface; struct vm_nicinfo_nic_list nl; size_t total, nictotal; - char *listdata; + char *listdata = NULL; int nics; NET_ASSERT_LOCKED();
Re: Swapped arguments on stoeplitz_ip{4,6}port
forget about it, patrick@ was way faster and it's already commited :)
Swapped arguments on stoeplitz_ip{4,6}port
Hi,
It seems this got the args swapped as described in CID 1501717.
stoeplitz_ip6port being a macro for stoeplitz_hash_ip6port and the arguments
placed as shown below. Similarly stoeplitz_ip4port also has the same problem
most likely due to copypasto.
Comments? OK?
173 uint16_t
174 stoeplitz_hash_ip6port(const struct stoeplitz_cache *scache,
175 const struct in6_addr *faddr6, const struct in6_addr *laddr6,
176 in_port_t fport, in_port_t lport)
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.253
diff -u -p -u -r1.253 in_pcb.c
--- netinet/in_pcb.c25 Jan 2021 03:40:46 - 1.253
+++ netinet/in_pcb.c11 Feb 2021 10:34:50 -
@@ -522,8 +522,8 @@ in_pcbconnect(struct inpcb *inp, struct
inp->inp_fport = sin->sin_port;
in_pcbrehash(inp);
#if NSTOEPLITZ > 0
- inp->inp_flowid = stoeplitz_ip4port(inp->inp_laddr.s_addr,
- inp->inp_faddr.s_addr, inp->inp_lport, inp->inp_fport);
+ inp->inp_flowid = stoeplitz_ip4port(inp->inp_faddr.s_addr,
+ inp->inp_laddr.s_addr, inp->inp_fport, inp->inp_lport);
#endif
#ifdef IPSEC
{
Index: netinet6/in6_pcb.c
===
RCS file: /cvs/src/sys/netinet6/in6_pcb.c,v
retrieving revision 1.111
diff -u -p -u -r1.111 in6_pcb.c
--- netinet6/in6_pcb.c 25 Jan 2021 03:40:47 - 1.111
+++ netinet6/in6_pcb.c 11 Feb 2021 10:34:50 -
@@ -303,8 +303,8 @@ in6_pcbconnect(struct inpcb *inp, struct
inp->inp_flowinfo |=
(htonl(ip6_randomflowlabel()) & IPV6_FLOWLABEL_MASK);
#if NSTOEPLITZ > 0
- inp->inp_flowid = stoeplitz_ip6port(>inp_laddr6,
- >inp_faddr6, inp->inp_lport, inp->inp_fport);
+ inp->inp_flowid = stoeplitz_ip6port(>inp_faddr6,
+ >inp_laddr6, inp->inp_fport, inp->inp_lport);
#endif
in_pcbrehash(inp);
return (0);
Re: Hiding the vim user in amdgpu_vcn.c (typo in comment)
Hi, This issue is present in upstream [0], please take it with them. [0] https://sourcegraph.com/github.com/torvalds/linux@30bb5572ce7a8710fa9ea720b6ecb382791c9800/-/blob/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c#L131 On 13:21 Tue 05 Jan , Stefan Hagen wrote: > Hi, > > I can totally relate to this one. > > Found after a conversation about muscle memory and grepping the source > tree for ":wq". > > Best Regards, > Stefan > > Index: ./sys/dev/pci/drm/amd/amdgpu/amdgpu_vcn.c > === > RCS file: /cvs/src/sys/dev/pci/drm/amd/amdgpu/amdgpu_vcn.c,v > retrieving revision 1.5 > diff -u -p -u -p -r1.5 amdgpu_vcn.c > --- ./sys/dev/pci/drm/amd/amdgpu/amdgpu_vcn.c 8 Jun 2020 04:47:59 - > 1.5 > +++ ./sys/dev/pci/drm/amd/amdgpu/amdgpu_vcn.c 5 Jan 2021 12:13:51 - > @@ -128,7 +128,7 @@ int amdgpu_vcn_sw_init(struct amdgpu_dev > > /* Bit 20-23, it is encode major and non-zero for new naming convention. >* This field is part of version minor and DRM_DISABLED_FLAG in old > naming > - * convention. Since the l:wq!atest version minor is 0x5B and > DRM_DISABLED_FLAG > + * convention. Since the latest version minor is 0x5B and > DRM_DISABLED_FLAG >* is zero in old naming convention, this field is always zero so far. >* These four bits are used to tell which naming convention is present. >*/ >
Re: panic on inteldrm attach in braswell device
the machine is still not properly setup and I didn't want to "pollute" bugs@ without a proper sendbug(1), sorry about that. that being said just tested booting the machine without the hdmi cable to stop inteldrm from attaching, but after everything is booted then attaching the cable makes inteldrm attach cleanly and the machine doesn't panic. I intend to use this as an headless router anyway so if it works this way I'm fine with it. [...] vscsi0 at root scsibus2 at vscsi0: 256 targets softraid0 at root scsibus3 at softraid0: 256 targets root on sd0a (6d2c4982bb37fa0d.a) swap on sd0b dump on sd0b inteldrm0: 1024x768, 32bpp wsdisplay0 at inteldrm0 mux 1 wskbd0: connecting to wsdisplay0 wsdisplay0: screen 0-5 added (std, vt100 emulation) > reports like this should go to bugs@ not tech > > Most of the pool use in drm is quite mechanical conversions of > kmem_cache*. The only suspect thing I can spot is in a detach path, > which isn't involved if a wsdisplay is being attached. > > I can't reproduce this here on a braswell system with the same > kernel from snapshots or a locally built kernel. >
panic on inteldrm attach in braswell device
Hi,
Since my edgerouter decided to commit seppuku, then to replace it, the cheap
bastard in me bought a crappy braswell based device which after 2 months
finally arrived, but of course it had to have at least one problem.
As soon as inteldrm attach I get the below panic, but with inteldrm disabled
I'm able to boot and use the system without issues:
wsdisplay0 at inteldrm0 mux 1: console (std, vt100 emulation), using wskbd0
wsdisplay0: screen 1-5 added (std, vt100 emulation)
kernel: protection fault trap, code=0
Stopped at pool_do_put+0xd0movq0x8(%rcx),%rcx
This is the trace without function params, typed manually since this box
doesn't have serial console, but link [0] has them. Any clues about this
or should I just dump it in the fire?
[0] https://imgur.com/a/O5PFQpz
ddb{1}> trace
pool_do_put() at pool_do_put+0xd0
pool_put() at pool_put+0x5a
uvm_unmap_detach() at uvm_unmap_detach+0x90
uvm_map() at uvm_map+0x7fb
km_alloc() at km_alloc+0x162
pool_multi_alloc_ni at pool_multi_alloc_ni+0xa6
pool_p_alloc() at pool_p_alloc+0x5a
pool_do_get() at pool_do_get+0xd3
pool_get() at pool_get+0x8f
ufsdirhash_build at ufsdirhash_build+0x314
ufs_lookup() at ufs_lookup+0x18a
VOP_LOOKUP() at VOP_LOOKUP+0x46
vfs_lookup() at vfs_lookup+0x3d2
namei() at namei+0x3a5
start_init() at start_init+0xaa
end trace frame: 0x0, count: -15
And finally the dmesg:
OpenBSD 6.7-current (GENERIC.MP) #337: Wed Jul 8 10:37:10 MDT 2020
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4175974400 (3982MB)
avail mem = 4034359296 (3847MB)
random: good seed from bootblocks
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 3.0 @ 0xea540 (51 entries)
bios0: vendor American Megatrends Inc. version "5.11" date 06/23/2016
bios0: Default string Default string
acpi0 at bios0: ACPI 5.0
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP APIC FPDT FIDT MCFG SSDT SSDT SSDT UEFI LPIT BGRT CSRT
acpi0: wakeup devices BRC1(S0) XHC1(S4) HDEF(S4) RP01(S4) PXSX(S4) RP02(S4)
PXSX(S4) RP03(S4) PXSX(S4) RP04(S4) PXSX(S4)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Celeron(R) CPU N3150 @ 1.60GHz, 1600.44 MHz, 06-4c-03
cpu0:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,TSC_ADJUST,SMEP,ERMS,MD_CLEAR,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN
cpu0: 1MB 64b/line 16-way L2 cache
cpu0: TSC skew=0 observed drift=0
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 80MHz
cpu0: mwait min=64, max=64, C-substates=0.2.0.0.0.0.3.3, IBE
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Celeron(R) CPU N3150 @ 1.60GHz, 1600.01 MHz, 06-4c-03
cpu1:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,TSC_ADJUST,SMEP,ERMS,MD_CLEAR,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN
cpu1: 1MB 64b/line 16-way L2 cache
cpu1: TSC skew=-10 observed drift=0
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 4 (application processor)
cpu2: Intel(R) Celeron(R) CPU N3150 @ 1.60GHz, 1600.03 MHz, 06-4c-03
cpu2:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,TSC_ADJUST,SMEP,ERMS,MD_CLEAR,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN
cpu2: 1MB 64b/line 16-way L2 cache
cpu2: TSC skew=0 observed drift=0
cpu2: smt 0, core 2, package 0
cpu3 at mainbus0: apid 6 (application processor)
cpu3: Intel(R) Celeron(R) CPU N3150 @ 1.60GHz, 1600.02 MHz, 06-4c-03
cpu3:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,SSE4.1,SSE4.2,MOVBE,POPCNT,DEADLINE,AES,RDRAND,NXE,RDTSCP,LONG,LAHF,3DNOWP,PERF,ITSC,TSC_ADJUST,SMEP,ERMS,MD_CLEAR,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN
cpu3: 1MB 64b/line 16-way L2 cache
cpu3: TSC skew=10 observed drift=0
cpu3: smt 0, core 3, package 0
ioapic0 at mainbus0: apid 1 pa 0xfec0, version 20, 115 pins
acpimcfg0 at acpi0
acpimcfg0: addr 0xe000, bus 0-255
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 1 (RP01)
acpiprt2 at acpi0: bus 2 (RP02)
acpiprt3 at acpi0: bus 3 (RP03)
acpiprt4 at acpi0: bus -1 (RP04)
acpiec0 at acpi0: not present
acpicpu0 at acpi0: C1(@1 halt!)
acpicpu1 at acpi0: C1(@1 halt!)
acpicpu2 at acpi0: C1(@1 halt!)
Re: ldpd engine process exits with pledge "cpath"
this is getting in my nerves, I made a wrong assumption here and I
prefer to commit this than backout my previous commit so I'm asking for
commits for the below.
On 14:43 Fri 19 Jun , Ricardo Mestre wrote:
> mea culpa, but I'd rather just remove the unlink of the socket.
>
> OK?
>
> Index: control.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 control.c
> --- control.c 3 Mar 2017 23:30:57 - 1.29
> +++ control.c 19 Jun 2020 13:40:46 -
> @@ -98,11 +98,10 @@ control_listen(void)
> }
>
> void
> -control_cleanup(char *path)
> +control_cleanup(void)
> {
> accept_del(control_fd);
> close(control_fd);
> - unlink(path);
> }
>
> /* ARGSUSED */
> Index: control.h
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 control.h
> --- control.h 3 Mar 2017 23:30:57 - 1.9
> +++ control.h 19 Jun 2020 13:40:46 -
> @@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
>
> int control_init(char *);
> int control_listen(void);
> -void control_cleanup(char *);
> +void control_cleanup(void);
> int control_imsg_relay(struct imsg *);
>
> #endif /* _CONTROL_H_ */
> Index: ldpe.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> retrieving revision 1.76
> diff -u -p -u -r1.76 ldpe.c
> --- ldpe.c10 Aug 2019 01:30:53 - 1.76
> +++ ldpe.c19 Jun 2020 13:40:46 -
> @@ -171,7 +171,7 @@ ldpe_shutdown(void)
> msgbuf_clear(_main->ibuf.w);
> close(iev_main->ibuf.fd);
>
> - control_cleanup(global.csock);
> + control_cleanup();
> config_clear(leconf);
>
> if (sysdep.no_pfkey == 0) {
>
Re: ldpd engine process exits with pledge "cpath"
mea culpa, but I'd rather just remove the unlink of the socket.
OK?
Index: control.c
===
RCS file: /cvs/src/usr.sbin/ldpd/control.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 control.c
--- control.c 3 Mar 2017 23:30:57 - 1.29
+++ control.c 19 Jun 2020 13:40:46 -
@@ -98,11 +98,10 @@ control_listen(void)
}
void
-control_cleanup(char *path)
+control_cleanup(void)
{
accept_del(control_fd);
close(control_fd);
- unlink(path);
}
/* ARGSUSED */
Index: control.h
===
RCS file: /cvs/src/usr.sbin/ldpd/control.h,v
retrieving revision 1.9
diff -u -p -u -r1.9 control.h
--- control.h 3 Mar 2017 23:30:57 - 1.9
+++ control.h 19 Jun 2020 13:40:46 -
@@ -32,7 +32,7 @@ extern struct ctl_conns ctl_conns;
intcontrol_init(char *);
intcontrol_listen(void);
-void control_cleanup(char *);
+void control_cleanup(void);
intcontrol_imsg_relay(struct imsg *);
#endif /* _CONTROL_H_ */
Index: ldpe.c
===
RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
retrieving revision 1.76
diff -u -p -u -r1.76 ldpe.c
--- ldpe.c 10 Aug 2019 01:30:53 - 1.76
+++ ldpe.c 19 Jun 2020 13:40:46 -
@@ -171,7 +171,7 @@ ldpe_shutdown(void)
msgbuf_clear(_main->ibuf.w);
close(iev_main->ibuf.fd);
- control_cleanup(global.csock);
+ control_cleanup();
config_clear(leconf);
if (sysdep.no_pfkey == 0) {
unveil(2) relayd(8)'s main proc, now for real
Hi,
Yes, this is a really broad permission to give but it's needed in order to read
the config file (and those ones included from it) and also to exec the "check
script(s)" which I missed in my last attempt to unveil(2) relayd(8).
The reason it cannot be pledge(2)d is due to forbidden ioctls(2)s related to
carp(4).
This permits reading or execing anything from the filesystem but at least
prevents create/write/delete files and regress tests still pass.
Comments? OK?
Index: relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.182
diff -u -p -u -r1.182 relayd.c
--- relayd.c15 Sep 2019 19:23:29 - 1.182
+++ relayd.c18 Jun 2020 22:19:50 -
@@ -223,6 +223,11 @@ main(int argc, char *argv[])
if (ps->ps_noaction == 0)
log_info("startup");
+ if (unveil("/", "rx") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
event_init();
signal_set(>ps_evsigint, SIGINT, parent_sig_handler, ps);
unveil(2) login_chpass(8)
Hi,
login_chpass(8) only needs access to exec login_lchpass(8), so the below
unveils it accordingly.
Comments? OK?
Index: login_chpass.c
===
RCS file: /cvs/src/libexec/login_chpass/login_chpass.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 login_chpass.c
--- login_chpass.c 26 Apr 2018 12:42:51 - 1.21
+++ login_chpass.c 18 Jun 2020 20:53:52 -
@@ -60,6 +60,8 @@ main(int argc, char *argv[])
(void)setpriority(PRIO_PROCESS, 0, 0);
+ if (unveil(_PATH_LOGIN_LCHPASS, "x") == -1)
+ err(1, "unveil");
if (pledge("stdio exec", NULL) == -1)
err(1, "pledge");
Re: [PATCH] from.c: stricter pledge(2)
this is ok mestre@ may I commit it?
On 21:18 Thu 28 May , Martin Vahlensieck wrote:
> Hey!
>
> This pledge was added with the use of unveil(2), but doesn't require the
> getpw promise anymore (it is only needed in mail_spool to get the
> username).
>
> This patch makes it stricter.
>
> Best,
>
> Martin
>
> Index: from.c
> ===
> RCS file: /cvs/src/usr.bin/from/from.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 from.c
> --- from.c8 Aug 2018 17:52:46 - 1.26
> +++ from.c24 May 2020 12:01:06 -
> @@ -81,7 +81,7 @@ main(int argc, char *argv[])
>
> if (unveil(file, "r") == -1)
> err(1, "unveil");
> - if (pledge("stdio rpath getpw", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
> err(1, "pledge");
>
> if ((fp = fopen(file, "r")) == NULL) {
>
Re: pledge(2) sndioctl(1)
Hello,
I tried to open the raw device but now it seems I was to sleepy to
figure out that I couldn't access it due to sndiod(8) having the device
opened earlier and therefore coundn't reach that code path.
Here's the audio promise added, but maybe it raises the question again
if these utilities should have direct access to the devices, and also
includes sndiod(8) not running when this is done?
ratchov@ will this change eventually? Otherwise the below keeps that
code path happy.
Index: sndioctl.c
===
RCS file: /cvs/src/usr.bin/sndioctl/sndioctl.c,v
retrieving revision 1.10
diff -u -p -u -r1.10 sndioctl.c
--- sndioctl.c 17 May 2020 05:39:32 - 1.10
+++ sndioctl.c 22 May 2020 07:01:30 -
@@ -948,6 +948,13 @@ main(int argc, char **argv)
fprintf(stderr, "%s: can't open control device\n", devname);
exit(1);
}
+
+ if (pledge("stdio audio", NULL) == -1) {
+ fprintf(stderr, "%s: pledge: %s\n", getprogname(),
+ strerror(errno));
+ exit(1);
+ }
+
if (!sioctl_ondesc(hdl, ondesc, NULL)) {
fprintf(stderr, "%s: can't get device description\n", devname);
exit(1);
On 08:25 Fri 22 May , Sebastien Marie wrote:
> On Fri, May 22, 2020 at 06:57:00AM +0200, Sebastien Marie wrote:
> > On Thu, May 21, 2020 at 11:07:39PM +0100, Ricardo Mestre wrote:
> > > Hi,
> > >
> > > After the handle sioctl_hdl `hdl' is opened (which in itself requires rw
> > > fs
> > > access and opening an unix socket) then all operations happen over that
> > > handle
> > > so the program may be restricted to only "stdio".
> > >
> > > All options were tested successfully, including the examples from the
> > > manpage
> > > plus tweaking the audio from an app ($MYBROWSER).
> >
> > Did you only perform runtime checking ? or also auditing the code source ?
> >
> > Because depending your hardware and the way sndiod is configured, it isn't
> > necessary the same code path used.
> >
> > Just one example: with device "snd/0", the sioctl_open() will use
> > _sioctl_aucat_open() for handler initialisation, whereas with "rsnd/0", it
> > is
> > _sioctl_sun_open().
> >
> > As all functions called after sioctl_open() are using the handler, you might
> > have tested only a part of the code path.
> >
> > I didn't look deeper. So it might fine, but it could also break some
> > configurations.
>
> I looked a bit deeper.
>
> commit()
> sioctl_setval()
>[with "rsnd/0"]
>sioctl_sun_setctl()
> setvol()
>ioctl(hdl->fd, AUDIO_MIXER_WRITE, )
>
> calling ioctl(AUDIO_MIXER_WRITE) should required "audio".
>
> whereas when using default "snd/0" device, it is calling sioctl_aucat_setctl()
> and it will write(2) a message to sndiod(8) (and "stdio" is enough).
>
> I agree that "rsnd/0" isn't the standard case (it requires root to read/write
> to
> /dev), but it means you will broke sndioctl(1) in such cases.
>
> Thanks.
> --
> Sebastien Marie
>
pledge(2) sndioctl(1)
Hi,
After the handle sioctl_hdl `hdl' is opened (which in itself requires rw fs
access and opening an unix socket) then all operations happen over that handle
so the program may be restricted to only "stdio".
All options were tested successfully, including the examples from the manpage
plus tweaking the audio from an app ($MYBROWSER).
Comments? OK?
Index: sndioctl.c
===
RCS file: /cvs/src/usr.bin/sndioctl/sndioctl.c,v
retrieving revision 1.10
diff -u -p -u -r1.10 sndioctl.c
--- sndioctl.c 17 May 2020 05:39:32 - 1.10
+++ sndioctl.c 21 May 2020 22:04:58 -
@@ -948,6 +948,13 @@ main(int argc, char **argv)
fprintf(stderr, "%s: can't open control device\n", devname);
exit(1);
}
+
+ if (pledge("stdio", NULL) == -1) {
+ fprintf(stderr, "%s: pledge: %s\n", getprogname(),
+ strerror(errno));
+ exit(1);
+ }
+
if (!sioctl_ondesc(hdl, ondesc, NULL)) {
fprintf(stderr, "%s: can't get device description\n", devname);
exit(1);
Fix typo on if_mcx.c
Hi, This one looks like a typo and might cause a double free. CID 1492713 OK? Index: if_mcx.c === RCS file: /cvs/src/sys/dev/pci/if_mcx.c,v retrieving revision 1.43 diff -u -p -u -r1.43 if_mcx.c --- if_mcx.c21 Apr 2020 05:49:25 - 1.43 +++ if_mcx.c23 Apr 2020 18:27:34 - @@ -6023,7 +6023,7 @@ mcx_up(struct mcx_softc *sc) return ENETRESET; destroy_tx_slots: mcx_free_slots(sc, sc->sc_tx_slots, i, (1 << MCX_LOG_SQ_SIZE)); - sc->sc_rx_slots = NULL; + sc->sc_tx_slots = NULL; i = (1 << MCX_LOG_RQ_SIZE); destroy_rx_slots:
Re: umb(4) WIP diff and questions
Hi, Disclaimer: I don't have such hardware to test with or without the diff below, but I think if we add this change in any shape or form then we should add this as well otherwise we could bump into the vuln [0] that Ilja found on NetBSD which could leak the credentials. [0] https://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2020-001.txt.asc Index: if_umb.c === RCS file: /cvs/src/sys/dev/usb/if_umb.c,v retrieving revision 1.31 diff -u -p -u -r1.31 if_umb.c --- if_umb.c26 Nov 2019 23:04:28 - 1.31 +++ if_umb.c21 Jan 2020 23:23:43 - @@ -699,6 +699,8 @@ umb_ioctl(struct ifnet *ifp, u_long cmd, usb_add_task(sc->sc_udev, >sc_umb_task); break; case SIOCGUMBINFO: + if ((error = suser(p)) != 0) + break; error = copyout(>sc_info, ifr->ifr_data, sizeof (sc->sc_info)); break; On 12:40 Tue 14 Jan , Theo de Raadt wrote: > Theo de Raadt wrote: > > > Stuart Henderson wrote: > > > > > On 2020/01/14 10:27, Theo de Raadt wrote: > > > > Unfortunate part of this diff is that the password is (very > > > > momentarily) visible with ps(1) in the root-run ifconfig argv[] array. > > > > It's a tight race, but still it is visible. > > > > > > > > People do run "sh /etc/netstart umb0" to activate the interface > > > > during multiuser. > > > > > > > > If the password is truly sensitive, it should be placed in a file, > > > > and the ifconfig's extension should be changed to read the file. > > > > > > That's not unique to umb though, it's been a problem forever with carp, > > > pppoe and especially wlan interfaces. > > > > When creating new versions of the same problem... that's a great time > > to reconsider the old solutions. > > > > > Another fix would be to accept > > > ifconfig options on stdin, which is more convenient for quick runtime > > > changes than two steps of writing to a file then pointing ifconfig at > > > it, and changing netstart to use it would improve things for existing > > > users without them needing to touch any config files. > > > > I think using the shell is silly, because what if there are two such > > options? Then you can't seperate the stdin. > > > > Additionally, most of the time when creating such temporary configuration, > > the pattern is that if it works you'll want to apply it permanent. > > > > Another thought is to create both versions of the option. One on the > > commandline, and one pointing at a file. > > > > Channeling a conversation from 15 years ago: "How about wpakeyfile" > > > Another consideration is... many of these passwords are locked to narrow > usage cases, so does it really matter all that much? >
Re: unveil radioctl/fdformat/gpioctl
Hello fellow citizens!
Did we have any brave souls with the hardware below that tested this and
can give me an OK?
On 18:57 Mon 02 Dec , Ricardo Mestre wrote:
> Hi tech@
>
> radioctl/fdformat/gpioctl need to open the device and then all operations go
> through ioctls forbidden by pledge but no further filesystem access is needed
> so it can be disallowed right afterwards.
>
> CAVEAT: The sources for these applications are simple enough to follow, but
> unfortunately I don't have any of these devices to actually test them, so take
> this with a really tiny pinch of salt.
>
> Comments, OK?
>
> /mestre
>
> Index: usr.bin/radioctl/radioctl.c
> ===
> RCS file: /cvs/src/usr.bin/radioctl/radioctl.c,v
> retrieving revision 1.20
> diff -u -p -u -r1.20 radioctl.c
> --- usr.bin/radioctl/radioctl.c 28 Jun 2019 13:35:03 - 1.20
> +++ usr.bin/radioctl/radioctl.c 2 Dec 2019 18:51:03 -
> @@ -186,6 +186,11 @@ main(int argc, char **argv)
> if (rd == -1)
> err(1, "%s open error", radiodev);
>
> + if (unveil("/", "") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> +
> if (ioctl(rd, RIOCGINFO, ) == -1)
> err(1, "RIOCGINFO");
>
> Index: usr.sbin/fdformat/fdformat.c
> ===
> RCS file: /cvs/src/usr.sbin/fdformat/fdformat.c,v
> retrieving revision 1.24
> diff -u -p -u -r1.24 fdformat.c
> --- usr.sbin/fdformat/fdformat.c 28 Jun 2019 13:32:47 - 1.24
> +++ usr.sbin/fdformat/fdformat.c 2 Dec 2019 18:51:04 -
> @@ -246,6 +246,11 @@ main(int argc, char *argv[])
> if ((fd = opendev(argv[optind], O_RDWR, OPENDEV_PART, )) == -1)
> err(1, "%s", devname);
>
> + if (unveil("/", "") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> +
> if (ioctl(fd, FD_GTYPE, ) == -1)
> errx(1, "not a floppy disk: %s", devname);
>
> Index: usr.sbin/gpioctl/gpioctl.c
> ===
> RCS file: /cvs/src/usr.sbin/gpioctl/gpioctl.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 gpioctl.c
> --- usr.sbin/gpioctl/gpioctl.c26 Dec 2015 20:52:03 - 1.17
> +++ usr.sbin/gpioctl/gpioctl.c2 Dec 2019 18:51:04 -
> @@ -101,6 +101,11 @@ main(int argc, char *argv[])
> if ((devfd = open(dev, O_RDWR)) == -1)
> err(1, "%s", dev);
>
> + if (unveil("/", "") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> +
> if (argc == 1) {
> getinfo();
> return 0;
>
unveil radioctl/fdformat/gpioctl
Hi tech@
radioctl/fdformat/gpioctl need to open the device and then all operations go
through ioctls forbidden by pledge but no further filesystem access is needed
so it can be disallowed right afterwards.
CAVEAT: The sources for these applications are simple enough to follow, but
unfortunately I don't have any of these devices to actually test them, so take
this with a really tiny pinch of salt.
Comments, OK?
/mestre
Index: usr.bin/radioctl/radioctl.c
===
RCS file: /cvs/src/usr.bin/radioctl/radioctl.c,v
retrieving revision 1.20
diff -u -p -u -r1.20 radioctl.c
--- usr.bin/radioctl/radioctl.c 28 Jun 2019 13:35:03 - 1.20
+++ usr.bin/radioctl/radioctl.c 2 Dec 2019 18:51:03 -
@@ -186,6 +186,11 @@ main(int argc, char **argv)
if (rd == -1)
err(1, "%s open error", radiodev);
+ if (unveil("/", "") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (ioctl(rd, RIOCGINFO, ) == -1)
err(1, "RIOCGINFO");
Index: usr.sbin/fdformat/fdformat.c
===
RCS file: /cvs/src/usr.sbin/fdformat/fdformat.c,v
retrieving revision 1.24
diff -u -p -u -r1.24 fdformat.c
--- usr.sbin/fdformat/fdformat.c28 Jun 2019 13:32:47 - 1.24
+++ usr.sbin/fdformat/fdformat.c2 Dec 2019 18:51:04 -
@@ -246,6 +246,11 @@ main(int argc, char *argv[])
if ((fd = opendev(argv[optind], O_RDWR, OPENDEV_PART, )) == -1)
err(1, "%s", devname);
+ if (unveil("/", "") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (ioctl(fd, FD_GTYPE, ) == -1)
errx(1, "not a floppy disk: %s", devname);
Index: usr.sbin/gpioctl/gpioctl.c
===
RCS file: /cvs/src/usr.sbin/gpioctl/gpioctl.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 gpioctl.c
--- usr.sbin/gpioctl/gpioctl.c 26 Dec 2015 20:52:03 - 1.17
+++ usr.sbin/gpioctl/gpioctl.c 2 Dec 2019 18:51:04 -
@@ -101,6 +101,11 @@ main(int argc, char *argv[])
if ((devfd = open(dev, O_RDWR)) == -1)
err(1, "%s", dev);
+ if (unveil("/", "") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (argc == 1) {
getinfo();
return 0;
Re: unveil(2) pcidump(8)
Of course I missed unveil(NULL, NULL)
Index: pcidump.c
===
RCS file: /cvs/src/usr.sbin/pcidump/pcidump.c,v
retrieving revision 1.55
diff -u -p -u -r1.55 pcidump.c
--- pcidump.c 28 Jun 2019 13:32:49 - 1.55
+++ pcidump.c 29 Nov 2019 15:22:15 -
@@ -188,6 +188,11 @@ main(int argc, char *argv[])
err(1, "%s", romfile);
}
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (hex > 1)
size = 256;
if (hex > 2)
On 15:01 Fri 29 Nov , Ricardo Mestre wrote:
> Hi,
>
> pcidump(8) only opens devices in O_RDONLY from /dev, and additionally writes a
> `romfile' if -r is used, but since I'm only unveiling after that file is
> actually opened there's no need to unveil it as well.
>
> All combination of parameters were tested, comments, OK?
Re: unveil(2) pcidump(8)
My emails are getting delayed, I sent another one with unveil(NULL,
NULL) right afterwards :)
On 10:05 Fri 29 Nov , Theo de Raadt wrote:
> Klemens Nanni wrote:
>
> > On Fri, Nov 29, 2019 at 03:01:45PM +0000, Ricardo Mestre wrote:
> > > All combination of parameters were tested, comments, OK?
> > OK kn
> >
> > > Index: pcidump.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/pcidump/pcidump.c,v
> > > retrieving revision 1.55
> > > diff -u -p -u -r1.55 pcidump.c
> > > --- pcidump.c 28 Jun 2019 13:32:49 - 1.55
> > > +++ pcidump.c 29 Nov 2019 14:54:32 -
> > > @@ -188,6 +188,9 @@ main(int argc, char *argv[])
> > > err(1, "%s", romfile);
> > > }
> > >
> > > + if (unveil("/dev", "r") == -1)
> > > + err(1, "unveil");
> > > +
> > There is no pledge call afterwards, so this unveil should be finalised.
>
> needs a unveil(NULL, NULL)
>
> pledge will not work. The code is full of ioctl.
>
> >
> > > if (hex > 1)
> > > size = 256;
> > > if (hex > 2)
> > >
> >
unveil(2) usbdevs(8)
Hi,
usbdevs(8) only needs to open devices in O_RDONLY mode from /dev
Comments? OK?
Index: usbdevs.c
===
RCS file: /cvs/src/usr.sbin/usbdevs/usbdevs.c,v
retrieving revision 1.31
diff -u -p -u -r1.31 usbdevs.c
--- usbdevs.c 14 Apr 2019 18:16:19 - 1.31
+++ usbdevs.c 29 Nov 2019 15:38:37 -
@@ -267,6 +267,11 @@ main(int argc, char **argv)
if (argc != 0)
usage();
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (dev == 0) {
for (ncont = 0, i = 0; i < 10; i++) {
snprintf(buf, sizeof buf, "%s%d", USBDEV, i);
unveil(2) pcidump(8)
Hi,
pcidump(8) only opens devices in O_RDONLY from /dev, and additionally writes a
`romfile' if -r is used, but since I'm only unveiling after that file is
actually opened there's no need to unveil it as well.
All combination of parameters were tested, comments, OK?
Index: pcidump.c
===
RCS file: /cvs/src/usr.sbin/pcidump/pcidump.c,v
retrieving revision 1.55
diff -u -p -u -r1.55 pcidump.c
--- pcidump.c 28 Jun 2019 13:32:49 - 1.55
+++ pcidump.c 29 Nov 2019 14:54:32 -
@@ -188,6 +188,9 @@ main(int argc, char *argv[])
err(1, "%s", romfile);
}
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+
if (hex > 1)
size = 256;
if (hex > 2)
Reduce pledge(2) on file(1)'s main proc
Hi,
After fork(2) the main proc needs rpath for {l,}stat/open and sendfd for imsg_*
to send fds to the child proc which is already pledged by recvfd to receive
them.
Still passes regress tests, OK?
Index: file.c
===
RCS file: /cvs/src/usr.bin/file/file.c,v
retrieving revision 1.68
diff -u -p -u -r1.68 file.c
--- file.c 5 Feb 2019 02:17:32 - 1.68
+++ file.c 29 Nov 2019 11:03:47 -
@@ -207,6 +207,9 @@ main(int argc, char **argv)
}
close(pair[1]);
+ if (pledge("stdio rpath sendfd", NULL) == -1)
+ err(1, "pledge");
+
fclose(magicfp);
magicfp = NULL;
Re: rad unveil
Hi benno,
If you remove "include" then please make the unveil next to pledge like
the below, that way we know right away why exactly we need rpath for in
pledge.
Also, you only need unveil(NULL, NULL) if you're not actually calling pledge
somewhere down the code.
Index: rad.c
===
RCS file: /cvs/src/usr.sbin/rad/rad.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 rad.c
--- rad.c 28 Jun 2019 13:32:49 - 1.21
+++ rad.c 27 Nov 2019 12:08:47 -
@@ -301,6 +301,8 @@ main(int argc, char *argv[])
main_imsg_compose_frontend_fd(IMSG_CONTROLFD, 0, control_fd);
main_imsg_send_config(main_conf);
+ if (unveil(conffile, "r") == -1)
+ fatal("unveil");
if (pledge("stdio rpath sendfd", NULL) == -1)
fatal("pledge");
On 19:22 Tue 26 Nov , Sebastian Benoit wrote:
> Sebastian Benoit(be...@openbsd.org) on 2019.11.26 18:46:11 +0100:
> >
> > remove include statement and unveil() rad.
> >
> > ok?
>
>
> diff --git usr.sbin/rad/parse.y usr.sbin/rad/parse.y
> index bb18c3d9c9c..443cff66065 100644
> --- usr.sbin/rad/parse.y
> +++ usr.sbin/rad/parse.y
> @@ -112,7 +112,7 @@ typedef struct {
>
> %}
>
> -%token RA_IFACE YES NO INCLUDE ERROR
> +%token RA_IFACE YES NO ERROR
> %token DEFAULT ROUTER HOP LIMIT MANAGED ADDRESS
> %token CONFIGURATION OTHER LIFETIME REACHABLE TIME RETRANS TIMER
> %token AUTO PREFIX VALID PREFERRED LIFETIME ONLINK AUTONOMOUS
> @@ -134,21 +134,6 @@ grammar : /* empty */
> | grammar error '\n'{ file->errors++; }
> ;
>
> -include : INCLUDE STRING{
> - struct file *nfile;
> -
> - if ((nfile = pushfile($2, 0)) == NULL) {
> - yyerror("failed to include file %s", $2);
> - free($2);
> - YYERROR;
> - }
> - free($2);
> -
> - file = nfile;
> - lungetc('\n');
> - }
> - ;
> -
> string : string STRING {
> if (asprintf(&$$, "%s %s", $1, $2) == -1) {
> free($1);
> @@ -428,7 +413,6 @@ lookup(char *s)
> {"default", DEFAULT},
> {"dns", DNS},
> {"hop", HOP},
> - {"include", INCLUDE},
> {"interface", RA_IFACE},
> {"lifetime",LIFETIME},
> {"limit", LIMIT},
> diff --git usr.sbin/rad/rad.c usr.sbin/rad/rad.c
> index 93675167b6b..3a79a08d3db 100644
> --- usr.sbin/rad/rad.c
> +++ usr.sbin/rad/rad.c
> @@ -296,6 +296,11 @@ main(int argc, char *argv[])
> if ((control_fd = control_init(csock)) == -1)
> fatalx("control socket setup failed");
>
> +if (unveil(conffile, "r") == -1)
> +err(1, "unveil");
> +if (unveil(NULL, NULL) == -1)
> +err(1, "unveil");
> +
> main_imsg_compose_frontend_fd(IMSG_ICMP6SOCK, 0, icmp6sock);
> main_imsg_compose_frontend_fd(IMSG_ROUTESOCK, 0, frontend_routesock);
> main_imsg_compose_frontend_fd(IMSG_CONTROLFD, 0, control_fd);
> diff --git usr.sbin/rad/rad.conf.5 usr.sbin/rad/rad.conf.5
> index d4ca72f4639..ea0cb95d77e 100644
> --- usr.sbin/rad/rad.conf.5
> +++ usr.sbin/rad/rad.conf.5
> @@ -50,10 +50,6 @@ sends IPv6 router advertisement messages.
> This section defines on which interfaces to advertise prefix information
> and their associated parameters.
> .El
> -.Pp
> -Additional configuration files can be included with the
> -.Ic include
> -keyword.
> .Sh MACROS
> Macros can be defined that will later be expanded in context.
> Macro names must start with a letter, digit, or underscore,
>
unveil vmd(8)'s priv process
Hi,
Currently vmd(8) has 3 processes that run under chroot(2)/chdir(2), namely
control, vmm and priv. From these both control and vmm already run under
different pledge(2)s but without any filesystem access, priv in the other hand
cannot use pledge due to forbidden ioctls.
That being said the priv proc can instead just run with an unveil("/", "") to
disable fs access, and while here remove the need of chroot/chdir dance since
it's not needed any longer.
I've been running this for more than a week now without issues. Any
comments/objections? OK?
/mestre
Index: priv.c
===
RCS file: /cvs/src/usr.sbin/vmd/priv.c,v
retrieving revision 1.15
diff -u -p -u -r1.15 priv.c
--- priv.c 28 Jun 2019 13:32:51 - 1.15
+++ priv.c 19 Aug 2019 09:38:51 -
@@ -66,8 +66,14 @@ priv_run(struct privsep *ps, struct priv
/*
* no pledge(2) in the "priv" process:
-* write ioctls are not permitted by pledge.
+* write ioctls are not permitted by pledge, but we can restrict fs
+* access with unveil
*/
+ /* no filesystem visibility */
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
/* Open our own socket for generic interface ioctls */
if ((env->vmd_fd = socket(AF_INET, SOCK_DGRAM, 0)) == -1)
Index: proc.c
===
RCS file: /cvs/src/usr.sbin/vmd/proc.c,v
retrieving revision 1.18
diff -u -p -u -r1.18 proc.c
--- proc.c 10 Sep 2018 10:36:01 - 1.18
+++ proc.c 19 Aug 2019 09:38:51 -
@@ -524,7 +524,6 @@ proc_run(struct privsep *ps, struct priv
void (*run)(struct privsep *, struct privsep_proc *, void *), void *arg)
{
struct passwd *pw;
- const char *root;
struct control_sock *rcs;
log_procinit(p->p_title);
@@ -545,17 +544,6 @@ proc_run(struct privsep *ps, struct priv
pw = p->p_pw;
else
pw = ps->ps_pw;
-
- /* Change root directory */
- if (p->p_chroot != NULL)
- root = p->p_chroot;
- else
- root = pw->pw_dir;
-
- if (chroot(root) == -1)
- fatal("%s: chroot", __func__);
- if (chdir("/") == -1)
- fatal("%s: chdir(\"/\")", __func__);
privsep_process = p->p_id;
Index: proc.h
===
RCS file: /cvs/src/usr.sbin/vmd/proc.h,v
retrieving revision 1.16
diff -u -p -u -r1.16 proc.h
--- proc.h 10 Sep 2018 10:36:01 - 1.16
+++ proc.h 19 Aug 2019 09:38:51 -
@@ -136,7 +136,6 @@ struct privsep_proc {
void(*p_init)(struct privsep *,
struct privsep_proc *);
void(*p_shutdown)(void);
- const char *p_chroot;
struct passwd *p_pw;
struct privsep *p_ps;
};
Index: vmd.c
===
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.115
diff -u -p -u -r1.115 vmd.c
--- vmd.c 14 Aug 2019 07:34:49 - 1.115
+++ vmd.c 19 Aug 2019 09:38:51 -
@@ -789,9 +789,8 @@ main(int argc, char **argv)
if ((ps->ps_pw = getpwnam(VMD_USER)) == NULL)
fatal("unknown user %s", VMD_USER);
- /* First proc runs as root without pledge but in default chroot */
+ /* First proc runs as root without pledge but with unveil */
proc_priv->p_pw = _privpw; /* initialized to all 0 */
- proc_priv->p_chroot = ps->ps_pw->pw_dir; /* from VMD_USER */
/* Open /dev/vmm */
if (env->vmd_noaction == 0) {
Index: vmm.c
===
RCS file: /cvs/src/usr.sbin/vmd/vmm.c,v
retrieving revision 1.93
diff -u -p -u -r1.93 vmm.c
--- vmm.c 28 Jun 2019 13:32:51 - 1.93
+++ vmm.c 19 Aug 2019 09:38:51 -
@@ -655,7 +655,7 @@ vmm_start_vm(struct imsg *imsg, uint32_t
if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, fds) == -1)
fatal("socketpair");
- /* Start child vmd for this VM (fork, chroot, drop privs) */
+ /* Start child vmd for this VM (fork, drop privs) */
ret = fork();
/* Start child failed? - cleanup and leave */
unveil(2) missing codepath on dhcpd(8)
Hi,
One missing piece when I added pledge(2) to dhcpd(8) was in the code path when
it's invoked with either -A/-C/-L, which at the time I left alone due to some
forbidden ioctls by pledge(2).
Now we have unveil(2) and this path can be further restricted by using it
instead of chroot(2) since this "sandbox" (not sure why people call sandbox to
about everything these days) can be escaped with *at(2) calls.
Since no filesystem access is needed here then we can disable its access by
calling unveil("/", "") unveil(NULL, NULL).
Comments? OK?
Index: pfutils.c
===
RCS file: /cvs/src/usr.sbin/dhcpd/pfutils.c,v
retrieving revision 1.20
diff -u -p -u -r1.20 pfutils.c
--- pfutils.c 28 Jun 2019 13:32:47 - 1.20
+++ pfutils.c 6 Aug 2019 13:28:11 -
@@ -54,14 +54,16 @@ pftable_handler()
if ((fd = open(_PATH_DEV_PF, O_RDWR|O_NOFOLLOW, 0660)) == -1)
fatal("can't open pf device");
- if (chroot(_PATH_VAREMPTY) == -1)
- fatal("chroot %s", _PATH_VAREMPTY);
- if (chdir("/") == -1)
- fatal("chdir(\"/\")");
+
if (setgroups(1, >pw_gid) ||
setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) ||
setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
fatal("can't drop privileges");
+
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
setproctitle("pf table handler");
l = sizeof(struct pf_cmd);
Re: remove chroot(2) from spamd(8)
As I already spoke with Theo this needs to be carefully looked app by app and not remove chroot just because, it needs to make sense first and foremost. This diff was also not one by random choice, spamd(8) was one of the first programs I actually studied, pledge(2)d it and use daily and sent it as a showcase for other programs where we could apply the same logic, but then again if it actually makes sense without getting into a personal vendetta of changing every single program that calls chroot(2). Regarding downstream I'm "not too worried" since if you follow us either you are already using OpenBSD or you should apply best policies already, one being porting both pledge/unveil but we all know it will takes years until someone goes ahead and do that, so that being said I'm worried on the other side if we do this and make the programs vulnerable to fs access where they shouldn't have. All things considered looking only at our own garden my diff makes sense so I'm asking for OKs specially from deraadt@ On 11:22 Wed 31 Jul , Theo de Raadt wrote: > Ingo Schwarze wrote: > > > /* > > * When porting this program to a platform lacking pledge(2), > > * don't forget to at least properly chroot(2) the child instead. > > */ > > I'm going to translate that to another plausible comment to put > throughout the source tree. > >/* When porting this program to a platform lacking strlcpy(3) > * don't forget to avoid buffer overflows with whatever replacement > * functions you use. > */ > > But I really don't see the benefit of such lines of text. > > Outsiders won't read the text, and it wastes our screen real estate. > > > The spamd(8) utility does look like a plausible candidate for porting, > > but i'm not sure how familiar the porters are with OpenBSD ways, > > so the removal of chroot(2) might easily cause misunderstandings. > > Well good, I hope they get burned. > > People who don't have sufficiently advanced technology need to grow up, > and add such things. This isn't 1980 anymore. >
remove chroot(2) from spamd(8)
Hi,
By now we are already confident that pledge(2) "just works(tm)" and that it can
be used to effectively remove filesystem access.
That being said, in spamd(8) when I pledge(2)d it the main priv process got
"stdio inet" which means there's no fs access at all so calling
chroot(2)/chdir(2) here doesn't make sense anymore. Just remove them.
Comments? OK?
Index: spamd.c
===
RCS file: /cvs/src/libexec/spamd/spamd.c,v
retrieving revision 1.155
diff -u -p -u -r1.155 spamd.c
--- spamd.c 22 Oct 2018 17:31:24 - 1.155
+++ spamd.c 31 Jul 2019 07:06:53 -
@@ -1519,15 +1519,6 @@ main(int argc, char *argv[])
}
close(trappipe[1]);
- if (chroot("/var/empty") == -1) {
- syslog(LOG_ERR, "cannot chroot to /var/empty.");
- exit(1);
- }
- if (chdir("/") == -1) {
- syslog(LOG_ERR, "cannot chdir to /");
- exit(1);
- }
-
if (setgroups(1, >pw_gid) ||
setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) ||
setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
Re: update acct(5)
this surely looks better than mine
ok mestre@
On 14:25 Mon 29 Jul , Todd C. Miller wrote:
> On Mon, 29 Jul 2019 18:14:14 +0100, Ricardo Mestre wrote:
>
> > Sorry, I'm not too good with words for manpages.
> >
> > Is this good enough? I'll commit the AUNVEIL bit separately anyway in a few
> > minutes.
> >
> > And while we are here what about ACOMPAT? It was used for PDP-11 compat on
> > va
> > x,
> > but we don't support it anymore, except on lastcomm(1) even though the
> > kernel
> > support is not present.
>
> How about we expand the flag descriptions as follows?
>
> - todd
>
> Index: share/man/man5/acct.5
> ===
> RCS file: /cvs/src/share/man/man5/acct.5,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 acct.5
> --- share/man/man5/acct.5 29 Jul 2019 17:26:00 - 1.17
> +++ share/man/man5/acct.5 29 Jul 2019 20:24:26 -
> @@ -86,6 +86,7 @@ struct acct {
>
> #ifdef _KERNEL
> int acct_process(struct proc *p);
> +int acct_shutdown(void);
> #endif
> .Ed
> .Pp
> @@ -96,16 +97,46 @@ is saved in the field
> .Fa ac_comm
> and its status is saved by setting one or more of the following flags in
> .Fa ac_flag :
> -.Dv AFORK ,
> -.Dv ASU ,
> -.Dv ACOMPAT ,
> -.Dv ACORE ,
> -and
> -.Dv AXSIG .
> +.Pp
> +.Bl -tag -width "AUNVEIL"
> +.It Dv AFORK
> +A new process was created via
> +.Xr fork 2
> +that was not followed by a call to
> +.Xr execve 2 .
> +.It Dv ASU
> +The process used super-user permissions.
> +This flag is never set and is provided for source compatibility only.
> +.It Dv ACOMPAT
> +On VAX, this indicated that the process executed in PDP/11 compatibility
> mode.
> +This flag is never set and is provided for source compatibility only.
> +.It Dv ACORE
> +The process terminated abnormally due to a signal and dumped
> +.Xr core 5 .
> +.It Dv AXSIG
> +The process was killed by a
> +.Xr signal 3 .
> +.It Dv APLEDGE
> +The process was killed due to a
> +.Xr pledge 2
> +violation.
> +.It Dv ATRAP
> +The process was killed due to a memory access violation
> +detected by a processor trap.
> +.It Dv AUNVEIL
> +The process attempted a file access that was prevented by
> +.Xr unveil 2
> +restrictions.
> +Note that this does not cause the process to terminate.
> +.El
> .Sh SEE ALSO
> .Xr lastcomm 1 ,
> .Xr acct 2 ,
> .Xr execve 2 ,
> +.Xr pledge 2 ,
> +.Xr unveil 2 ,
> +.Xr signal 3 ,
> +.Xr core 5 ,
> .Xr accton 8 ,
> .Xr sa 8
> .Sh HISTORY
Re: update acct(5)
Sorry, I'm not too good with words for manpages.
Is this good enough? I'll commit the AUNVEIL bit separately anyway in a few
minutes.
And while we are here what about ACOMPAT? It was used for PDP-11 compat on vax,
but we don't support it anymore, except on lastcomm(1) even though the kernel
support is not present.
Index: acct.5
===
RCS file: /cvs/src/share/man/man5/acct.5,v
retrieving revision 1.16
diff -u -p -u -r1.16 acct.5
--- acct.5 8 Jun 2017 17:14:02 - 1.16
+++ acct.5 29 Jul 2019 17:07:28 -
@@ -74,6 +74,7 @@ struct acct {
#defineAXSIG 0x10/* killed by a signal */
#defineAPLEDGE 0x20/* killed due to pledge violation */
#defineATRAP 0x40/* memory access violation */
+#defineAUNVEIL 0x80/* unveil access violation */
u_int8_t ac_flag; /* accounting flags */
};
@@ -101,10 +102,23 @@ and its status is saved by setting one o
.Dv ACORE ,
and
.Dv AXSIG .
+If terminated by either
+.Xr pledge 2
+or
+.Xr unveil 2
+then the field
+.Fa ac_flag
+additionally will have set
+.Dv APLEDGE
+or
+.Dv AUNVEIL
+respectively.
.Sh SEE ALSO
.Xr lastcomm 1 ,
.Xr acct 2 ,
.Xr execve 2 ,
+.Xr pledge 2 ,
+.Xr unveil 2 ,
.Xr accton 8 ,
.Xr sa 8
.Sh HISTORY
On 08:47 Mon 29 Jul , Todd C. Miller wrote:
> On Mon, 29 Jul 2019 13:55:32 +0100, Ricardo Mestre wrote:
>
> > Now that this is already being used in kernel and userland the manpage for
> > acct(5) needs to be updated.
>
> Do we need to mention APLEDGE and AUNVEIL in the last sentence of
> the DESCRIPTION too?
>
> - todd
update acct(5)
Hi,
Now that this is already being used in kernel and userland the manpage for
acct(5) needs to be updated.
Comments? OK?
Index: acct.5
===
RCS file: /cvs/src/share/man/man5/acct.5,v
retrieving revision 1.16
diff -u -p -u -r1.16 acct.5
--- acct.5 8 Jun 2017 17:14:02 - 1.16
+++ acct.5 29 Jul 2019 12:51:28 -
@@ -74,6 +74,7 @@ struct acct {
#defineAXSIG 0x10/* killed by a signal */
#defineAPLEDGE 0x20/* killed due to pledge violation */
#defineATRAP 0x40/* memory access violation */
+#defineAUNVEIL 0x80/* unveil access violation */
u_int8_t ac_flag; /* accounting flags */
};
remove BUGS section from spamd(8)
Hi, Ever since I introduced pledge(2) on spamd(8) the chroot'ed process, if running in default, cannot get anywhere near the filesystem since its only promises are "stdio inet". Furthermore, in blacklist mode this same codepath is not chroot'ed but once again it gets the same pledge(2). That being said then in my opinion I think that the BUGS section should be removed from the manpage, because even if the processes are running with the same user the concern here doesn't apply anymore. Comments? OK? Index: spamd.8 === RCS file: /cvs/src/libexec/spamd/spamd.8,v retrieving revision 1.134 diff -u -p -u -r1.134 spamd.8 --- spamd.8 2 Apr 2017 18:14:34 - 1.134 +++ spamd.8 24 Jul 2019 14:47:03 - @@ -607,17 +607,3 @@ The .Nm command first appeared in .Ox 3.3 . -.Sh BUGS -.Nm -currently uses the user -.Dq _spamd -outside a chroot jail when running in default mode, and requires -the greylisting database in -.Pa /var/db/spamd -to be owned by the -.Dq _spamd -user. -This is wrong and should change to a distinct user from the -one used by the chrooted -.Nm -process.
Re: ssh needs sendfd in pledge call?
Hi,
As Timothy reported, and with the options he selected for ssh then the codepath
taken will call mux_client_request_session -> mm_send_fd -> sendmsg(2). Since
sendmsg(2) is not allowed in that codepath then pledge(2) kills the process.
Please see below the trace he provided privately, and also his diff beneath it
which adds "sendfd" to pledge(2). In my opinion it's not that bad to add yet
another promise since a little bit later we'll reduce it into "stdio proc tty".
Comments? OK?
[New process 396642]
Core was generated by `ssh'.
Program terminated with signal SIGABRT, Aborted.
#0 _thread_sys_sendmsg () at -:3
#0 _thread_sys_sendmsg () at -:3
No locals.
#1 0x072d95537d7e in _libc_sendmsg_cancel (s=4, msg=0x7f7f26b0,
flags=0) at /usr/src/lib/libc/sys/w_sendmsg.c:27
_tib = 0x72d25280d80
#2 0x072afb52cb22 in mm_send_fd (sock=4, fd=0) at
/usr/src/usr.bin/ssh/ssh/../monitor_fdpass.c:70
cmsg = 0x0014
msg = {
msg_name = 0x0,
msg_namelen = 0,
msg_iov = 0x7f7f26e8,
msg_iovlen = 1,
msg_control = 0x7f7f26f8,
msg_controllen = 24,
msg_flags = 0
}
pfd = {
fd = 4,
events = 4,
revents = 0
}
n =
#3 0x072afb4e4183 in mux_client_request_session (fd=) at
/usr/src/usr.bin/ssh/ssh/../mux.c:1944
devnull =
term =
echar =
m =
r =
i =
type =
rid =
sid =
rawmode =
exitval =
exitval_seen =
esid =
e =
#4 muxclient (path=) at /usr/src/usr.bin/ssh/ssh/../mux.c:2359
sock =
pid =
#5 0x072afb4d14a8 in control_persist_detach () at
/usr/src/usr.bin/ssh/ssh/../ssh.c:1538
pid =
devnull =
#6 fork_postauth () at /usr/src/usr.bin/ssh/ssh/../ssh.c:1563
No locals.
#7 0x072afb4d1630 in ssh_confirm_remote_forward (ssh=0x72d84623780,
type=, seq=, ctxt=0x72d4049d540)
at /usr/src/usr.bin/ssh/ssh/../ssh.c:1632
rfwd = 0x72d4049d540
port =
r =
#8 0x072afb4dbccc in client_global_request_reply (type=,
seq=, ssh=0x72d84623780)
at /usr/src/usr.bin/ssh/ssh/../clientloop.c:463
gc = 0x72d4049de00
#9 0x072afb53012b in ssh_dispatch_run (ssh=0x72d84623780, mode=1,
done=0x72afb549810 ) at /usr/src/usr.bin/ssh/ssh/../dispatch.c:111
seqnr = 4294911664
type =
r =
#10 0x072afb5301ab in ssh_dispatch_run_fatal (ssh=0x72d84623780,
mode=-55632, done=0x0) at /usr/src/usr.bin/ssh/ssh/../dispatch.c:131
r =
#11 0x072afb4d9601 in client_process_buffered_input_packets (ssh=) at /usr/src/usr.bin/ssh/ssh/../clientloop.c:1182
No locals.
#12 client_loop (ssh=0x72d84623780, have_pty=0, escape_char_arg=92236,
ssh2_chan_id=) at /usr/src/usr.bin/ssh/ssh/../clientloop.c:1325
buf =
"cy\t\345ߍb\255\033q\005\242\vP\265/\263\b|\201\330X\354o\245\273\311\\^k*\253ѝTn\022<\025\261\244\a\342V6D[?\030\231.\001\264Xn\201\365\375H\373\021\255\256\247@\331W\247;P7\021\300~2e\362\005\264>\220\027\204\217\242\023\006\315eA\366\f\201\374\a\304*\211\235\071"
max_fd2 = 3
max_fd = 3
nalloc =
start_time = 92236.903928921995
r =
writeset =
readset =
len =
total_time =
ibytes =
obytes =
#13 0x072afb4d0384 in ssh_session2 (ssh=, pw=0x72d2e3d4800)
at /usr/src/usr.bin/ssh/ssh/../ssh.c:1966
id =
tun_fwd_ifname =
cp =
r =
devnull =
#14 main (ac=, av=) at
/usr/src/usr.bin/ssh/ssh/../ssh.c:1499
buf =
"/home/tbrown/.ssh\000\000\000\000\000\000\000}\r\001\000\022\000\v\000\340\231\035\000\000\000\000\000d\000\000\000\000\000\000\000\000\023\001\000\022\000\v\000\360\v\036\000\000\000\000\000\060\000\000\000\000\000\000\000^_\000\000\022\000\v\000@Q\020\000\000\000\000\000\060\000\000\000\000\000\000\000\215\271\000\000\022\000\v\000\060\364\024\000\000\000\000\000\320\000\000\000\000\000\000\000\031\322\000\000\022\000\v\000@Z\033\000\000\000\000\000\021\000\000\000\000\000\000\000\335\v\000\000\022\000\v\000\220\371\031\000\000\000\000\000\027\000\000\000\000\000\000\000\016*\000\000\022\000\v\000\020\302\r\000\000\000\000\000\060\000\000\000\000\000\000\000\362D\000\000\021\000\024\000\000"...
cname = '\000' , "\343l\000\000\020", '\000' , "\353l\000\000\020", '\000' , ".p\000\000\020",
'\000' , "\066p\000\000\020", '\000' ,
"\306q\000\000\020", '\000' , "\313q\000\000\020", '\000'
, "\311|\000\000\020", '\000' , "
\203\000\000\020", '\000' ...
conn_hash =
"@\003\244\227\263\017\204\273P\216[J\000:Z\355z\002\275\264\230-\345Q\000\064\267\225-\a\000\000^\215\177\v\375\025%n\210_\377\377\177\177\000\000ϻv\353-\a\000\000\230_\377\377\177\177\000"
want_final_pass =
opt_terminated =
config_test = 0
pw = 0x72d2e3d4800
Remove duplicate pledge(2) from tsort(1)
Hi,
pledge "stdio rpath" is already called in main(), so we can remove the
duplicate from parse_args(), along with the pledge commented out from another
era!
The second part is about placing pledge "stdio" in main() instead for better
readability (at least for me).
No functional change is intended here and regress still pass, comments ok?
Index: tsort.c
===
RCS file: /cvs/src/usr.bin/tsort/tsort.c,v
retrieving revision 1.36
diff -u -p -u -r1.36 tsort.c
--- tsort.c 20 May 2017 09:31:19 - 1.36
+++ tsort.c 11 Jul 2019 12:13:00 -
@@ -879,10 +879,6 @@ parse_args(int argc, char *argv[], struc
files[i] = NULL;
-/* if (pledge("stdio rpath", files) == -1) */
- if (pledge("stdio rpath", NULL) == -1)
- err(1, "pledge");
-
nodes_init(pairs);
order = 0;
@@ -910,9 +906,6 @@ parse_args(int argc, char *argv[], struc
order = read_pairs(stdin, pairs, reverse_flag, "stdin",
order, hints_flag == 2);
}
-
- if (pledge("stdio", NULL) == -1)
- err(1, "pledge");
}
static int
@@ -1003,6 +996,10 @@ main(int argc, char *argv[])
err(1, "pledge");
parse_args(argc, argv, );
+
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+
return tsort();
}
Re: unveil(2) switchd(8)
Of course when I mention in the second option to "unveil" / it's just to
call pledge with rpath, not actually calling unveil(2).
On 10:20 Thu 11 Jul , Ricardo Mestre wrote:
> Hi,
>
> switchd(8)'s main proc needs to open the following paths, and which can be
> unveiled:
>
> / -> read, it will open config files from anywhere in the system, and also
> needs to open /etc/services
>
> /dev -> read/write, in order to open /dev/tap* and /dev/switch*
>
> Just before the main loop the devices were already opened so we can drop wpath
> from pledge(2). We still need to keep rpath since the daemon may receive a
> SIGHUP and reload the config files again, along with /etc/services.
>
> Another option is to just remove the current pledge(2) placement and add the
> one I have below, this way there's no need to unveil /dev, just / .
>
> Comments? OK?
>
> Index: switchd.c
> ===
> RCS file: /cvs/src/usr.sbin/switchd/switchd.c,v
> retrieving revision 1.16
> diff -u -p -u -r1.16 switchd.c
> --- switchd.c 10 Sep 2018 13:21:39 - 1.16
> +++ switchd.c 11 Jul 2019 09:08:07 -
> @@ -191,6 +191,10 @@ main(int argc, char *argv[])
>
> log_procinit("parent");
>
> + if (unveil("/", "r") == -1)
> + fatal("unveil");
> + if (unveil("/dev", "rw") == -1)
> + fatal("unveil");
> /*
>* pledge in the parent process:
>* stdio - for malloc and basic I/O including events.
> @@ -221,6 +225,9 @@ main(int argc, char *argv[])
>
> if (parent_configure(sc) == -1)
> fatalx("configuration failed");
> +
> + if (pledge("stdio rpath inet dns sendfd", NULL) == -1)
> + fatal("pledge");
>
> event_dispatch();
>
>
unveil(2) switchd(8)
Hi,
switchd(8)'s main proc needs to open the following paths, and which can be
unveiled:
/ -> read, it will open config files from anywhere in the system, and also
needs to open /etc/services
/dev -> read/write, in order to open /dev/tap* and /dev/switch*
Just before the main loop the devices were already opened so we can drop wpath
from pledge(2). We still need to keep rpath since the daemon may receive a
SIGHUP and reload the config files again, along with /etc/services.
Another option is to just remove the current pledge(2) placement and add the
one I have below, this way there's no need to unveil /dev, just / .
Comments? OK?
Index: switchd.c
===
RCS file: /cvs/src/usr.sbin/switchd/switchd.c,v
retrieving revision 1.16
diff -u -p -u -r1.16 switchd.c
--- switchd.c 10 Sep 2018 13:21:39 - 1.16
+++ switchd.c 11 Jul 2019 09:08:07 -
@@ -191,6 +191,10 @@ main(int argc, char *argv[])
log_procinit("parent");
+ if (unveil("/", "r") == -1)
+ fatal("unveil");
+ if (unveil("/dev", "rw") == -1)
+ fatal("unveil");
/*
* pledge in the parent process:
* stdio - for malloc and basic I/O including events.
@@ -221,6 +225,9 @@ main(int argc, char *argv[])
if (parent_configure(sc) == -1)
fatalx("configuration failed");
+
+ if (pledge("stdio rpath inet dns sendfd", NULL) == -1)
+ fatal("pledge");
event_dispatch();
Re: ldpd(8): unveil(2) main proc / reduce pledge(2) on ldpe
The third's the charm? :)
OK?
On 20:23 Fri 14 Jun , Ricardo Mestre wrote:
> ping?
>
> On 12:33 Wed 22 May , Ricardo Mestre wrote:
> > Hi,
> >
> > Like we did on other daemons that cannot be pledged due to forbidden ioctls
> > the
> > main process can be unveiled to restrict filesystem access. In this case we
> > can
> > restrict it to only read, although it must be the entire / since the daemon
> > is
> > able to include config files from anywhere.
> >
> > Additionally the ldpe process currently has cpath promise to unlink the
> > socket,
> > nevertheless the socket is actually unlinked from the main proc so this
> > permission can be removed. As we discussed before leaving the socket behind
> > doesn't do any harm that's why I didn't unveil it in the main proc.
> >
> > Comments? OK?
> >
> > Index: ldpd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldpd/ldpd.c,v
> > retrieving revision 1.64
> > diff -u -p -u -r1.64 ldpd.c
> > --- ldpd.c 31 Mar 2019 03:36:18 - 1.64
> > +++ ldpd.c 22 May 2019 11:09:33 -
> > @@ -222,6 +222,11 @@ main(int argc, char *argv[])
> > pipe_parent2ldpe[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE,
> > sockname);
> >
> > + if (unveil("/", "r") == -1)
> > + fatal("unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + fatal("unveil");
> > +
> > event_init();
> >
> > /* setup signal handler */
> > Index: ldpe.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> > retrieving revision 1.75
> > diff -u -p -u -r1.75 ldpe.c
> > --- ldpe.c 23 Jan 2019 02:02:04 - 1.75
> > +++ ldpe.c 22 May 2019 11:09:33 -
> > @@ -107,7 +107,7 @@ ldpe(int debug, int verbose, char *sockn
> > setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
> > fatal("can't drop privileges");
> >
> > - if (pledge("stdio cpath inet mcast recvfd", NULL) == -1)
> > + if (pledge("stdio inet mcast recvfd", NULL) == -1)
> > fatal("pledge");
> >
> > event_init();
Re: unveil dhclient (privileged process)
Since krw@ gave me feedback on yet another way to handle the defines I'd rather
leave that alone for now, it can be left as an exercise later on as you point
out.
I'll go ahead and commit this instead if no one objects.
Index: dhclient.c
===
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.641
diff -u -p -u -r1.641 dhclient.c
--- dhclient.c 1 Jul 2019 16:53:59 - 1.641
+++ dhclient.c 11 Jul 2019 06:23:49 -
@@ -2232,6 +2232,13 @@ fork_privchld(struct interface_info *ifi
if ((routefd = socket(AF_ROUTE, SOCK_RAW, 0)) == -1)
fatal("socket(AF_ROUTE, SOCK_RAW)");
+ if (unveil("/etc/resolv.conf", "wc") == -1)
+ fatal("unveil");
+ if (unveil("/etc/resolv.conf.tail", "r") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
while (quit == 0) {
pfd[0].fd = priv_ibuf->fd;
pfd[0].events = POLLIN;
Re: unveil dhclient (privileged process)
n("%s: open(%s)", log_procname, _PATH_RESOLV_CONF);
return;
}
sz = strlen(contents);
n = write(fd, contents, sz);
if (n == -1)
- log_warn("%s: write(%s)", log_procname, path);
+ log_warn("%s: write(%s)", log_procname, _PATH_RESOLV_CONF);
else if ((size_t)n < sz)
log_warnx("%s: write(%s): %zd of %zu bytes", log_procname,
- path, n, sz);
+ _PATH_RESOLV_CONF, n, sz);
close(fd);
}
On 14:04 Tue 06 Nov , Ricardo Mestre wrote:
> I guess dhclient is a special one, the others need to re-exec themselves but
> at
> fork+exec time, not again. does this suits you better? don't want to keep
> bothering you with silly diffs.
>
> On 08:38 Tue 06 Nov , kwesterb...@gmail.com wrote:
> > My objection is to having "/sbin/dhclient" coded anywhere. This makes it
> > difficult to test different versions. Do these other daemons restart
> > themselves as part of their normal operation?
> >
> > Why not take argv[0] and run it through realpath() on startup to get rid of
> > symlinks, etc.?
> >
> > Ken
Re: unveil(2) sysctl(8)
sure, it wasn't mine! it was just missing one unveil so it's ok mestre@
on this one
On 11:42 Sat 15 Jun , Theo de Raadt wrote:
> yeah that was my idea..
>
> Ricardo Mestre wrote:
>
> > Hi,
> >
> > Sorry to be late in the game, but as jca@ pointed out sysctl(8) tries to
> > open _PATH_DEVDB first and then /dev if it cannot open the former, so
> > both should be unveil(2)ed. Scramble the includes while at it.
> >
> > Index: sysctl.c
> > ===
> > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > retrieving revision 1.242
> > diff -u -p -u -r1.242 sysctl.c
> > --- sysctl.c13 May 2019 20:47:19 - 1.242
> > +++ sysctl.c14 Jun 2019 19:04:01 -
> > @@ -94,13 +94,14 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > -#include
> > -#include
> > #include
> >
> > #include
> > @@ -162,6 +163,8 @@ struct list secondlevel[] = {
> >
> > intAflag, aflag, nflag, qflag;
> >
> > +time_t boottime;
> > +
> > /*
> > * Variables requiring special processing.
> > */
> > @@ -255,6 +258,15 @@ main(int argc, char *argv[])
> > argc -= optind;
> > argv += optind;
> >
> > + ctime(); /* satisfy potential $TZ expansion before unveil() */
> > +
> > + if (unveil(_PATH_DEVDB, "r") == -1)
> > + err(1,"unveil");
> > + if (unveil("/dev", "r") == -1)
> > + err(1, "unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + err(1, "unveil");
> > +
> > if (argc == 0 || (Aflag || aflag)) {
> > debuginit();
> > vfsinit();
> > @@ -893,7 +905,6 @@ parse(char *string, int flags)
> > }
> > if (special & BOOTTIME) {
> > struct timeval *btp = (struct timeval *)buf;
> > - time_t boottime;
> >
> > if (!nflag) {
> > boottime = btp->tv_sec;
> >
> > On 10:35 Sat 08 Jun , Theo de Raadt wrote:
> > > When userland was massaged for pledge(), I hesitated using the
> > > "manually call tzset()" approach for handling things. It felt
> > > too low-level to call tzset(), an API almost noone knows the
> > > existance of.
> > >
> > > Arriving in the same situation to satisfy unveil(). Again calling
> > > tzset() feels too unfamiliar and low level.
> > >
> > > Regarding the comment in your diff, it says "localtime", but what is
> > > actually called is ctime(), which calls localtime() (which calls
> > > tzset(), which is where the unveil-files-missing or pledge-whatver
> > > issues would show up in some programs). Probably should adjust
> > > the comment
> > >
> > > Here's the later troublesome chunk:
> > >
> > > if (special & BOOTTIME) {
> > > struct timeval *btp = (struct timeval *)buf;
> > > time_t boottime;
> > >
> > > if (!nflag) {
> > > boottime = btp->tv_sec;
> > > (void)printf("%s%s%s", string, equ,
> > > ctime());
> > >
> > > That makes me wonder, can we be less obtuse up front, and
> > > prime the subsystem before unveil by calling the same function which
> > > will be called later?
> > >
> > > Something like this. It feels slightly better to me.
> > >
> > > Index: sysctl.c
> > > ===
> > > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > > retrieving revision 1.242
> > > diff -u -p -u -r1.242 sysctl.c
> > > --- sysctl.c 13 May 2019 20:47:19 - 1.242
> > > +++ sysctl.c 8 Jun 2019 16:33:07 -
> > > @@ -162,6 +162,8 @@ struct list secondlevel[] = {
> > >
> > > int Aflag, aflag, nflag, qflag;
> > >
> > > +time_t boottime;
> > > +
> > > /*
> > > * Variables requiring special processing.
> > > */
> > > @@ -255,6 +257,12 @@ main(int argc, char *argv[])
> > > argc -= optind;
> > > argv += optind;
> > >
> > > + ctime(); /* satisfy pot
Re: ldpd(8): unveil(2) main proc / reduce pledge(2) on ldpe
ping?
On 12:33 Wed 22 May , Ricardo Mestre wrote:
> Hi,
>
> Like we did on other daemons that cannot be pledged due to forbidden ioctls
> the
> main process can be unveiled to restrict filesystem access. In this case we
> can
> restrict it to only read, although it must be the entire / since the daemon is
> able to include config files from anywhere.
>
> Additionally the ldpe process currently has cpath promise to unlink the
> socket,
> nevertheless the socket is actually unlinked from the main proc so this
> permission can be removed. As we discussed before leaving the socket behind
> doesn't do any harm that's why I didn't unveil it in the main proc.
>
> Comments? OK?
>
> Index: ldpd.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/ldpd.c,v
> retrieving revision 1.64
> diff -u -p -u -r1.64 ldpd.c
> --- ldpd.c31 Mar 2019 03:36:18 - 1.64
> +++ ldpd.c22 May 2019 11:09:33 -
> @@ -222,6 +222,11 @@ main(int argc, char *argv[])
> pipe_parent2ldpe[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE,
> sockname);
>
> + if (unveil("/", "r") == -1)
> + fatal("unveil");
> + if (unveil(NULL, NULL) == -1)
> + fatal("unveil");
> +
> event_init();
>
> /* setup signal handler */
> Index: ldpe.c
> ===
> RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
> retrieving revision 1.75
> diff -u -p -u -r1.75 ldpe.c
> --- ldpe.c23 Jan 2019 02:02:04 - 1.75
> +++ ldpe.c22 May 2019 11:09:33 -
> @@ -107,7 +107,7 @@ ldpe(int debug, int verbose, char *sockn
> setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
> fatal("can't drop privileges");
>
> - if (pledge("stdio cpath inet mcast recvfd", NULL) == -1)
> + if (pledge("stdio inet mcast recvfd", NULL) == -1)
> fatal("pledge");
>
> event_init();
Re: unveil(2) sysctl(8)
Hi,
Sorry to be late in the game, but as jca@ pointed out sysctl(8) tries to
open _PATH_DEVDB first and then /dev if it cannot open the former, so
both should be unveil(2)ed. Scramble the includes while at it.
Index: sysctl.c
===
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.242
diff -u -p -u -r1.242 sysctl.c
--- sysctl.c13 May 2019 20:47:19 - 1.242
+++ sysctl.c14 Jun 2019 19:04:01 -
@@ -94,13 +94,14 @@
#include
#include
+#include
#include
#include
+#include
+#include
#include
#include
#include
-#include
-#include
#include
#include
@@ -162,6 +163,8 @@ struct list secondlevel[] = {
intAflag, aflag, nflag, qflag;
+time_t boottime;
+
/*
* Variables requiring special processing.
*/
@@ -255,6 +258,15 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ ctime(); /* satisfy potential $TZ expansion before unveil() */
+
+ if (unveil(_PATH_DEVDB, "r") == -1)
+ err(1,"unveil");
+ if (unveil("/dev", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (argc == 0 || (Aflag || aflag)) {
debuginit();
vfsinit();
@@ -893,7 +905,6 @@ parse(char *string, int flags)
}
if (special & BOOTTIME) {
struct timeval *btp = (struct timeval *)buf;
- time_t boottime;
if (!nflag) {
boottime = btp->tv_sec;
On 10:35 Sat 08 Jun , Theo de Raadt wrote:
> When userland was massaged for pledge(), I hesitated using the
> "manually call tzset()" approach for handling things. It felt
> too low-level to call tzset(), an API almost noone knows the
> existance of.
>
> Arriving in the same situation to satisfy unveil(). Again calling
> tzset() feels too unfamiliar and low level.
>
> Regarding the comment in your diff, it says "localtime", but what is
> actually called is ctime(), which calls localtime() (which calls
> tzset(), which is where the unveil-files-missing or pledge-whatver
> issues would show up in some programs). Probably should adjust
> the comment
>
> Here's the later troublesome chunk:
>
> if (special & BOOTTIME) {
> struct timeval *btp = (struct timeval *)buf;
> time_t boottime;
>
> if (!nflag) {
> boottime = btp->tv_sec;
> (void)printf("%s%s%s", string, equ, ctime());
>
> That makes me wonder, can we be less obtuse up front, and
> prime the subsystem before unveil by calling the same function which
> will be called later?
>
> Something like this. It feels slightly better to me.
>
> Index: sysctl.c
> ===
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.242
> diff -u -p -u -r1.242 sysctl.c
> --- sysctl.c 13 May 2019 20:47:19 - 1.242
> +++ sysctl.c 8 Jun 2019 16:33:07 -
> @@ -162,6 +162,8 @@ struct list secondlevel[] = {
>
> int Aflag, aflag, nflag, qflag;
>
> +time_t boottime;
> +
> /*
> * Variables requiring special processing.
> */
> @@ -255,6 +257,12 @@ main(int argc, char *argv[])
> argc -= optind;
> argv += optind;
>
> + ctime(); /* satisfy potential $TZ expansion before unveil() */
> +
> + if (unveil("/dev", "r") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
> if (argc == 0 || (Aflag || aflag)) {
> debuginit();
> vfsinit();
> @@ -893,7 +901,6 @@ parse(char *string, int flags)
> }
> if (special & BOOTTIME) {
> struct timeval *btp = (struct timeval *)buf;
> - time_t boottime;
>
> if (!nflag) {
> boottime = btp->tv_sec;
>
>
>
>
>
> Florian Obser wrote:
>
> > On Fri, Jun 07, 2019 at 11:24:30PM +0100, Ricardo Mestre wrote:
> > > i did that and for some for reason i didn't get it! it tries to open
> > > timezone so it kinda looks like a red flag right there...
> > >
> > > apart from /dev do we need to look into TZ on this one as well? if TZ
> > > var needs to be looked at then all bets are off :/
> >
> > this seems to do the right thing:
> >
> > diff --git sysctl.c sysctl.c
> > index dc6abc16670..c74e706942a 100644
> > --- sysctl.c
> > +++ sysctl
Re: unveil(2) sysctl(8)
i did that and for some for reason i didn't get it! it tries to open
timezone so it kinda looks like a red flag right there...
apart from /dev do we need to look into TZ on this one as well? if TZ
var needs to be looked at then all bets are off :/
On 01:01 Sat 08 Jun , Consus wrote:
> On 18:14 Fri 07 Jun, Ricardo Mestre wrote:
> > Hi,
> >
> > My eyes may be cheating me in plain sight, but sysctl(8) doesn't seem to
> > require fs access at all.
> >
> > Comments? OK?
> >
> > Index: sysctl.c
> > ===
> > RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> > retrieving revision 1.242
> > diff -u -p -u -r1.242 sysctl.c
> > --- sysctl.c13 May 2019 20:47:19 - 1.242
> > +++ sysctl.c7 Jun 2019 17:01:23 -
> > @@ -255,6 +255,11 @@ main(int argc, char *argv[])
> > argc -= optind;
> > argv += optind;
> >
> > + if (unveil("/", "") == -1)
> > + err(1, "unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + err(1, "unveil");
> > +
> > if (argc == 0 || (Aflag || aflag)) {
> > debuginit();
> > vfsinit();
> >
>
> $ ktrace sysctl >/dev/null; kdump 2>&1 | grep -c 'CALL open'
> 3
>
unveil(2) sysctl(8)
Hi,
My eyes may be cheating me in plain sight, but sysctl(8) doesn't seem to
require fs access at all.
Comments? OK?
Index: sysctl.c
===
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.242
diff -u -p -u -r1.242 sysctl.c
--- sysctl.c13 May 2019 20:47:19 - 1.242
+++ sysctl.c7 Jun 2019 17:01:23 -
@@ -255,6 +255,11 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
+ if (unveil("/", "") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (argc == 0 || (Aflag || aflag)) {
debuginit();
vfsinit();
Re: pledge(2) unbound-checkconf(8)
it must be one of those days...
it's ok mestre if you feel like commiting it and doesn't add any burden
for you when upgrading unbound.
On 13:18 Thu 23 May , Stuart Henderson wrote:
> check_mod(cfg, val_get_funcblock());
>
> - needs to read the DNSSEC root key,
>
> check_hints(cfg);
>
> - needs to read hints files,
>
> check_auth(cfg);
>
> - needs to read zones
>
> I think you could do this, though:
>
> Index: smallapp/unbound-checkconf.c
> ===
> RCS file: /cvs/src/usr.sbin/unbound/smallapp/unbound-checkconf.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 unbound-checkconf.c
> --- smallapp/unbound-checkconf.c 8 Feb 2019 10:29:08 - 1.11
> +++ smallapp/unbound-checkconf.c 23 May 2019 12:17:03 -
> @@ -587,6 +587,10 @@ morechecks(struct config_file* cfg)
> endpwent();
> # endif
> }
> +
> + if (pledge("stdio rpath", NULL) == -1)
> + fatal_exit("Could not pledge");
> +
> #endif
> if(cfg->remote_control_enable && options_remote_is_address(cfg)
> && cfg->control_use_cert) {
> @@ -724,6 +728,10 @@ int main(int argc, char* argv[])
> if(argc == 1)
> f = argv[0];
> elsef = cfgfile;
> +
> + if (pledge("stdio rpath getpw", NULL) == -1)
> + fatal_exit("Could not pledge");
> +
> checkconf(f, opt, final);
> checklock_stop();
> return 0;
>
Re: pledge(2) unbound-checkconf(8)
bonkers my brain must have farted :\ rpath should be dropped after loading the
certs. I just tested it with remote-control with certificates, could you
please let me know if it works for you now?
Index: unbound-checkconf.c
===
RCS file: /cvs/src/usr.sbin/unbound/smallapp/unbound-checkconf.c,v
retrieving revision 1.11
diff -u -p -u -r1.11 unbound-checkconf.c
--- unbound-checkconf.c 8 Feb 2019 10:29:08 - 1.11
+++ unbound-checkconf.c 23 May 2019 10:45:48 -
@@ -602,6 +602,9 @@ morechecks(struct config_file* cfg)
cfg->control_cert_file);
}
+ if (pledge("stdio", NULL) == -1)
+ fatal_exit("Could not pledge");
+
localzonechecks(cfg);
view_and_respipchecks(cfg);
#ifdef CLIENT_SUBNET
@@ -724,6 +727,10 @@ int main(int argc, char* argv[])
if(argc == 1)
f = argv[0];
elsef = cfgfile;
+
+ if (pledge("stdio rpath getpw", NULL) == -1)
+ fatal_exit("Could not pledge");
+
checkconf(f, opt, final);
checklock_stop();
return 0;
On 10:29 Thu 23 May , Stuart Henderson wrote:
> Not ok - if you're using remote-control with certificates (for example,
> to control remote unbound instances over a network connection) it hits the
> following:
>
> unbound-checkcon[21086]: pledge "rpath", syscall 38
>
> (gdb) bt
> #0 stat () at -:3
> #1 0x04da8ddd61dc in is_file (fname=0x4dd11e9e3c0
> "/var/unbound/etc/unbound_server.key")
> at /usr/src/usr.sbin/unbound/smallapp/unbound-checkconf.c:278
> #2 0x04da8ddd5f10 in check_chroot_string (desc=0x4da8dda7c5d
> "server-key-file", ss=0x4dca3ee33d0,
> chrootdir=0x0, cfg=0x4dca3ee3000) at
> /usr/src/usr.sbin/unbound/smallapp/unbound-checkconf.c:335
> #3 0x04da8ddd5114 in morechecks (cfg=0x4dca3ee3000)
> at /usr/src/usr.sbin/unbound/smallapp/unbound-checkconf.c:597
> #4 0x04da8ddd4776 in checkconf (cfgfile=0x4da8dda9506
> "/var/unbound/etc/unbound.conf", opt=0x0, final=0)
> at /usr/src/usr.sbin/unbound/smallapp/unbound-checkconf.c:674
> #5 0x04da8ddd44e2 in main (argc=0, argv=0x7f7d1850)
> at /usr/src/usr.sbin/unbound/smallapp/unbound-checkconf.c:735
>
pledge(2) unbound-checkconf(8)
Hi,
unbound-checkconf(8) needs to chdir(2) and then open(2) the config file and
to call getpwnam(3). This means it needs to pledge for rpath and getpw, but
after calling getpwnam(3) the config file was already loaded so we can drop
both promises afterwards.
Comments? OK?
Index: unbound-checkconf.c
===
RCS file: /cvs/src/usr.sbin/unbound/smallapp/unbound-checkconf.c,v
retrieving revision 1.11
diff -u -p -u -r1.11 unbound-checkconf.c
--- unbound-checkconf.c 8 Feb 2019 10:29:08 - 1.11
+++ unbound-checkconf.c 22 May 2019 12:49:12 -
@@ -588,6 +588,10 @@ morechecks(struct config_file* cfg)
# endif
}
#endif
+
+ if (pledge("stdio", NULL) == -1)
+ fatal_exit("Could not pledge");
+
if(cfg->remote_control_enable && options_remote_is_address(cfg)
&& cfg->control_use_cert) {
check_chroot_string("server-key-file", >server_key_file,
@@ -724,6 +728,10 @@ int main(int argc, char* argv[])
if(argc == 1)
f = argv[0];
elsef = cfgfile;
+
+ if (pledge("stdio rpath getpw", NULL) == -1)
+ fatal_exit("Could not pledge");
+
checkconf(f, opt, final);
checklock_stop();
return 0;
ldpd(8): unveil(2) main proc / reduce pledge(2) on ldpe
Hi,
Like we did on other daemons that cannot be pledged due to forbidden ioctls the
main process can be unveiled to restrict filesystem access. In this case we can
restrict it to only read, although it must be the entire / since the daemon is
able to include config files from anywhere.
Additionally the ldpe process currently has cpath promise to unlink the socket,
nevertheless the socket is actually unlinked from the main proc so this
permission can be removed. As we discussed before leaving the socket behind
doesn't do any harm that's why I didn't unveil it in the main proc.
Comments? OK?
Index: ldpd.c
===
RCS file: /cvs/src/usr.sbin/ldpd/ldpd.c,v
retrieving revision 1.64
diff -u -p -u -r1.64 ldpd.c
--- ldpd.c 31 Mar 2019 03:36:18 - 1.64
+++ ldpd.c 22 May 2019 11:09:33 -
@@ -222,6 +222,11 @@ main(int argc, char *argv[])
pipe_parent2ldpe[1], debug, global.cmd_opts & LDPD_OPT_VERBOSE,
sockname);
+ if (unveil("/", "r") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_init();
/* setup signal handler */
Index: ldpe.c
===
RCS file: /cvs/src/usr.sbin/ldpd/ldpe.c,v
retrieving revision 1.75
diff -u -p -u -r1.75 ldpe.c
--- ldpe.c 23 Jan 2019 02:02:04 - 1.75
+++ ldpe.c 22 May 2019 11:09:33 -
@@ -107,7 +107,7 @@ ldpe(int debug, int verbose, char *sockn
setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
fatal("can't drop privileges");
- if (pledge("stdio cpath inet mcast recvfd", NULL) == -1)
+ if (pledge("stdio inet mcast recvfd", NULL) == -1)
fatal("pledge");
event_init();
Re: drop rpath from openssl(1) ciphers
At least it went through ktrace -di, with all flags tested, without opening
any files.
Let's see what the crypto guys say :)
On 12:12 Tue 30 Apr , Theo de Raadt wrote:
> I am a bit sceptical, and worry that some internal function may
> do something.
>
> > openssl ciphers doesn't seem to need to open any files to show the full
> > list of
> > ciphers, or manually selected so drop rpath from pledge.
> >
> > OK?
> >
> > Index: ciphers.c
> > ===
> > RCS file: /cvs/src/usr.bin/openssl/ciphers.c,v
> > retrieving revision 1.9
> > diff -u -p -u -r1.9 ciphers.c
> > --- ciphers.c 7 Feb 2018 05:47:55 - 1.9
> > +++ ciphers.c 30 Apr 2019 18:04:07 -
> > @@ -82,7 +82,7 @@ ciphers_main(int argc, char **argv)
> > char *desc;
> >
> > if (single_execution) {
> > - if (pledge("stdio rpath", NULL) == -1) {
> > + if (pledge("stdio", NULL) == -1) {
> > perror("pledge");
> > exit(1);
> > }
> >
>
drop rpath from openssl(1) ciphers
Hi,
openssl ciphers doesn't seem to need to open any files to show the full list of
ciphers, or manually selected so drop rpath from pledge.
OK?
Index: ciphers.c
===
RCS file: /cvs/src/usr.bin/openssl/ciphers.c,v
retrieving revision 1.9
diff -u -p -u -r1.9 ciphers.c
--- ciphers.c 7 Feb 2018 05:47:55 - 1.9
+++ ciphers.c 30 Apr 2019 18:04:07 -
@@ -82,7 +82,7 @@ ciphers_main(int argc, char **argv)
char *desc;
if (single_execution) {
- if (pledge("stdio rpath", NULL) == -1) {
+ if (pledge("stdio", NULL) == -1) {
perror("pledge");
exit(1);
}
Re: unveil chpass
Here's another one also pending to apply unveil(2) on chpass(1)
OK?
On 12:58 Wed 07 Nov , Ricardo Mestre wrote:
> Hi,
>
> chpass(1) without parameters enters in edit mode by default, in here it will
> need to execute _PATH_BSHELL to spawn a new EDITOR, _PATH_SHELLS to check
> (read) if we are changing from/to a non-standard shell (in case we are not
> root) and read access to `tempname' to verify if the file has valid entries
> and
> create to unlink it.
>
> If -s is used to change a user's shell then it will need read access to
> _PATH_SHELLS by the same reason already mentioned above.
>
> Unconditionally we need to unveil _PATH_MASTERPASSWD_LOCK with write/create
> permissions, _PATH_MASTERPASSWD with read and _PATH_PWD_MKDB to execute
> pwd_mkdb(8).
>
> In the -a case I'm not unveiling /etc/spwd.db since we can get it through
> pledge "getpw", which can be added for completeness of all code paths.
> Note also that the first pledges need "unveil" since we will call unveil(2)
> afterwards.
>
> Tested all paths successfully with a test account. Comments? OK?
>
> Index: chpass.c
> ===
> RCS file: /cvs/src/usr.bin/chpass/chpass.c,v
> retrieving revision 1.44
> diff -u -p -u -r1.44 chpass.c
> --- chpass.c 8 Dec 2017 17:04:15 - 1.44
> +++ chpass.c 7 Nov 2018 12:50:07 -
> @@ -136,7 +136,13 @@ main(int argc, char *argv[])
> pw_error(tempname, 1, 1);
> display(tempname, dfd, pw);
>
> - if (pledge("stdio rpath wpath cpath id proc exec",
> + if (unveil(_PATH_BSHELL, "x") == -1)
> + err(1, "unveil");
> + if (unveil(_PATH_SHELLS, "r") == -1)
> + err(1, "unveil");
> + if (unveil(tempname, "rc") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath wpath cpath id proc exec unveil",
> NULL) == -1)
> err(1, "pledge");
>
> @@ -158,7 +164,9 @@ main(int argc, char *argv[])
> }
>
> if (op == NEWSH) {
> - if (pledge("stdio rpath wpath cpath id proc exec",
> + if (unveil(_PATH_SHELLS, "r") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath wpath cpath id proc exec unveil",
> NULL) == -1)
> err(1, "pledge");
>
> @@ -175,6 +183,12 @@ main(int argc, char *argv[])
> sigdelset(, SIGINT);
> sigprocmask(SIG_BLOCK, , NULL);
>
> + if (unveil(_PATH_MASTERPASSWD_LOCK, "wc") == -1)
> + err(1, "unveil");
> + if (unveil(_PATH_MASTERPASSWD, "r") == -1)
> + err(1, "unveil");
> + if (unveil(_PATH_PWD_MKDB, "x") == -1)
> + err(1, "unveil");
> if (pledge("stdio rpath wpath cpath proc exec", NULL) == -1)
> err(1, "pledge");
>
Re: unveil tcpdrop
Went through my old sent emails and saw this one still pending on my tree.
Is this OK?
On 13:02 Wed 07 Nov , Ricardo Mestre wrote:
> Hi,
>
> tcpdrop(8) needs to access only two files, in this case /etc/hosts and
> /etc/resolv.conf both with read permissions for the purpose of name
> resolution.
> ethers(5) is not needed since we are not using any of the ether_*(3) family.
>
> Since unistd.h needs to be included I also shuffled netdb.h into the right
> place.
>
> Comments? OK?
>
> Index: tcpdrop.c
> ===
> RCS file: /cvs/src/usr.sbin/tcpdrop/tcpdrop.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 tcpdrop.c
> --- tcpdrop.c 16 Jan 2015 06:40:21 - 1.17
> +++ tcpdrop.c 6 Nov 2018 10:48:10 -
> @@ -27,10 +27,11 @@
> #include
>
> #include
> +#include
> #include
> #include
> #include
> -#include
> +#include
>
> __dead void usage(void);
>
> @@ -61,6 +62,13 @@ main(int argc, char **argv)
> char *laddr1, *addr1, *port1, *faddr2, *addr2, *port2;
> struct tcp_ident_mapping tir;
> int gaierr, rval = 0;
> +
> + if (unveil("/etc/hosts", "r") == -1)
> + err(1, "unveil");
> + if (unveil("/etc/resolv.conf", "r") == -1)
> + err(1, "unveil");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
>
> memset(, 0, sizeof(hints));
> hints.ai_family = AF_UNSPEC;
unveil relayd
Hi,
I had a patch with pledge(2) for quite a while ago, but my setup is too simple
and cannot test it enough so at least we can have restricted read access to the
fs in relayd(8)'s main process through unveil(2).
Comments? OK?
Index: relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.174
diff -u -p -u -r1.174 relayd.c
--- relayd.c9 Sep 2018 21:06:51 - 1.174
+++ relayd.c22 Apr 2019 23:36:43 -
@@ -222,6 +222,11 @@ main(int argc, char *argv[])
if (ps->ps_noaction == 0)
log_info("startup");
+ if (unveil("/", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
event_init();
signal_set(>ps_evsigint, SIGINT, parent_sig_handler, ps);
unveil hotplugd
Hi,
Now that I'm finally on my new home and tree is opened again...
hotplugd(8) needs to open(2) `device' with read permissions, /dev/hotplug by
default but can be changed via arguments. Then it needs read/execute on both
_PATH_ETC_HOTPLUG_{ATTACH,DETACH} to access(2) and execl(3) them.
Tested successfully attaching/dettaching (mount/umount) an USB pen. Comments?
OK?
Index: hotplugd.c
===
RCS file: /cvs/src/usr.sbin/hotplugd/hotplugd.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 hotplugd.c
--- hotplugd.c 31 Jul 2016 20:13:12 - 1.14
+++ hotplugd.c 22 Apr 2019 23:02:50 -
@@ -61,9 +61,6 @@ main(int argc, char *argv[])
struct sigaction sact;
struct hotplug_event he;
- if (pledge("stdio rpath proc exec", NULL) == -1)
- err(1, "pledge");
-
while ((ch = getopt(argc, argv, "d:")) != -1)
switch (ch) {
case 'd':
@@ -79,6 +76,15 @@ main(int argc, char *argv[])
argv += optind;
if (argc > 0)
usage();
+
+ if (unveil(device, "r") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_ETC_HOTPLUG_ATTACH, "rx") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_ETC_HOTPLUG_DETACH, "rx") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath proc exec", NULL) == -1)
+ err(1, "pledge");
if ((devfd = open(device, O_RDONLY | O_CLOEXEC)) == -1)
err(1, "%s", device);
Re: YP/NIS support in /etc/ethers, libc ether_ntohost/ether_hostton
Paraphrasing an excerpt of my commit on getent(1) to add unveil(2): "After a discussion with millert@ regarding YP then deraadt@ chimed in referring that when he wrote this code even though we can have YP mappings with several of these dbs "it doesn't mean that things use it, or should, or will" so adding unveil(2) here should not impact any YP environments." I think we can let it go. On 22:01 Fri 09 Nov , Jonathan Matthew wrote: > On Thu, Nov 08, 2018 at 08:05:13PM -0500, Bryan Steele wrote: > > These libc functions are used to map hardware MAC addresses to hostnames > > and vice versa. If it exists, /etc/ethers will typically contain a > > number of lines like so: > > > > 34:00:8a:56:10:20 superman > > > > In addition to that, there is support for using a YP (nee Yellow Pee) > > lookup service: > > > > "If a '+' appears alone on a line in the file, then ether_hostton() will > > consult the x ethers.byname YP map, and ether_ntohost() will consult the > > ethers.byaddr YP map." > > > > This support currently interferes with my work to reduce the pledge(2) > > in tcpdump(8), as the "inet" promise is required to perform these > > lookups.. > > > > I've come up with small a diff to remove it, but it was suggested there > > may be some interactions with ldap, and I'm not sure how important this > > functionality may be to existing YP users (I am not one). > > ypldap does not provide ethers.byname or ethers.byaddr maps, if that's the > ldap interaction in question here. >
Re: unveil error - src/usr.bin/passwd/local_passwd.c (-current)
Hi Mark, Thanks for the report, don't know how I missed it but it's now fixed. On 16:20 Thu 08 Nov , Mark Patruck wrote: > Hi Ricardo, > > when running > > $ passwd > > with src/usr.bin/passwd/local_passwd.c v1.54 > > i see the following error in /var/log/messages > > passwd: cannot stat /etc/login.conf: No such file or directory > > With the following diff, the error disappears > > Index: local_passwd.c > === > RCS file: /cvs/src/usr.bin/passwd/local_passwd.c,v > retrieving revision 1.54 > diff -u -p -r1.54 local_passwd.c > --- local_passwd.c 25 Oct 2018 06:41:38 - 1.54 > +++ local_passwd.c 8 Nov 2018 15:17:40 - > @@ -76,6 +76,8 @@ local_passwd(char *uname, int authentica > err(1, "unveil"); > if (unveil(_PATH_MASTERPASSWD, "r") == -1) > err(1, "unveil"); > + if (unveil(_PATH_LOGIN_CONF, "r") == -1) > + err(1, "unveil"); > if (unveil(_PATH_BSHELL, "x") == -1) > err(1, "unveil"); > if (unveil(_PATH_PWD_MKDB, "x") == -1) > > > > -- > Mark Patruck ( mark at wrapped.cx ) > GPG key 0xF2865E51 / 187F F6D3 EE04 1DCE 1C74 F644 0D3C F66F F286 5E51 > > http://www.wrapped.cx
Re: tcpdump: revisiting some old diffs, cleanup unused functions
Still works as advertised, ok mestre@
On 19:32 Wed 07 Nov , Bryan Steele wrote:
> On Wed, Nov 07, 2018 at 07:06:09PM -0500, Bryan Steele wrote:
> > I'm revisiting some old tcpdump diffs, now that mestre@ has added proper
> > unveil(2) support! :-)
> >
> > Refresher: https://marc.info/?l=openbsd-tech=150535073209723=2
> >
> > This hoists opening pf.os(5) fingerprints '-o' from the 'RUN' state to
> > the 'FILTER' state, this will allow for a reduced pledge(2) at runtime
> > in the (currently root) monitor process.
>
> This was a bit of copy & paste, sorry. This moves the opening of pf.os
> earlier and avoids the unveil later on. Of course, reducing the runtime
> pledge(2) promises will come later! :-)
>
> >
> > This still works as well as it already has. :-)
> >
> > ( ... ) [tcp sum ok] (src OS: OpenBSD 6.1) 3311509932:3311509932(0) win
> > 16384
> > (DF) (ttl 64, id 41239, len 64)
> >
> > The only potential difference is that if /etc/pf.os is replaced at
> > runtime, tcpdump won't reopen it.
> >
> > I don't think that's a problem..
> >
> > ok?
> >
> > -Bryan.
>
> Remove the now unused internal privsep "getline" code, which passed
> lines over a socket, replaced with explicit fdpassing of /etc/pf.os.
>
> This depends on the previous diff..
>
> ok?
>
> -Bryan.
>
> Index: privsep.c
> ===
> RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
> retrieving revision 1.49
> diff -u -p -u -r1.49 privsep.c
> --- privsep.c 28 Sep 2018 06:48:59 - 1.49
> +++ privsep.c 8 Nov 2018 00:19:47 -
> @@ -77,8 +77,8 @@ static const int allowed_max[] = {
> ALLOW(PRIV_GETPROTOENTRIES) |
> ALLOW(PRIV_ETHER_NTOHOST) | ALLOW(PRIV_INIT_DONE),
> /* RUN */ ALLOW(PRIV_GETHOSTBYADDR) | ALLOW(PRIV_ETHER_NTOHOST) |
> - ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_GETLINES) |
> - ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
> + ALLOW(PRIV_GETRPCBYNUMBER) | ALLOW(PRIV_LOCALTIME) |
> + ALLOW(PRIV_PCAP_STATS),
> /* EXIT */ 0
> };
>
> @@ -90,21 +90,10 @@ static int allowed_ext[] = {
> /* INIT */ ALLOW(PRIV_SETFILTER),
> /* BPF */ ALLOW(PRIV_SETFILTER),
> /* FILTER */ALLOW(PRIV_GETSERVENTRIES),
> - /* RUN */ ALLOW(PRIV_GETLINES) | ALLOW(PRIV_LOCALTIME) |
> - ALLOW(PRIV_PCAP_STATS),
> + /* RUN */ ALLOW(PRIV_LOCALTIME) | ALLOW(PRIV_PCAP_STATS),
> /* EXIT */ 0
> };
>
> -struct ftab {
> - char *name;
> - int max;
> - int count;
> -};
> -
> -static struct ftab file_table[] = {{PF_OSFP_FILE, 1, 0}};
> -
> -#define NUM_FILETAB (sizeof(file_table) / sizeof(struct ftab))
> -
> int debug_level = LOG_INFO;
> int priv_fd = -1;
> volatile pid_t child_pid = -1;
> @@ -123,7 +112,6 @@ static void impl_getrpcbynumber(int);
> static void impl_getserventries(int);
> static void impl_getprotoentries(int);
> static void impl_localtime(int fd);
> -static void impl_getlines(int);
> static void impl_pcap_stats(int, int *);
>
> static void test_state(int, int);
> @@ -345,10 +333,6 @@ priv_exec(int argc, char *argv[])
> test_state(cmd, STATE_RUN);
> impl_localtime(sock);
> break;
> - case PRIV_GETLINES:
> - test_state(cmd, STATE_RUN);
> - impl_getlines(sock);
> - break;
> case PRIV_PCAP_STATS:
> test_state(cmd, STATE_RUN);
> impl_pcap_stats(sock, );
> @@ -577,55 +561,6 @@ impl_localtime(int fd)
> }
>
> static void
> -impl_getlines(int fd)
> -{
> - FILE *fp;
> - char *buf, *lbuf, *file;
> - size_t len, fid;
> -
> - logmsg(LOG_DEBUG, "[priv]: msg PRIV_GETLINES received");
> -
> - must_read(fd, , sizeof(size_t));
> - if (fid >= NUM_FILETAB)
> - errx(1, "invalid file id");
> -
> - file = file_table[fid].name;
> -
> - if (file == NULL)
> - errx(1, "invalid file referenced");
> -
> - if (file_table[fid].count >= file_table[fid].max)
> - errx(1, "maximum open count exceeded for %s", file);
> -
> - file_table[fid].count++;
> -
> - if ((fp = fopen(file, "r")) == NULL) {
> - write_zero(fd);
> - return;
> - }
> -
> - lbuf = NULL;
> - while ((buf = fgetln(fp, ))) {
> - if (buf[len - 1] == '\n')
> - buf[len - 1] = '\0';
> - else {
> - if ((lbuf = malloc(len + 1)) == NULL)
> - err(1, NULL);
> - memcpy(lbuf, buf, len);
> - lbuf[len] = '\0';
> - buf = lbuf;
> - }
> -
> - write_string(fd, buf);
> -
> -
unveil hotplugd
Hi,
hotplugd(8) needs to open(2) `device' with read permissions, /dev/hotplug by
default but can be changed via arguments. Then it needs read/execute on both
_PATH_ETC_HOTPLUG_{ATTACH,DETACH} to access(2) and execl(3) them.
Tested successfully attaching/dettaching (mount/umount) an USB pen. Comments?
OK?
Index: hotplugd.c
===
RCS file: /cvs/src/usr.sbin/hotplugd/hotplugd.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 hotplugd.c
--- hotplugd.c 31 Jul 2016 20:13:12 - 1.14
+++ hotplugd.c 7 Nov 2018 15:31:19 -
@@ -61,9 +61,6 @@ main(int argc, char *argv[])
struct sigaction sact;
struct hotplug_event he;
- if (pledge("stdio rpath proc exec", NULL) == -1)
- err(1, "pledge");
-
while ((ch = getopt(argc, argv, "d:")) != -1)
switch (ch) {
case 'd':
@@ -79,6 +76,15 @@ main(int argc, char *argv[])
argv += optind;
if (argc > 0)
usage();
+
+ if (unveil(device, "r") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_ETC_HOTPLUG_ATTACH, "rx") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_ETC_HOTPLUG_DETACH, "rx") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath proc exec", NULL) == -1)
+ err(1, "pledge");
if ((devfd = open(device, O_RDONLY | O_CLOEXEC)) == -1)
err(1, "%s", device);
unveil tcpdrop
Hi,
tcpdrop(8) needs to access only two files, in this case /etc/hosts and
/etc/resolv.conf both with read permissions for the purpose of name resolution.
ethers(5) is not needed since we are not using any of the ether_*(3) family.
Since unistd.h needs to be included I also shuffled netdb.h into the right
place.
Comments? OK?
Index: tcpdrop.c
===
RCS file: /cvs/src/usr.sbin/tcpdrop/tcpdrop.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 tcpdrop.c
--- tcpdrop.c 16 Jan 2015 06:40:21 - 1.17
+++ tcpdrop.c 6 Nov 2018 10:48:10 -
@@ -27,10 +27,11 @@
#include
#include
+#include
#include
#include
#include
-#include
+#include
__dead void usage(void);
@@ -61,6 +62,13 @@ main(int argc, char **argv)
char *laddr1, *addr1, *port1, *faddr2, *addr2, *port2;
struct tcp_ident_mapping tir;
int gaierr, rval = 0;
+
+ if (unveil("/etc/hosts", "r") == -1)
+ err(1, "unveil");
+ if (unveil("/etc/resolv.conf", "r") == -1)
+ err(1, "unveil");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
memset(, 0, sizeof(hints));
hints.ai_family = AF_UNSPEC;
unveil chpass
Hi,
chpass(1) without parameters enters in edit mode by default, in here it will
need to execute _PATH_BSHELL to spawn a new EDITOR, _PATH_SHELLS to check
(read) if we are changing from/to a non-standard shell (in case we are not
root) and read access to `tempname' to verify if the file has valid entries and
create to unlink it.
If -s is used to change a user's shell then it will need read access to
_PATH_SHELLS by the same reason already mentioned above.
Unconditionally we need to unveil _PATH_MASTERPASSWD_LOCK with write/create
permissions, _PATH_MASTERPASSWD with read and _PATH_PWD_MKDB to execute
pwd_mkdb(8).
In the -a case I'm not unveiling /etc/spwd.db since we can get it through
pledge "getpw", which can be added for completeness of all code paths.
Note also that the first pledges need "unveil" since we will call unveil(2)
afterwards.
Tested all paths successfully with a test account. Comments? OK?
Index: chpass.c
===
RCS file: /cvs/src/usr.bin/chpass/chpass.c,v
retrieving revision 1.44
diff -u -p -u -r1.44 chpass.c
--- chpass.c8 Dec 2017 17:04:15 - 1.44
+++ chpass.c7 Nov 2018 12:50:07 -
@@ -136,7 +136,13 @@ main(int argc, char *argv[])
pw_error(tempname, 1, 1);
display(tempname, dfd, pw);
- if (pledge("stdio rpath wpath cpath id proc exec",
+ if (unveil(_PATH_BSHELL, "x") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_SHELLS, "r") == -1)
+ err(1, "unveil");
+ if (unveil(tempname, "rc") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath wpath cpath id proc exec unveil",
NULL) == -1)
err(1, "pledge");
@@ -158,7 +164,9 @@ main(int argc, char *argv[])
}
if (op == NEWSH) {
- if (pledge("stdio rpath wpath cpath id proc exec",
+ if (unveil(_PATH_SHELLS, "r") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath wpath cpath id proc exec unveil",
NULL) == -1)
err(1, "pledge");
@@ -175,6 +183,12 @@ main(int argc, char *argv[])
sigdelset(, SIGINT);
sigprocmask(SIG_BLOCK, , NULL);
+ if (unveil(_PATH_MASTERPASSWD_LOCK, "wc") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_MASTERPASSWD, "r") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_PWD_MKDB, "x") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath wpath cpath proc exec", NULL) == -1)
err(1, "pledge");
Re: unveil dhclient (privileged process)
something like the below? I added a new define for /etc/resolv.conf since it's
now used on 2 different places and hardcoded the executable path to avoid
strange errors if running from a symlink directory as pointed out by remi@
Index: dhclient.c
===
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.581
diff -u -p -u -r1.581 dhclient.c
--- dhclient.c 4 Nov 2018 19:10:34 - 1.581
+++ dhclient.c 6 Nov 2018 07:34:55 -
@@ -2234,6 +2234,13 @@ fork_privchld(struct interface_info *ifi
if ((routefd = socket(AF_ROUTE, SOCK_RAW, 0)) == -1)
fatal("socket(AF_ROUTE, SOCK_RAW)");
+ if (unveil(_PATH_RESOLV_CONF, "wc") == -1)
+ fatal("unveil");
+ if (unveil("/sbin/dhclient", "x") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
while (quit == 0) {
pfd[0].fd = priv_ibuf->fd;
pfd[0].events = POLLIN;
Index: dhcpd.h
===
RCS file: /cvs/src/sbin/dhclient/dhcpd.h,v
retrieving revision 1.257
diff -u -p -u -r1.257 dhcpd.h
--- dhcpd.h 2 Nov 2018 16:15:55 - 1.257
+++ dhcpd.h 6 Nov 2018 07:34:55 -
@@ -153,6 +153,7 @@ struct interface_info {
};
#define_PATH_DHCLIENT_CONF "/etc/dhclient.conf"
+#define_PATH_RESOLV_CONF "/etc/resolv.conf"
#define_PATH_LEASE_DB "/var/db/dhclient.leases"
/* options.c */
Index: kroute.c
===
RCS file: /cvs/src/sbin/dhclient/kroute.c,v
retrieving revision 1.156
diff -u -p -u -r1.156 kroute.c
--- kroute.c13 Jun 2018 01:37:54 - 1.156
+++ kroute.c6 Nov 2018 07:34:55 -
@@ -594,7 +594,6 @@ write_resolv_conf(void)
void
priv_write_resolv_conf(char *contents)
{
- const char *path = "/etc/resolv.conf";
ssize_t n;
size_t sz;
int fd;
@@ -602,21 +601,21 @@ priv_write_resolv_conf(char *contents)
if (contents == NULL)
return;
- fd = open(path, O_WRONLY | O_CREAT | O_TRUNC,
+ fd = open(_PATH_RESOLV_CONF, O_WRONLY | O_CREAT | O_TRUNC,
S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
if (fd == -1) {
- log_warn("%s: open(%s)", log_procname, path);
+ log_warn("%s: open(%s)", log_procname, _PATH_RESOLV_CONF);
return;
}
sz = strlen(contents);
n = write(fd, contents, sz);
if (n == -1)
- log_warn("%s: write(%s)", log_procname, path);
+ log_warn("%s: write(%s)", log_procname, _PATH_RESOLV_CONF);
else if ((size_t)n < sz)
log_warnx("%s: write(%s): %zd of %zu bytes", log_procname,
- path, n, sz);
+ _PATH_RESOLV_CONF, n, sz);
close(fd);
}
Re: unveil dhclient (privileged process)
As per krw@ I probably should add a #define to /sbin/dhclient and use
that instead of saved_argv and you wouldn't have that error but you'd still
have to make install.
On 22:53 Mon 05 Nov , Remi Locherer wrote:
> On Mon, Nov 05, 2018 at 12:30:08PM +0000, Ricardo Mestre wrote:
> > Hi,
> >
> > dhclient(8)'s privileged process cannot be pledged yet due to some route
> > related sysctl(2)'s, but it seems it only needs to access two files. One is
> > /etc/resolv.conf with write/create permissions and saved_argv[0] (usually
> > /sbin/dhclient) with execute since we may receive a SIGHUP and it will need
> > to
> > re-exec itself. We could go further and keep /etc/resolv.conf veiled if we
> > superseed both domain-name and domain-name-servers in the config file, but
> > it
> > seems a bit overkill, and with the simple diff below I didn't have any
> > problems.
> >
> > Comments? OK? Cluebat stick?
>
> First I thougt the diff does not work:
>
> typhoon ..n/dhclient$ doas obj/dhclient -d iwm0
> fatal in iwm0 [priv]: unveil: No such file or directory
> iwm0: DHCPREQUEST to 255.255.255.255
> iwm0: unpriv_ibuf: ERR|HUP|NVAL
>
>
> It does not work because "obj" is a symlink here. When called without
> a symlink in the path it works as expected. The error message is a bit
> awkward.
>
> >
> > Index: dhclient.c
> > ===
> > RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
> > retrieving revision 1.581
> > diff -u -p -u -r1.581 dhclient.c
> > --- dhclient.c 4 Nov 2018 19:10:34 - 1.581
> > +++ dhclient.c 5 Nov 2018 12:02:51 -
> > @@ -2234,6 +2234,13 @@ fork_privchld(struct interface_info *ifi
> > if ((routefd = socket(AF_ROUTE, SOCK_RAW, 0)) == -1)
> > fatal("socket(AF_ROUTE, SOCK_RAW)");
> >
> > + if (unveil("/etc/resolv.conf", "wc") == -1)
> > + fatal("unveil");
> > + if (unveil(saved_argv[0], "x") == -1)
> > + fatal("unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + fatal("unveil");
> > +
> > while (quit == 0) {
> > pfd[0].fd = priv_ibuf->fd;
> > pfd[0].events = POLLIN;
> >
>
unveil dhclient (privileged process)
Hi,
dhclient(8)'s privileged process cannot be pledged yet due to some route
related sysctl(2)'s, but it seems it only needs to access two files. One is
/etc/resolv.conf with write/create permissions and saved_argv[0] (usually
/sbin/dhclient) with execute since we may receive a SIGHUP and it will need to
re-exec itself. We could go further and keep /etc/resolv.conf veiled if we
superseed both domain-name and domain-name-servers in the config file, but it
seems a bit overkill, and with the simple diff below I didn't have any
problems.
Comments? OK? Cluebat stick?
Index: dhclient.c
===
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.581
diff -u -p -u -r1.581 dhclient.c
--- dhclient.c 4 Nov 2018 19:10:34 - 1.581
+++ dhclient.c 5 Nov 2018 12:02:51 -
@@ -2234,6 +2234,13 @@ fork_privchld(struct interface_info *ifi
if ((routefd = socket(AF_ROUTE, SOCK_RAW, 0)) == -1)
fatal("socket(AF_ROUTE, SOCK_RAW)");
+ if (unveil("/etc/resolv.conf", "wc") == -1)
+ fatal("unveil");
+ if (unveil(saved_argv[0], "x") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
while (quit == 0) {
pfd[0].fd = priv_ibuf->fd;
pfd[0].events = POLLIN;
Re: pledge xenodm
this is actually good, I made it as minimal as possible so that it
wouldn't break for me, if it breaks for you we can start from there.
On 13:41 Sat 03 Nov , Matthieu Herrb wrote:
> On Fri, Nov 02, 2018 at 07:03:11PM +0000, Ricardo Mestre wrote:
> > Hi,
> >
> > Looking at pledging xenodm a little bit more I was able to run it with
> > the diff below across X restarts, with stock configuration.
> >
> > Please test it, if you have special configs better since most likely
> > this will break it but we need to know where and why and with that maybe
> > trim xenodm's bloat.
>
> Hi,
>
> xenodm crashes if one logs out from the first session that was
> created. the pldege() call in StartDisplay is too strict.
>
> I've not had time to fully figure out if this can be fixed. Iirc I
> tried when I did the initial work on adding pledge to xenodm and it's
> not possible because of the code that creates the auth cookie and
> chown()s it to _x11.
>
> I didn't have time yet to do a more serious testing of the session
> pledge.
>
> >
> > DisplayManager:
> > rpath: open(2) /etc/X11/xenodm/xenodm-config
> > cpath: unlink(2) d->authFile
> > proc: kill(2)
> >
> > Session:
> > rpath: open(2) /etc/fbtab
> > wpath/cpath: open(2) d->authFile
> > fattr: chmod(2) d->authFile
> > dns: sysctl(2) for name resolution
> > proc: kill(2)
> > exec: execve(2) /etc/X11/xenodm/TakeConsole
> > id: setuid(2)
> >
> > Index: dm.c
> > ===
> > RCS file: /cvs/xenocara/app/xenodm/xenodm/dm.c,v
> > retrieving revision 1.6
> > diff -u -p -u -r1.6 dm.c
> > --- dm.c11 Jul 2018 16:57:04 - 1.6
> > +++ dm.c31 Oct 2018 15:15:49 -
> > @@ -604,6 +604,10 @@ StartDisplay (struct display *d)
> > Debug ("pid: %d\n", pid);
> > d->pid = pid;
> > d->status = running;
> > +
> > + if (pledge("stdio rpath cpath proc", NULL) == -1)
> > + exit(OPENFAILED_DISPLAY);
> > +
> > break;
> > }
> > }
> > Index: session.c
> > ===
> > RCS file: /cvs/xenocara/app/xenodm/xenodm/session.c,v
> > retrieving revision 1.12
> > diff -u -p -u -r1.12 session.c
> > --- session.c 11 Jul 2018 20:28:41 - 1.12
> > +++ session.c 31 Oct 2018 15:15:49 -
> > @@ -378,6 +378,10 @@ StartClient (
> > default:
> > Debug ("StartSession, fork succeeded %d\n", pid);
> > *pidp = pid;
> > +
> > + if (pledge("stdio rpath wpath cpath fattr dns proc exec id", NULL) ==
> > -1)
> > + exit(25);
> > +
> > return 1;
> > }
> > }
> >
> > - End forwarded message -
>
> --
> Matthieu Herrb
>
Re: pledge xenodm
prodded by deraadt@, here's a rebased diff on xenocara's source root directory,
usually /usr/xenocara.
Index: app/xenodm/xenodm/dm.c
===
RCS file: /cvs/xenocara/app/xenodm/xenodm/dm.c,v
retrieving revision 1.6
diff -u -p -u -r1.6 dm.c
--- app/xenodm/xenodm/dm.c 11 Jul 2018 16:57:04 - 1.6
+++ app/xenodm/xenodm/dm.c 3 Nov 2018 11:10:06 -
@@ -604,6 +604,10 @@ StartDisplay (struct display *d)
Debug ("pid: %d\n", pid);
d->pid = pid;
d->status = running;
+
+ if (pledge("stdio rpath cpath proc", NULL) == -1)
+ exit(OPENFAILED_DISPLAY);
+
break;
}
}
Index: app/xenodm/xenodm/session.c
===
RCS file: /cvs/xenocara/app/xenodm/xenodm/session.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 session.c
--- app/xenodm/xenodm/session.c 11 Jul 2018 20:28:41 - 1.12
+++ app/xenodm/xenodm/session.c 3 Nov 2018 11:10:06 -
@@ -378,6 +378,10 @@ StartClient (
default:
Debug ("StartSession, fork succeeded %d\n", pid);
*pidp = pid;
+
+ if (pledge("stdio rpath wpath cpath fattr dns proc exec id", NULL) ==
-1)
+ exit(25);
+
return 1;
}
}
Re: disable fs access on ripd
reads OK for me as well
On 10:28 Sat 03 Nov , Claudio Jeker wrote:
> On Sat, Nov 03, 2018 at 10:24:23AM +0100, Remi Locherer wrote:
> > On Tue, Oct 30, 2018 at 05:31:04PM +, Ricardo Mestre wrote:
> > > clearly an oversight due to looking at too many daemons at the same
> > > time. since the only thing ripd needs to do is unlink the socket I think
> > > we can remove control_cleanup, even though I'd rather do this
> > > introducing pledge, but for now it's a great compromise.
> > >
> >
> > In addition to your diff this pledges the rde and ripe.
> >
> > OK?
>
> Reads OK. Not sure about the ripe pledge but looking at ospfd I think it
> should be right. Can't test right now.
>
> > Index: control.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ripd/control.c,v
> > retrieving revision 1.25
> > diff -u -p -r1.25 control.c
> > --- control.c 17 Jan 2017 22:10:56 - 1.25
> > +++ control.c 3 Nov 2018 09:11:39 -
> > @@ -100,14 +100,6 @@ control_listen(void)
> > return (0);
> > }
> >
> > -void
> > -control_cleanup(char *path)
> > -{
> > - event_del(_state.ev);
> > - event_del(_state.evt);
> > - unlink(path);
> > -}
> > -
> > /* ARGSUSED */
> > void
> > control_accept(int listenfd, short event, void *bula)
> > Index: control.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ripd/control.h,v
> > retrieving revision 1.5
> > diff -u -p -r1.5 control.h
> > --- control.h 2 Aug 2016 16:05:32 - 1.5
> > +++ control.h 3 Nov 2018 09:11:20 -
> > @@ -39,6 +39,5 @@ int control_listen(void);
> > void control_accept(int, short, void *);
> > void control_dispatch_imsg(int, short, void *);
> > intcontrol_imsg_relay(struct imsg *);
> > -void control_cleanup(char *);
> >
> > #endif /* _CONTROL_H_ */
> > Index: rde.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ripd/rde.c,v
> > retrieving revision 1.21
> > diff -u -p -r1.21 rde.c
> > --- rde.c 3 Sep 2016 10:28:08 - 1.21
> > +++ rde.c 3 Nov 2018 07:38:41 -
> > @@ -109,6 +109,9 @@ rde(struct ripd_conf *xconf, int pipe_pa
> > setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
> > fatal("can't drop privileges");
> >
> > + if (pledge("stdio", NULL) == -1)
> > + fatal("pledge");
> > +
> > event_init();
> >
> > /* setup signal handler */
> > Index: ripd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ripd/ripd.c,v
> > retrieving revision 1.30
> > diff -u -p -r1.30 ripd.c
> > --- ripd.c 3 Sep 2016 10:28:08 - 1.30
> > +++ ripd.c 3 Nov 2018 09:14:38 -
> > @@ -211,6 +211,11 @@ main(int argc, char *argv[])
> > rde_pid = rde(conf, pipe_parent2rde, pipe_ripe2rde, pipe_parent2ripe);
> > ripe_pid = ripe(conf, pipe_parent2ripe, pipe_ripe2rde, pipe_parent2rde);
> >
> > + if (unveil("/", "") == -1)
> > + fatal("unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + fatal("unveil");
> > +
> > event_init();
> >
> > /* setup signal handler */
> > @@ -276,7 +281,6 @@ ripd_shutdown(void)
> > if_del(i);
> > }
> >
> > - control_cleanup(conf->csock);
> > kr_shutdown();
> >
> > log_debug("waiting for children to terminate");
> > Index: ripe.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ripd/ripe.c,v
> > retrieving revision 1.22
> > diff -u -p -r1.22 ripe.c
> > --- ripe.c 3 Sep 2016 10:28:08 - 1.22
> > +++ ripe.c 3 Nov 2018 09:07:24 -
> > @@ -196,6 +196,9 @@ ripe(struct ripd_conf *xconf, int pipe_p
> > iface->name);
> > }
> >
> > + if (pledge("stdio inet mcast", NULL) == -1)
> > + fatal("pledge");
> > +
> > evtimer_set(>report_timer, report_timer, oeconf);
> > start_report_timer();
> >
>
> --
> :wq Claudio
pledge xenodm
Hi,
Looking at pledging xenodm a little bit more I was able to run it with
the diff below across X restarts, with stock configuration.
Please test it, if you have special configs better since most likely
this will break it but we need to know where and why and with that maybe
trim xenodm's bloat.
DisplayManager:
rpath: open(2) /etc/X11/xenodm/xenodm-config
cpath: unlink(2) d->authFile
proc: kill(2)
Session:
rpath: open(2) /etc/fbtab
wpath/cpath: open(2) d->authFile
fattr: chmod(2) d->authFile
dns: sysctl(2) for name resolution
proc: kill(2)
exec: execve(2) /etc/X11/xenodm/TakeConsole
id: setuid(2)
Index: dm.c
===
RCS file: /cvs/xenocara/app/xenodm/xenodm/dm.c,v
retrieving revision 1.6
diff -u -p -u -r1.6 dm.c
--- dm.c11 Jul 2018 16:57:04 - 1.6
+++ dm.c31 Oct 2018 15:15:49 -
@@ -604,6 +604,10 @@ StartDisplay (struct display *d)
Debug ("pid: %d\n", pid);
d->pid = pid;
d->status = running;
+
+ if (pledge("stdio rpath cpath proc", NULL) == -1)
+ exit(OPENFAILED_DISPLAY);
+
break;
}
}
Index: session.c
===
RCS file: /cvs/xenocara/app/xenodm/xenodm/session.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 session.c
--- session.c 11 Jul 2018 20:28:41 - 1.12
+++ session.c 31 Oct 2018 15:15:49 -
@@ -378,6 +378,10 @@ StartClient (
default:
Debug ("StartSession, fork succeeded %d\n", pid);
*pidp = pid;
+
+ if (pledge("stdio rpath wpath cpath fattr dns proc exec id", NULL) ==
-1)
+ exit(25);
+
return 1;
}
}
- End forwarded message -
Re: disable fs access on ripd
>From my point of view, and my sole opinion, the process running as root
from a long standing daemon, and which is not under a chroot, should not have
the hability to write and/or create files (even better if it cannot even
read). But we should reach a consensus once again, and if control_cleanup()
should be brought back to the daemons I removed it from I won't be against it.
The combination of unveil/pledge, or even just by using unveil is very
powerful and if we by all means can limit the access the most we can to the
filesystem is really great against this type of attack vector.
On 21:53 Tue 30 Oct , Sebastian Benoit wrote:
> Florian Obser(flor...@openbsd.org) on 2018.10.30 18:32:15 +0100:
> > On Tue, Oct 30, 2018 at 10:54:10AM -0600, Theo de Raadt wrote:
> > > Remi Locherer wrote:
> > >
> > > > On Tue, Oct 30, 2018 at 03:20:35PM +, Ricardo Mestre wrote:
> > > > > Hi,
> > > > >
> > > > > After all files are opened ripd(8) can have the fs access disabled
> > > > > just before
> > > > > each process main loop. Its 2 childs already run under chroot, but
> > > > > since they
> > > > > are still not pledged at least they have no way to read/write/create
> > > > > files within
> > > > > the chroot. No loads or reloads of the config file happen through any
> > > > > signal,
> > > > > nor can we do it via ripctl(8).
> > > > >
> > > > > I was able to run a simple daemon with the example file. Comments? OK?
> > > >
> > > > control_cleanup() unlinks the control socket on exit. I think you should
> > > > either unveil(conf->csock, "c") or remove control_cleanup().
> > >
> > > I don't understand this latter comment, let me ask.
> > >
> > > You think it is smart to leave these sockets lying around?
> > >
> >
> > Back in the summer the consensus was that it is fine to leave the
> > sockets lying around. Furthermore the consensus was that it is worth
> > to do so if we can drop the cpath pledge in daemons. (IIRC at least benno,
> > claudio, deraadt, florian and mestre where in the loop.)
> >
> > mestre went on a rampage and removed control_cleanup and cpath pledge
> > in a bunch of daemons. I think he stopped when he couldn't find any
> > more daemons where the parent process was pledged.
> >
> > This was before the
> > unveil("/", "r");
> > unveil(control_socket, "rwc");
> > pattern was discovered which is also very powerful.
> >
> > > I suspect there are a few oddball programs where it makes senes, but as
> > > a general rule I think it is a bad idea; as stated in other threads it
> > > means control programs and restart sequences have a bunch more oddball
> > > behaviours which will be inconsistant.
> >
> > I want consistency, if we go with the unveil pattern, fine, then we
> > should restore the control_cleanup code in a bunch of daemons.
> >
> > However:
> > - the control program behaviour change had been discussed in the
> > summer and the only difference discovered then was "file not found" vs.
> > "connection refused".
> > - benno's comment about only one instance of ospfd running is not
> > effected by leaving the socket behind, it checks if the socket has a
> > listener. a dead socket is no problem.
> > - daemons/routers might crash and leave a socket behind anyway. they
> > must be able to deal with this and I'm pretty sure they all do. I just
> > checked ospfd, bgpd and rad.
>
> i agree with that. In the summer i first thought it would be better to clean
> up, but the discussion back then convinced me otherwise, and florian summed
> it up nicely.
Re: disable fs access on ripd
clearly an oversight due to looking at too many daemons at the same
time. since the only thing ripd needs to do is unlink the socket I think
we can remove control_cleanup, even though I'd rather do this
introducing pledge, but for now it's a great compromise.
OK?
Index: control.c
===
RCS file: /cvs/src/usr.sbin/ripd/control.c,v
retrieving revision 1.25
diff -u -p -u -r1.25 control.c
--- control.c 17 Jan 2017 22:10:56 - 1.25
+++ control.c 30 Oct 2018 17:24:44 -
@@ -100,14 +100,6 @@ control_listen(void)
return (0);
}
-void
-control_cleanup(char *path)
-{
- event_del(_state.ev);
- event_del(_state.evt);
- unlink(path);
-}
-
/* ARGSUSED */
void
control_accept(int listenfd, short event, void *bula)
Index: control.h
===
RCS file: /cvs/src/usr.sbin/ripd/control.h,v
retrieving revision 1.5
diff -u -p -u -r1.5 control.h
--- control.h 2 Aug 2016 16:05:32 - 1.5
+++ control.h 30 Oct 2018 17:24:44 -
@@ -39,6 +39,5 @@ int control_listen(void);
void control_accept(int, short, void *);
void control_dispatch_imsg(int, short, void *);
intcontrol_imsg_relay(struct imsg *);
-void control_cleanup(char *);
#endif /* _CONTROL_H_ */
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/ripd/rde.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 rde.c
--- rde.c 3 Sep 2016 10:28:08 - 1.21
+++ rde.c 30 Oct 2018 17:24:44 -
@@ -151,6 +151,11 @@ rde(struct ripd_conf *xconf, int pipe_pa
free(r);
}
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
rde_shutdown();
Index: ripd.c
===
RCS file: /cvs/src/usr.sbin/ripd/ripd.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 ripd.c
--- ripd.c 3 Sep 2016 10:28:08 - 1.30
+++ ripd.c 30 Oct 2018 17:24:45 -
@@ -251,6 +251,11 @@ main(int argc, char *argv[])
conf->rdomain) == -1)
fatalx("kr_init failed");
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
ripd_shutdown();
@@ -276,7 +281,6 @@ ripd_shutdown(void)
if_del(i);
}
- control_cleanup(conf->csock);
kr_shutdown();
log_debug("waiting for children to terminate");
Index: ripe.c
===
RCS file: /cvs/src/usr.sbin/ripd/ripe.c,v
retrieving revision 1.22
diff -u -p -u -r1.22 ripe.c
--- ripe.c 3 Sep 2016 10:28:08 - 1.22
+++ ripe.c 30 Oct 2018 17:24:45 -
@@ -201,6 +201,11 @@ ripe(struct ripd_conf *xconf, int pipe_p
ripe_imsg_compose_rde(IMSG_FULL_REQUEST, 0, 0, NULL, 0);
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
ripe_shutdown();
On 11:15 Tue 30 Oct , Theo de Raadt wrote:
> Remi Locherer wrote:
>
> > On Tue, Oct 30, 2018 at 10:54:10AM -0600, Theo de Raadt wrote:
> > > Remi Locherer wrote:
> > >
> > > > On Tue, Oct 30, 2018 at 03:20:35PM +, Ricardo Mestre wrote:
> > > > > Hi,
> > > > >
> > > > > After all files are opened ripd(8) can have the fs access disabled
> > > > > just before
> > > > > each process main loop. Its 2 childs already run under chroot, but
> > > > > since they
> > > > > are still not pledged at least they have no way to read/write/create
> > > > > files within
> > > > > the chroot. No loads or reloads of the config file happen through any
> > > > > signal,
> > > > > nor can we do it via ripctl(8).
> > > > >
> > > > > I was able to run a simple daemon with the example file. Comments? OK?
> > > >
> > > > control_cleanup() unlinks the control socket on exit. I think you should
> > > > either unveil(conf->csock, "c") or remove control_cleanup().
> > >
> > > I don't understand this latter comment, let me ask.
> > >
> > > You think it is smart to leave these sockets lying around?
> > >
> > > I suspect there are a few oddball programs where it makes senes, but as
> > > a general rule I think it is a bad idea; as stated in other threads it
> > > means control programs and restart sequences have a bunch more oddball
> > > behaviours which will be inconsistant.
> > >
> >
> > I prefer if sockets get removed on exit. But I was not sure if this is
> > just my personal taste.
>
> I didn't speak about taste, I spoke about behaviour relative to the
> control program. Why do you propose potentially changing behaviour
> without checking first?
>
disable fs access on ripd
Hi,
After all files are opened ripd(8) can have the fs access disabled just before
each process main loop. Its 2 childs already run under chroot, but since they
are still not pledged at least they have no way to read/write/create files
within
the chroot. No loads or reloads of the config file happen through any signal,
nor can we do it via ripctl(8).
I was able to run a simple daemon with the example file. Comments? OK?
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/ripd/rde.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 rde.c
--- rde.c 3 Sep 2016 10:28:08 - 1.21
+++ rde.c 30 Oct 2018 15:09:44 -
@@ -151,6 +151,11 @@ rde(struct ripd_conf *xconf, int pipe_pa
free(r);
}
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
rde_shutdown();
Index: ripd.c
===
RCS file: /cvs/src/usr.sbin/ripd/ripd.c,v
retrieving revision 1.30
diff -u -p -u -r1.30 ripd.c
--- ripd.c 3 Sep 2016 10:28:08 - 1.30
+++ ripd.c 30 Oct 2018 15:09:44 -
@@ -251,6 +251,11 @@ main(int argc, char *argv[])
conf->rdomain) == -1)
fatalx("kr_init failed");
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
ripd_shutdown();
Index: ripe.c
===
RCS file: /cvs/src/usr.sbin/ripd/ripe.c,v
retrieving revision 1.22
diff -u -p -u -r1.22 ripe.c
--- ripe.c 3 Sep 2016 10:28:08 - 1.22
+++ ripe.c 30 Oct 2018 15:09:44 -
@@ -201,6 +201,11 @@ ripe(struct ripd_conf *xconf, int pipe_p
ripe_imsg_compose_rde(IMSG_FULL_REQUEST, 0, 0, NULL, 0);
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
ripe_shutdown();
disable fs access on snmpd
Hi,
snmpd(8)'s main process needs to open the config file and /dev/pf both with
read permissions, but once it reaches pledge(2) just before the main loop both
were already opened. Since snmpd(8) doesn't have a way to load or reload the
config file, not even through SIGHUP, then rpath promise is not needed.
The snmpe process cannot yet be pledged, but it doesn't need fs access so we
can disable the access through unveil("/", ""); unveil(NULL, NULL);
The traphandler is already pledged to not access the fs at all.
With both modifications the regress tests still pass. Comments? OK?
Index: snmpd.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 snmpd.c
--- snmpd.c 5 Aug 2018 09:33:13 - 1.39
+++ snmpd.c 30 Oct 2018 14:03:38 -
@@ -255,7 +255,7 @@ main(int argc, char *argv[])
proc_connect(ps);
- if (pledge("stdio rpath dns sendfd proc exec id", NULL) == -1)
+ if (pledge("stdio dns sendfd proc exec id", NULL) == -1)
fatal("pledge");
event_dispatch();
Index: snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.54
diff -u -p -u -r1.54 snmpe.c
--- snmpe.c 31 Jul 2018 11:01:29 - 1.54
+++ snmpe.c 30 Oct 2018 14:03:38 -
@@ -120,6 +120,10 @@ snmpe_init(struct privsep *ps, struct pr
event_add(>s_ev, NULL);
}
+ if (unveil("/", "") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
#if 0
/*
* XXX Refactoring required to move illegal ioctls and sysctls.
unveil ifstated
Hi,
ifstated(8) needs to load configfile from within the main loop, but also to
reload it on SIGHUP so unveil(2) it with read permissions. Additionally all
commands are exec'ed through /bin/sh instead of directly so we can just
unveil(2) /bin/sh with x perms. Since /bin/sh is already used on another place
I used _PATH_BSHELL.
Both regress tests passed. Comments? OK?
Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.61
diff -u -p -u -r1.61 ifstated.c
--- ifstated.c 30 Aug 2017 16:14:52 - 1.61
+++ ifstated.c 30 Oct 2018 12:01:20 -
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
#include
@@ -160,6 +161,10 @@ main(int argc, char *argv[])
, sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt tablefilter", __func__);
+ if (unveil(configfile, "r") == -1)
+ fatal("unveil");
+ if (unveil(_PATH_BSHELL, "x") == -1)
+ fatal("unveil");
if (pledge("stdio rpath route proc exec", NULL) == -1)
fatal("pledge");
@@ -326,7 +331,7 @@ external_exec(struct ifsd_external *exte
if (pid < 0) {
log_warn("fork error");
} else if (pid == 0) {
- execv("/bin/sh", argp);
+ execv(_PATH_BSHELL, argp);
_exit(1);
/* NOTREACHED */
} else {
unveil htpasswd
Hi,
htpasswd(1) when in batch mode (-I) and 1 argument is used, or when not in
batch mode and 2 arguments are used we know we have to access argv[0] with rwc
permissions and also to rwc a temporary file in /tmp so we can unveil(2) both
argv[0] and /tmp with rwc permissions. In order to avoid adding "unveil" to
pledge(2), just call it after getopt(3).
Remaining code paths already have fs access disabled via pledge(2).
Comments? OK?
Index: htpasswd.c
===
RCS file: /cvs/src/usr.bin/htpasswd/htpasswd.c,v
retrieving revision 1.16
diff -u -p -u -r1.16 htpasswd.c
--- htpasswd.c 7 Jun 2017 09:11:52 - 1.16
+++ htpasswd.c 30 Oct 2018 08:55:45 -
@@ -57,9 +57,6 @@ main(int argc, char** argv)
ssize_t linelen;
mode_t old_umask;
- if (pledge("stdio rpath wpath cpath flock tmppath tty", NULL) == -1)
- err(1, "pledge");
-
while ((c = getopt(argc, argv, "I")) != -1) {
switch (c) {
case 'I':
@@ -74,6 +71,15 @@ main(int argc, char** argv)
argc -= optind;
argv += optind;
+
+ if ((batch && argc == 1) || (!batch && argc == 2)) {
+ if (unveil(argv[0], "rwc") == -1)
+ err(1, "unveil");
+ if (unveil("/tmp", "rwc") == -1)
+ err(1, "unveil");
+ }
+ if (pledge("stdio rpath wpath cpath flock tmppath tty", NULL) == -1)
+ err(1, "pledge");
if (batch) {
if (argc == 1)
Re: unveil ospfd's parent proc
Not all daemons have the same behaviour, so if this is still used then
Remi's diff of course makes more sense.
On 19:24 Sun 28 Oct , Sebastian Benoit wrote:
> Ricardo Mestre(ser...@helheim.mooo.com) on 2018.10.28 17:26:24 +:
> in ospfd, ospf6d (and hopefully soon bgpd) we make sure the daemon does not
> run twice by checking the socket.
>
> Since it really is bad if you run two ospfd, please keep this.
>
> /Benno
>
>
> >
> > Index: control.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
> > retrieving revision 1.45
> > diff -u -p -u -r1.45 control.c
> > --- control.c 29 Aug 2018 08:43:16 - 1.45
> > +++ control.c 28 Oct 2018 17:24:46 -
> > @@ -124,16 +124,6 @@ control_listen(void)
> > return (0);
> > }
> >
> > -void
> > -control_cleanup(char *path)
> > -{
> > - if (path == NULL)
> > - return;
> > - event_del(_state.ev);
> > - event_del(_state.evt);
> > - unlink(path);
> > -}
> > -
> > /* ARGSUSED */
> > void
> > control_accept(int listenfd, short event, void *bula)
> > Index: control.h
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
> > retrieving revision 1.7
> > diff -u -p -u -r1.7 control.h
> > --- control.h 29 Aug 2018 08:43:16 - 1.7
> > +++ control.h 28 Oct 2018 17:24:46 -
> > @@ -40,6 +40,5 @@ int control_listen(void);
> > void control_accept(int, short, void *);
> > void control_dispatch_imsg(int, short, void *);
> > intcontrol_imsg_relay(struct imsg *);
> > -void control_cleanup(char *);
> >
> > #endif /* _CONTROL_H_ */
> > Index: ospfd.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> > retrieving revision 1.100
> > diff -u -p -u -r1.100 ospfd.c
> > --- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> > +++ ospfd.c 28 Oct 2018 17:24:46 -
> > @@ -288,6 +288,11 @@ main(int argc, char *argv[])
> > area_del(a);
> > }
> >
> > + if (unveil("/", "r") == -1)
> > + fatal("unveil");
> > + if (unveil(NULL, NULL) == -1)
> > + fatal("unveil");
> > +
> > event_dispatch();
> >
> > ospfd_shutdown();
> > @@ -308,7 +313,6 @@ ospfd_shutdown(void)
> > msgbuf_clear(_rde->ibuf.w);
> > close(iev_rde->ibuf.fd);
> >
> > - control_cleanup(ospfd_conf->csock);
> > while ((r = SIMPLEQ_FIRST(_conf->redist_list)) != NULL) {
> > SIMPLEQ_REMOVE_HEAD(_conf->redist_list, entry);
> > free(r);
> >
> > On 15:58 Sun 28 Oct , Florian Obser wrote:
> > > Sorry, I'm on a phone. The diff context looks like the control FD is
> > > already open at this point. Does ospfd later re-open it?
> > >
> > > On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> > > wrote:
> > > >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> > > >> Remi Locherer wrote:
> > > >>
> > > >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> > > >> > > This breaks usage of the "include" keyword. Something that all
> > > >the parse.y daemons support.
> > > >> > >
> > > >> >
> > > >> > Oh, of course!
> > > >> >
> > > >> > I guess this is similar to unveil files based on a list of command
> > > >line args.
> > > >>
> > > >> correct.
> > > >>
> > > >> Now that unveil is used in the tree, there are 3 types of programs
> > > >>
> > > >> 1) they use unveil
> > > >> 2) they use pledge, heading close towards "stdio" without a "*path"
> > > >> 3) they access arbitrary files based upon argv
> > > >>
> > > >> this is (3), except not argv, it nested inside the config file
> > > >>
> > > >> Well there are maybe 20 programs beyond that which aren't converted
> > > >yet,
> > > >> but things are looking pretty good.
> > > >>
> > > >
> > > >Since ospfd is not suppose to write or execute f
Re: unveil ospfd's parent proc
Correct, and I'd go even further by not unveiling the socket at all. A few
weeks ago I removed the logic of unlinking the socket when the program stops,
for a few daemons, but left untouched the ones that don't have the main process
pledged since it wouldn't make much difference.
If we want to go this way then I think we also should remove the logic for
these daemons and adding unveil just like the below.
Comments? OK?
Index: control.c
===
RCS file: /cvs/src/usr.sbin/ospfd/control.c,v
retrieving revision 1.45
diff -u -p -u -r1.45 control.c
--- control.c 29 Aug 2018 08:43:16 - 1.45
+++ control.c 28 Oct 2018 17:24:46 -
@@ -124,16 +124,6 @@ control_listen(void)
return (0);
}
-void
-control_cleanup(char *path)
-{
- if (path == NULL)
- return;
- event_del(_state.ev);
- event_del(_state.evt);
- unlink(path);
-}
-
/* ARGSUSED */
void
control_accept(int listenfd, short event, void *bula)
Index: control.h
===
RCS file: /cvs/src/usr.sbin/ospfd/control.h,v
retrieving revision 1.7
diff -u -p -u -r1.7 control.h
--- control.h 29 Aug 2018 08:43:16 - 1.7
+++ control.h 28 Oct 2018 17:24:46 -
@@ -40,6 +40,5 @@ int control_listen(void);
void control_accept(int, short, void *);
void control_dispatch_imsg(int, short, void *);
intcontrol_imsg_relay(struct imsg *);
-void control_cleanup(char *);
#endif /* _CONTROL_H_ */
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.100
diff -u -p -u -r1.100 ospfd.c
--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
+++ ospfd.c 28 Oct 2018 17:24:46 -
@@ -288,6 +288,11 @@ main(int argc, char *argv[])
area_del(a);
}
+ if (unveil("/", "r") == -1)
+ fatal("unveil");
+ if (unveil(NULL, NULL) == -1)
+ fatal("unveil");
+
event_dispatch();
ospfd_shutdown();
@@ -308,7 +313,6 @@ ospfd_shutdown(void)
msgbuf_clear(_rde->ibuf.w);
close(iev_rde->ibuf.fd);
- control_cleanup(ospfd_conf->csock);
while ((r = SIMPLEQ_FIRST(_conf->redist_list)) != NULL) {
SIMPLEQ_REMOVE_HEAD(_conf->redist_list, entry);
free(r);
On 15:58 Sun 28 Oct , Florian Obser wrote:
> Sorry, I'm on a phone. The diff context looks like the control FD is already
> open at this point. Does ospfd later re-open it?
>
> On October 27, 2018 11:25:58 PM GMT+02:00, Remi Locherer
> wrote:
> >On Fri, Oct 26, 2018 at 10:19:01AM -0600, Theo de Raadt wrote:
> >> Remi Locherer wrote:
> >>
> >> > On Fri, Oct 26, 2018 at 06:01:40PM +0200, Florian Obser wrote:
> >> > > This breaks usage of the "include" keyword. Something that all
> >the parse.y daemons support.
> >> > >
> >> >
> >> > Oh, of course!
> >> >
> >> > I guess this is similar to unveil files based on a list of command
> >line args.
> >>
> >> correct.
> >>
> >> Now that unveil is used in the tree, there are 3 types of programs
> >>
> >> 1) they use unveil
> >> 2) they use pledge, heading close towards "stdio" without a "*path"
> >> 3) they access arbitrary files based upon argv
> >>
> >> this is (3), except not argv, it nested inside the config file
> >>
> >> Well there are maybe 20 programs beyond that which aren't converted
> >yet,
> >> but things are looking pretty good.
> >>
> >
> >Since ospfd is not suppose to write or execute files we could make the
> >file system read only (with the exception of the control socket).
> >
> >(Once we can add pledge to ospfd's parent proc this will probably not
> >make
> >sense anymore.)
> >
> >
> >
> >cvs diff: Diffing .
> >Index: ospfd.c
> >===
> >RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> >retrieving revision 1.100
> >diff -u -p -r1.100 ospfd.c
> >--- ospfd.c 29 Aug 2018 08:43:17 - 1.100
> >+++ ospfd.c 27 Oct 2018 07:28:58 -
> >@@ -278,6 +278,13 @@ main(int argc, char *argv[])
> > fatalx("control socket setup failed");
> > main_imsg_compose_ospfe_fd(IMSG_CONTROLFD, 0, control_fd);
> >
> >+if (unveil("/", "r") == -1)
> >+fatal("unveil");
> >+if (unveil(ospfd_conf->csock, "c") == -1)
> >+fatal("unveil");
> >+if (unveil(NULL, NULL) == -1)
> >+fatal("unveil");
> >+
> > if (kr_init(!(ospfd_conf->flags & OSPFD_FLAG_NO_FIB_UPDATE),
> > ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1)
> > fatalx("kr_init failed");
unveil getconf
Hi,
The code path were we pass `pathname' in the arguments is already limited
with pledge(2), but since we know exactly what it is then we can go further and
also unveil(2) it with read permissions.
Comments? OK?
Index: getconf.c
===
RCS file: /cvs/src/usr.bin/getconf/getconf.c,v
retrieving revision 1.19
diff -u -p -u -r1.19 getconf.c
--- getconf.c 28 Oct 2016 07:22:59 - 1.19
+++ getconf.c 25 Oct 2018 10:12:31 -
@@ -513,6 +513,8 @@ main(int argc, char *argv[])
break;
case PATHCONF:
+ if (unveil(argv[1], "r") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
errno = 0;
unveil kvm_mkdb
Hi,
If we pass `file' via args then we need to unveil(2) it with read permission,
otherwise if omitted we need to unveil(2) both _PATH_UNIX and _PATH_KSYMS with
same permissions.
Unconditionally we need to also unveil(2) dbdir, which by default is
_PATH_VARDB but can be changed via args (-o directory), with read/write/create
permissions. There are a couple of temp files that will be created but it's
inside dbdir so there's no need to unveil(2) them individually.
Since we already call pledge(2) before, twice, we need to add "unveil" promise
to both of them, and finally call pledge(2) once again with the needed promises
except "unveil".
Comments? OK?
Index: kvm_mkdb.c
===
RCS file: /cvs/src/usr.sbin/kvm_mkdb/kvm_mkdb.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 kvm_mkdb.c
--- kvm_mkdb.c 21 Nov 2017 12:07:00 - 1.29
+++ kvm_mkdb.c 25 Oct 2018 09:45:31 -
@@ -70,7 +70,7 @@ main(int argc, char *argv[])
char *nlistpath, *nlistname;
char dbdir[PATH_MAX];
- if (pledge("stdio rpath wpath cpath fattr getpw flock id", NULL) == -1)
+ if (pledge("stdio rpath wpath cpath fattr getpw flock id unveil", NULL)
== -1)
err(1, "pledge");
/* Try to use the kmem group to be able to fchown() in kvm_mkdb(). */
@@ -89,7 +89,7 @@ main(int argc, char *argv[])
warn("can't set rlimit data size");
}
- if (pledge("stdio rpath wpath cpath fattr flock", NULL) == -1)
+ if (pledge("stdio rpath wpath cpath fattr flock unveil", NULL) == -1)
err(1, "pledge");
strlcpy(dbdir, _PATH_VARDB, sizeof(dbdir));
@@ -114,6 +114,20 @@ main(int argc, char *argv[])
if (argc > 1)
usage();
+
+ if (argc > 0) {
+ if (unveil(argv[0], "r") == -1)
+ err(1, "unveil");
+ } else {
+ if (unveil(_PATH_UNIX, "r") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_KSYMS, "r") == -1)
+ err(1, "unveil");
+ }
+ if (unveil(dbdir, "rwc") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath wpath cpath fattr flock", NULL) == -1)
+ err(1, "pledge");
/* If no kernel specified use _PATH_KSYMS and fall back to _PATH_UNIX */
if (argc > 0) {
Re: unveil bdftopcf
Something like this then?
If it's too much burden to keep these local patches I can drop it, no problem.
Index: bdftopcf.c
===
RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 bdftopcf.c
--- bdftopcf.c 29 Mar 2018 20:34:30 - 1.5
+++ bdftopcf.c 25 Oct 2018 07:00:50 -
@@ -39,6 +39,7 @@ from The Open Group.
#include "bdfint.h"
#include "pcf.h"
#include
+#include
#include
int
@@ -158,6 +159,38 @@ main(int argc, char *argv[])
}
argv++;
}
+
+if (input_name) {
+if (unveil(input_name, "r") == -1) {
+fprintf(stderr, "%s: could not unveil %s\n",
+program_name, input_name);
+exit(1);
+ }
+}
+if (output_name) {
+if (unveil(output_name, "rwc") == -1) {
+fprintf(stderr, "%s: could not unveil %s\n",
+program_name, output_name);
+exit(1);
+}
+if (pledge("stdio rpath wpath cpath", NULL) == -1) {
+fprintf(stderr, "%s: could not pledge", program_name);
+exit(1);
+}
+}
+if (input_name && !output_name) {
+if (pledge("stdio rpath", NULL) == -1) {
+fprintf(stderr, "%s: could not pledge", program_name);
+exit(1);
+}
+}
+if (!input_name && !output_name) {
+if (pledge("stdio", NULL) == -1) {
+fprintf(stderr, "%s: could not pledge", program_name);
+exit(1);
+}
+}
+
if (input_name) {
input = FontFileOpen(input_name);
if (!input) {
On 10:41 Wed 24 Oct , Theo de Raadt wrote:
> Matthieu Herrb wrote:
>
> > Generally, I'm not too found of pledging/unveiling random X client
> > programs. There are a lot of "hidden" features in X libraries that
> > will probably break with too strict pledges and/or unveils.
>
> Well eventually we want to see if something can be done about xterm.
> Especially if the lessons learned (I suspect some hoisting will occur)
> can be pushed back upstream, and maybe allow others to apply their
> own system call limiter mechanism. Perhaps not possible...
>
> > Also since this is OpenBSD-specific, it will be difficult to get it
> > upstreams, especially if you don't provide the autoconf goo to make
> > the code still build/work on Linux. And when not upstreaming it
> > creates more burden to merge new versions of the applications.
>
> Well, I doubt it will create too much burden, generally these unveil
> or pledge chunks are a small set of + lines, without changing other
> logic.
>
> Anyways, bdftopcf is not running near a security boundary.
>
unveil bdftopcf
Hi,
If input_name is provided we can unveil it with read permissions, if
output_name is provided we need to unveil this one with rwc. Additionally
depending on the different combinations of if these files are passed via args
or from stdin/to stdout we can also pledge accordingly to the code path. This
has been tested succefully with bdf fonts we have bundled in xenocara.
Since I have several other X apps unveiled and/or pledged could you please
comment not only with the unveil/pledge part, but also err vs fprintf/exit, the
placement of the #includes and also tabs vs spaces?
Index: bdftopcf.c
===
RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 bdftopcf.c
--- bdftopcf.c 29 Mar 2018 20:34:30 - 1.5
+++ bdftopcf.c 24 Oct 2018 10:15:41 -
@@ -38,7 +38,9 @@ from The Open Group.
#include "fntfil.h"
#include "bdfint.h"
#include "pcf.h"
+#include
#include
+#include
#include
int
@@ -158,6 +160,26 @@ main(int argc, char *argv[])
}
argv++;
}
+
+ if (input_name) {
+ if (unveil(input_name, "r") == -1)
+ err(1, "unveil");
+ }
+ if (output_name) {
+ if (unveil(output_name, "rwc") == -1)
+ err(1, "unveil");
+ if (pledge("stdio rpath wpath cpath", NULL) == -1)
+ err(1, "pledge");
+ }
+ if (input_name && !output_name) {
+ if (pledge("stdio rpath", NULL) == -1)
+ err(1, "pledge");
+ }
+ if (!input_name && !output_name) {
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+ }
+
if (input_name) {
input = FontFileOpen(input_name);
if (!input) {
Re: unveil xserver's priv proc
Hello,
semarie@ already gave positive feedback for unveiling xserver, did
anyone tested it yet and comment on it or OK?
Index: privsep.c
===
RCS file: /cvs/xenocara/xserver/os/privsep.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 privsep.c
--- privsep.c 6 Aug 2018 20:11:34 - 1.29
+++ privsep.c 24 Oct 2018 09:35:01 -
@@ -274,6 +274,10 @@ priv_init(uid_t uid, gid_t gid)
setproctitle("[priv]");
close(socks[1]);
+ for (dev = allowed_devices; dev->name != NULL; dev++) {
+ if (unveil(dev->name, "rw") == -1)
+ err(1, "unveil");
+ }
if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
err(1, "pledge");
unveil passwd
Hi,
The diff below unveils passwd with exactly the same ones used on vipw, the only
difference is that in this case _PATH_BSHELL is used to spawn an external
passwordcheck program (if defined in /etc/login.conf) instead of an EDITOR.
Tested by changing my users' passwords back and forth several times, but if you
want to test this please backup your /etc/master.passwd first otherwise it may
eat your kittens!
OK?
Index: local_passwd.c
===
RCS file: /cvs/src/usr.bin/passwd/local_passwd.c,v
retrieving revision 1.53
diff -u -p -u -r1.53 local_passwd.c
--- local_passwd.c 30 Dec 2016 23:32:14 - 1.53
+++ local_passwd.c 24 Oct 2018 09:18:44 -
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -71,6 +72,14 @@ local_passwd(char *uname, int authentica
return(1);
}
+ if (unveil(_PATH_MASTERPASSWD_LOCK, "wc") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_MASTERPASSWD, "r") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_BSHELL, "x") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_PWD_MKDB, "x") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath wpath cpath getpw tty id proc exec", NULL) ==
-1)
err(1, "pledge");
unveil spamlogd
Hi,
The only file that spamlogd needs to access after calling pledge is
PATH_SPAMD_DB, so unveil it with O_RDWR permissions.
OK?
Index: spamlogd.c
===
RCS file: /cvs/src/libexec/spamlogd/spamlogd.c,v
retrieving revision 1.27
diff -u -p -u -r1.27 spamlogd.c
--- spamlogd.c 16 Mar 2016 14:47:04 - 1.27
+++ spamlogd.c 24 Oct 2018 07:00:09 -
@@ -375,6 +375,8 @@ main(int argc, char **argv)
openlog_r("spamlogd", LOG_PID | LOG_NDELAY, LOG_DAEMON, );
}
+ if (unveil(PATH_SPAMD_DB, "rw") == -1)
+ err(1, "unveil");
if (syncsend) {
if (pledge("stdio rpath wpath inet flock", NULL) == -1)
err(1, "pledge");
unveil spamd
Hi,
When spamd runs in greylist mode the parent process (which runs greywatcher())
we know that the only files that it will ever access are PATH_SPAMD_DB in rw,
alloweddomains_file in r and that it will need to exec PATH_PFCTL so we can
unveil them with those permissions.
All other necessary files, such as certificates if used and /dev/pf, were
already opened by this time so they don't need to be unveiled. /dev/fd/*
doesn't need to be unveiled since from spamd's perspective is only a parameter
for pfctl. Furthermore, the child process loop already runs without fs access.
Comments? OK?
Index: grey.c
===
RCS file: /cvs/src/libexec/spamd/grey.c,v
retrieving revision 1.65
diff -u -p -u -r1.65 grey.c
--- grey.c 18 Oct 2017 17:31:01 - 1.65
+++ grey.c 23 Oct 2018 08:39:38 -
@@ -1078,6 +1078,18 @@ greywatcher(void)
drop_privs();
+ if (unveil(PATH_SPAMD_DB, "rw") == -1) {
+ syslog_r(LOG_ERR, , "unveil failed (%m)");
+ exit(1);
+ }
+ if (unveil(alloweddomains_file, "r") == -1) {
+ syslog_r(LOG_ERR, , "unveil failed (%m)");
+ exit(1);
+ }
+ if (unveil(PATH_PFCTL, "x") == -1) {
+ syslog_r(LOG_ERR, , "unveil failed (%m)");
+ exit(1);
+ }
if (pledge("stdio rpath wpath inet flock proc exec", NULL) == -1) {
syslog_r(LOG_ERR, , "pledge failed (%m)");
exit(1);
Re: unveil xserver's priv proc
Oops I missed the obvious kill(2) only a few lines later, silly me :\
On 13:56 Tue 16 Oct , Sebastien Marie wrote:
> about unveil: it seems fine. open_ok() functions checks if
> cmd.arg.open.path is in allowed_devices. so having it locked to only
> that seems correct.
>
>
> about pledge: "proc" isn't only used for fork(2). but also for using
> kill(2) for others pids than itself.
>
> in the main loop, the process could have to send USR1 signal to
> parent_pid. if it doesn't have "proc" it will be killed.
>
>277 if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
>278 err(1, "pledge");
>279
>280 while (1) {
>281 if (read(socks[0], , sizeof(cmd)) == 0) {
>282 exit(0);
>283 }
>284 switch (cmd.cmd) {
>285
>286 case PRIV_OPEN_DEVICE:
>287 if ((dev = open_ok(cmd.arg.open.path)) !=
> NULL) {
>288 fd = open(cmd.arg.open.path,
> dev->flags);
>289 } else {
>290 fd = -1;
>291 errno = EPERM;
>292 }
>293 send_fd(socks[0], fd);
>294 if (fd >= 0)
>295 close(fd);
>296 break;
>297 case PRIV_SIG_PARENT:
>298 if (parent_pid > 1)
>299 kill(parent_pid, SIGUSR1);
>300 break;
>301 default:
>302 errx(1, "%s: unknown command %d", __func__,
> cmd.cmd);
>303 break;
>304 }
>305 }
>
> > Index: privsep.c
> > ===
> > RCS file: /cvs/xenocara/xserver/os/privsep.c,v
> > retrieving revision 1.29
> > diff -u -p -u -r1.29 privsep.c
> > --- privsep.c 6 Aug 2018 20:11:34 - 1.29
> > +++ privsep.c 16 Oct 2018 10:51:10 -
> > @@ -274,7 +274,11 @@ priv_init(uid_t uid, gid_t gid)
> > setproctitle("[priv]");
> > close(socks[1]);
> >
> > - if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
> > + for (dev = allowed_devices; dev->name != NULL; dev++) {
> > + if (unveil(dev->name, "rw") == -1)
> > + err(1, "unveil");
> > + }
> > + if (pledge("stdio rpath wpath sendfd", NULL) == -1)
> > err(1, "pledge");
> >
> > while (1) {
> >
>
> --
> Sebastien Marie
>
unveil xserver's priv proc
Hi,
xserver's priv proc is responsible for opening devices in O_RDWR mode and send
their fds over to the parent proc. Knowing this then we already have a list of
all possible devices that might be opened in the future and we can unveil(2)
them by traversing allowed_devices and yes it's a long list, but we won't hit
the limit defined by UNVEIL_MAX_VNODES (currently set to 128). But yes, this
change might be disputable due to a limitation of vnodes available.
Additionally, by this point we already fork(2)ed so we can drop "proc" promise
from pledge(2) and I didn't run into any troubles with both these changes.
Comments on either unveil or pledge? OK?
Index: privsep.c
===
RCS file: /cvs/xenocara/xserver/os/privsep.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 privsep.c
--- privsep.c 6 Aug 2018 20:11:34 - 1.29
+++ privsep.c 16 Oct 2018 10:51:10 -
@@ -274,7 +274,11 @@ priv_init(uid_t uid, gid_t gid)
setproctitle("[priv]");
close(socks[1]);
- if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
+ for (dev = allowed_devices; dev->name != NULL; dev++) {
+ if (unveil(dev->name, "rw") == -1)
+ err(1, "unveil");
+ }
+ if (pledge("stdio rpath wpath sendfd", NULL) == -1)
err(1, "pledge");
while (1) {
Re: savecore: unveil bug
Hi Sebastien,
I had a similar diff lying around, although with if/else, and forgot
about it :\
OK mestre@ if someone wants to commit it and thank you!
On 09:03 Fri 28 Sep , Sebastien Marie wrote:
> Hi,
>
> The unveil(2) call for savecore(8) is incomplete. savecore(8) needs to
> access to the /bsd to copying it.
>
> Without it, savecore(8) abort the process, and due to karl, the original
> kernel is lost.
>
> Without the patch:
> --
> savecore: reboot after panic: pool_do_get: vmmpepl free list modified: page
> 0xff018754; item addr 0xff01875400a8; offset 0x38=0xd6adbeef
> savecore: system went down at Fri Sep 28 08:07:31 2018
> savecore: /bsd: No such file or directory
>
>
> I was able to successfully extract the dump with the patch (but /bsd
> wasn't the right kernel anymore due to reboot).
>
> Thanks.
> --
> Sebastien Marie
>
>
> Index: savecore.c
> ===
> RCS file: /cvs/src/sbin/savecore/savecore.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 savecore.c
> --- savecore.c24 Sep 2018 21:26:38 - 1.58
> +++ savecore.c28 Sep 2018 06:47:58 -
> @@ -175,6 +175,10 @@ main(int argc, char *argv[])
> syslog(LOG_ERR, "unveil: %m");
> exit(1);
> }
> + if (unveil(kernel ? kernel : _PATH_UNIX, "r") == -1) {
> + syslog(LOG_ERR, "unveil: %m");
> + exit(1);
> + }
> if (pledge("stdio rpath wpath cpath", NULL) == -1) {
> syslog(LOG_ERR, "pledge: %m");
> exit(1);
>
Re: unveil(2) tcpdump(8)
Hi tech@
I've commited this, please test it as much as possible by applying the diff
right now or just wait for the next snapshot.
Let it run for a long time and let me know of any problems as soon as you get
any. For this your ktrace, pcap and coredump files will be VERY important for
further analysis of the issue. As noted in sysctl(8)'s man in order to generate
the dumps in a safe place please do the following:
# mkdir -m 700 /var/crash/tcpdump
# sysctl kern.nosuidcoredump=3
If someone has weird networks then even better for testing it since mine is
pretty much vanilla and might not have been enough to see flaws with the diff.
I can't stress this enough, please test it and report it ASAP either here or in
private.
On 18:08 Wed 26 Sep , Ricardo Mestre wrote:
> I'm not too worried about the crash, I was playing with removing rpath
> from pledge in the case that /etc/ethers wasn't need which sure enough
> it is. pledge was most likely the reason that made it crash.
>
> But brynet@ pointed out that while he was working on tcpdump last year
> he saw that it also needs to open /etc/rpc, and yes it calls
> getrpcbynumber(3) so an updated diff is below.
>
> Index: privsep.c
> ===
> RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
> retrieving revision 1.48
> diff -u -p -u -r1.48 privsep.c
> --- privsep.c 8 Aug 2018 22:57:12 - 1.48
> +++ privsep.c 26 Sep 2018 17:04:25 -
> @@ -207,7 +207,7 @@ __dead void
> priv_exec(int argc, char *argv[])
> {
> int bpfd = -1;
> - int i, sock, cmd, nflag = 0, Pflag = 0;
> + int i, sock, cmd, nflag = 0, oflag = 0, Pflag = 0;
> char *cmdbuf, *infile = NULL;
> char *RFileName = NULL;
> char *WFileName = NULL;
> @@ -229,6 +229,10 @@ priv_exec(int argc, char *argv[])
> nflag++;
> break;
>
> + case 'o':
> + oflag = 1;
> + break;
> +
> case 'r':
> RFileName = optarg;
> break;
> @@ -305,6 +309,14 @@ priv_exec(int argc, char *argv[])
> test_state(cmd, STATE_RUN);
> impl_init_done(sock, );
>
> + if (oflag) {
> + if (unveil("/etc/pf.os", "r") == -1)
> + err(1, "unveil");
> + }
> + if (unveil("/etc/ethers", "r") == -1)
> + err(1, "unveil");
> + if (unveil("/etc/rpc", "r") == -1)
> + err(1, "unveil");
> if (pledge("stdio rpath inet dns recvfd bpf", NULL) ==
> -1)
> err(1, "pledge");
>
>
> On 08:58 Wed 26 Sep , Theo de Raadt wrote:
> > Ricardo Mestre wrote:
> >
> > > Hi,
> > >
> > > This has been shown internally for some time, but deraadt@ asked me to
> > > show it
> > > to a bigger audience now so here it is!
> > >
> > > If we want OS fingerprinting by using -o flag then we can unveil
> > > /etc/pf.os in
> > > read mode, nevertheless in order to do this we need to inform the privsep
> > > proc
> > > that we are using -o so I added it to priv_exec().
> >
> > looks right
> >
> > > The other file needed to be unveiled is /etc/ethers in read mode, which I
> > > tried
> > > to make it conditional but after several successful tests I bumped into a
> > > packet which made tcpdump crash after some time. Unfortunately I don't
> > > have the
> > > core nor the pcap files to investigate what happen so for now the unveil
> > > of
> > > this file will be kept unconditional regardless of the flags or expression
> > > used.
> >
> > It is very likely that a protocol parser will find a MAC address nested
> > inside, and try to parse it via /etc/ethers. So makes sense, and there
> > is little risk in exposing ethers
> >
> > There is far more risk that some other file is required, and will fail to
> > open silently, and tcpdump willbehave differently
> >
> > You know well: unveil requires a different audit approach than pledge
> >
> > But it is rare for a missing file via unveil to crash a program, so
> > something
> > is fishy. A crash isn't good, perhaps try to reproduce and collect a
> > corefile
> > using the sysctl kern.nosuidcoredump=3 method.
> >
>
unveil(2) vipw(8)
Hi,
I've just commited this to unveil vipw, all the tests I've done were
successful and didn't bump into any problems, nevertheless if you get any
troubles because of this, like getting locked out of the machine, please let me
know ASAP!
Index: vipw.c
===
RCS file: /cvs/src/usr.sbin/vipw/vipw.c,v
retrieving revision 1.21
diff -u -p -u -r1.21 vipw.c
--- vipw.c 12 Jul 2017 23:10:28 - 1.21
+++ vipw.c 25 Sep 2018 14:45:57 -
@@ -34,6 +34,7 @@
#include
#include
+#include
#include
#include
#include
@@ -62,6 +63,14 @@ main(int argc, char *argv[])
if (argc != 0)
usage();
+ if (unveil(_PATH_MASTERPASSWD_LOCK, "wc") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_MASTERPASSWD, "r") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_BSHELL, "x") == -1)
+ err(1, "unveil");
+ if (unveil(_PATH_PWD_MKDB, "x") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath wpath cpath fattr proc exec", NULL) == -1)
err(1, "pledge");
Re: unveil(2) tcpdump(8)
I'm not too worried about the crash, I was playing with removing rpath
from pledge in the case that /etc/ethers wasn't need which sure enough
it is. pledge was most likely the reason that made it crash.
But brynet@ pointed out that while he was working on tcpdump last year
he saw that it also needs to open /etc/rpc, and yes it calls
getrpcbynumber(3) so an updated diff is below.
Index: privsep.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.48
diff -u -p -u -r1.48 privsep.c
--- privsep.c 8 Aug 2018 22:57:12 - 1.48
+++ privsep.c 26 Sep 2018 17:04:25 -
@@ -207,7 +207,7 @@ __dead void
priv_exec(int argc, char *argv[])
{
int bpfd = -1;
- int i, sock, cmd, nflag = 0, Pflag = 0;
+ int i, sock, cmd, nflag = 0, oflag = 0, Pflag = 0;
char *cmdbuf, *infile = NULL;
char *RFileName = NULL;
char *WFileName = NULL;
@@ -229,6 +229,10 @@ priv_exec(int argc, char *argv[])
nflag++;
break;
+ case 'o':
+ oflag = 1;
+ break;
+
case 'r':
RFileName = optarg;
break;
@@ -305,6 +309,14 @@ priv_exec(int argc, char *argv[])
test_state(cmd, STATE_RUN);
impl_init_done(sock, );
+ if (oflag) {
+ if (unveil("/etc/pf.os", "r") == -1)
+ err(1, "unveil");
+ }
+ if (unveil("/etc/ethers", "r") == -1)
+ err(1, "unveil");
+ if (unveil("/etc/rpc", "r") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath inet dns recvfd bpf", NULL) ==
-1)
err(1, "pledge");
On 08:58 Wed 26 Sep , Theo de Raadt wrote:
> Ricardo Mestre wrote:
>
> > Hi,
> >
> > This has been shown internally for some time, but deraadt@ asked me to show
> > it
> > to a bigger audience now so here it is!
> >
> > If we want OS fingerprinting by using -o flag then we can unveil /etc/pf.os
> > in
> > read mode, nevertheless in order to do this we need to inform the privsep
> > proc
> > that we are using -o so I added it to priv_exec().
>
> looks right
>
> > The other file needed to be unveiled is /etc/ethers in read mode, which I
> > tried
> > to make it conditional but after several successful tests I bumped into a
> > packet which made tcpdump crash after some time. Unfortunately I don't have
> > the
> > core nor the pcap files to investigate what happen so for now the unveil of
> > this file will be kept unconditional regardless of the flags or expression
> > used.
>
> It is very likely that a protocol parser will find a MAC address nested
> inside, and try to parse it via /etc/ethers. So makes sense, and there
> is little risk in exposing ethers
>
> There is far more risk that some other file is required, and will fail to
> open silently, and tcpdump willbehave differently
>
> You know well: unveil requires a different audit approach than pledge
>
> But it is rare for a missing file via unveil to crash a program, so something
> is fishy. A crash isn't good, perhaps try to reproduce and collect a corefile
> using the sysctl kern.nosuidcoredump=3 method.
>
fix usermod -l
Hi,
While doing something else here I noticed that changing the login name of an
existing user with usermod -l the program gets a segmentation fault.
This looks like it was introduced when millert@ changed pwcache and the fix is
a matter of changing getpwnam(3) to uid_from_user(3).
OK?
Index: user.c
===
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.121
diff -u -p -u -r1.121 user.c
--- user.c 13 Sep 2018 15:23:32 - 1.121
+++ user.c 26 Sep 2018 09:34:07 -
@@ -1369,6 +1369,7 @@ moduser(char *login_name, char *newlogin
int ptmpfd;
int rval;
int i;
+ uid_t uid;
if (!valid_login(newlogin)) {
errx(EXIT_FAILURE, "`%s' is not a valid login name",
login_name);
@@ -1427,7 +1428,8 @@ moduser(char *login_name, char *newlogin
if (up != NULL) {
if (up->u_flags & F_USERNAME) {
/* if changing name, check new name isn't already in
use */
- if (strcmp(login_name, newlogin) != 0 &&
getpwnam(newlogin) != NULL) {
+ if (strcmp(login_name, newlogin) != 0 &&
+ uid_from_user(newlogin, ) != -1) {
close(ptmpfd);
pw_abort();
errx(EXIT_FAILURE, "already a `%s' user",
newlogin);
unveil(2) tcpdump(8)
Hi,
This has been shown internally for some time, but deraadt@ asked me to show it
to a bigger audience now so here it is!
If we want OS fingerprinting by using -o flag then we can unveil /etc/pf.os in
read mode, nevertheless in order to do this we need to inform the privsep proc
that we are using -o so I added it to priv_exec().
The other file needed to be unveiled is /etc/ethers in read mode, which I tried
to make it conditional but after several successful tests I bumped into a
packet which made tcpdump crash after some time. Unfortunately I don't have the
core nor the pcap files to investigate what happen so for now the unveil of
this file will be kept unconditional regardless of the flags or expression
used.
Could you please test tcpdump on your network with this patch? You should test
several different flags and the different combinations between them just as I
did, and please also try different expressions then report back if you had any
issues or not and if this can go in.
Index: privsep.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/privsep.c,v
retrieving revision 1.48
diff -u -p -u -r1.48 privsep.c
--- privsep.c 8 Aug 2018 22:57:12 - 1.48
+++ privsep.c 26 Sep 2018 06:57:20 -
@@ -207,7 +207,7 @@ __dead void
priv_exec(int argc, char *argv[])
{
int bpfd = -1;
- int i, sock, cmd, nflag = 0, Pflag = 0;
+ int i, sock, cmd, nflag = 0, oflag = 0, Pflag = 0;
char *cmdbuf, *infile = NULL;
char *RFileName = NULL;
char *WFileName = NULL;
@@ -229,6 +229,10 @@ priv_exec(int argc, char *argv[])
nflag++;
break;
+ case 'o':
+ oflag = 1;
+ break;
+
case 'r':
RFileName = optarg;
break;
@@ -305,6 +309,12 @@ priv_exec(int argc, char *argv[])
test_state(cmd, STATE_RUN);
impl_init_done(sock, );
+ if (oflag) {
+ if (unveil("/etc/pf.os", "r") == -1)
+ err(1, "unveil");
+ }
+ if (unveil("/etc/ethers", "r") == -1)
+ err(1, "unveil");
if (pledge("stdio rpath inet dns recvfd bpf", NULL) ==
-1)
err(1, "pledge");
Re: yacc + unveil
This is an example of better to start at just hoisting the code that
opens the many fds and put them all inside open_files(). After that it's
just a matter of calling pledge("stdio") and we are done.
Of course that after this is done we can still make a list of all the files
we need to open and unveil them, but not the way it's done here.
Once I get back home from $DAYJOB I'll try to have a look at this.
On 08:04 Tue 25 Sep , Theo de Raadt wrote:
> Theo de Raadt wrote:
>
> > Michael Mikonos wrote:
> >
> > > On Mon, Sep 24, 2018 at 10:53:47PM -0600, Theo de Raadt wrote:
> > > > Ugh. A diff which doens't check error returns. Averting my gaze
> > > > is similar to "no way". Hope you have another quarter, because you
> > > > need to try again
> > >
> > > Oops... new coin inserted. I decided to create a fatal_perror()
> > > function which behaves like perror(). yacc's error.c didn't seem
> > > to have anything like that. Now if either pledge() or unveil()
> > > fails we see the appropriate error string instead of always seeing
> > > "invalid arguments" for pledge().
> >
> > I don't understand parts of the diff.
> >
> > > Index: defs.h
> > > ===
> > > RCS file: /cvs/src/usr.bin/yacc/defs.h,v
> > > retrieving revision 1.18
> > > diff -u -p -u -r1.18 defs.h
> > > --- defs.h2 Dec 2014 15:56:22 - 1.18
> > > +++ defs.h25 Sep 2018 05:34:27 -
> > > @@ -307,6 +307,7 @@ extern void closure(short *, int);
> > > extern void finalize_closure(void);
> > >
> > > extern __dead void fatal(char *);
> > > +extern __dead void fatal_perror(char *);
> > >
> > > extern void reflexive_transitive_closure(unsigned *, int);
> > > extern __dead void done(int);
> > > Index: error.c
> > > ===
> > > RCS file: /cvs/src/usr.bin/yacc/error.c,v
> > > retrieving revision 1.14
> > > diff -u -p -u -r1.14 error.c
> > > --- error.c 8 Mar 2014 01:05:39 - 1.14
> > > +++ error.c 25 Sep 2018 05:34:27 -
> > > @@ -35,6 +35,8 @@
> > >
> > > /* routines for printing error messages */
> > >
> > > +#include
> > > +
> > > #include "defs.h"
> > >
> > >
> > > @@ -45,6 +47,12 @@ fatal(char *msg)
> > > done(2);
> > > }
> > >
> > > +void
> > > +fatal_perror(char *msg)
> > > +{
> > > + fprintf(stderr, "%s: %s\n", msg, strerror(errno));
> > > + done(2);
> > > +}
> > >
> > > void
> > > no_space(void)
> > > Index: main.c
> > > ===
> > > RCS file: /cvs/src/usr.bin/yacc/main.c,v
> > > retrieving revision 1.29
> > > diff -u -p -u -r1.29 main.c
> > > --- main.c25 May 2017 20:11:03 - 1.29
> > > +++ main.c25 Sep 2018 05:34:27 -
> > > @@ -305,10 +305,14 @@ open_files(void)
> > > create_file_names();
> > >
> > > if (input_file == 0) {
> > > + if (unveil(input_file_name, "r") == -1)
> > > + fatal_perror("unveil");
> > > input_file = fopen(input_file_name, "r");
> >
> > At this point, all files are allowed.
> > So you restrict it to one.
> > Then you open it.
> > You won't open it again.
> > Why does it remain on the permitted open list?
> > Why not just open it, and not have it on the permitted open list?
> >
> > Meanwhile, unveil hasn't been blocked. So if someone finds a bug
> > at this point which gives them control, they can continue calling
> > unveil, and get full filesystem access back.
>
>
> As another way of explaining, the logic you created goes like this
>
>allow only this file
>open it
>do some more calculation
>oh wait, allow this file also!
>open it
>do some more calculation
>OH WAIT, here is another file to open...
>
Re: unveil(2) getent(1)
I actually prefer to see it go away since it doesn't protect us much and the
real meat is actually on the pledge(2) inside the loop. Nevertheless this still
should on a separate commit.
OK?
Index: getent.c
===
RCS file: /cvs/src/usr.bin/getent/getent.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 getent.c
--- getent.c1 Feb 2016 19:57:28 - 1.14
+++ getent.c24 Sep 2018 19:52:35 -
@@ -95,9 +95,6 @@ main(int argc, char *argv[])
{
struct getentdb *curdb;
- if (pledge("stdio dns rpath getpw", NULL) == -1)
- err(1, "pledge");
-
if (argc < 2)
usage();
for (curdb = databases; curdb->name != NULL; curdb++) {
On 20:41 Mon 24 Sep , Klemens Nanni wrote:
> On Mon, Sep 24, 2018 at 10:49:42AM +0100, Ricardo Mestre wrote:
> > Comments? OK? The initial pledge(2) is so short lived that I was tempted to
> > remove it, but I'm open to suggestions :)
> Is there any compelling reason to keep the initial superset pledge?
>
> Without it, the only code paths without pledge/unveil are either
> main()->usage()->exit() or main()->{all failing strcmp() loop}->return.
Re: unveil(2) getent(1)
Oh boy, I took a brief look into Makefile.yp(8), let's forget about this since ALL of them can have YP maps (except for /etc/shells). On 06:20 Mon 24 Sep , Todd C. Miller wrote: > On Mon, 24 Sep 2018 12:25:51 +0100, Ricardo Mestre wrote: > > > Wouldn't this be already contemplated by pledge(getpw) on both group and > > passwd databases? I'm not touching those since they already whitelist > > all necessary files through pledge(2). > > I think you are correct, it appears the getpw pledge should unveil > what is needed for YP. Note that the ethers database can also be > stored YP. > > - todd >
Re: unveil(2) getent(1)
Wouldn't this be already contemplated by pledge(getpw) on both group and passwd databases? I'm not touching those since they already whitelist all necessary files through pledge(2). On 05:11 Mon 24 Sep , Todd C. Miller wrote: > I doubt this will work on systems using YP or ypldap. > > - todd
unveil(2) getent(1)
Hi,
Since the databases that require rpath only need to access one file we can add
one attribute to the struct getentdb to identify which of those DBs we need
unveiled. For group/hosts/passwd the files are already whitelisted through
pledge(2) so I set them as NULL.
With that information we can now check if the unveil attribute is not NULL and
then unveil(2) the database file as needed. I tested all databases by
enumerating all entries (for the ones that support it) and by selecting
specific entries without any apparent issues.
Comments? OK? The initial pledge(2) is so short lived that I was tempted to
remove it, but I'm open to suggestions :)
Index: getent.c
===
RCS file: /cvs/src/usr.bin/getent/getent.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 getent.c
--- getent.c1 Feb 2016 19:57:28 - 1.14
+++ getent.c24 Sep 2018 09:36:43 -
@@ -77,15 +77,16 @@ static struct getentdb {
const char *name;
int (*fn)(int, char *[]);
const char *pledge;
+ const char *unveil;
} databases[] = {
- { "ethers", ethers, "stdio rpath" },
- { "group",group, "stdio getpw" },
- { "hosts",hosts, "stdio dns" },
- { "passwd", passwd, "stdio getpw" },
- { "protocols",protocols, "stdio rpath" },
- { "rpc", rpc,"stdio rpath" },
- { "services", services, "stdio rpath" },
- { "shells", shells, "stdio rpath" },
+ { "ethers", ethers, "stdio rpath", "/etc/ethers"
},
+ { "group",group, "stdio getpw", NULL},
+ { "hosts",hosts, "stdio dns",NULL},
+ { "passwd", passwd, "stdio getpw", NULL},
+ { "protocols",protocols, "stdio rpath",
"/etc/protocols"},
+ { "rpc", rpc,"stdio rpath", "/etc/rpc"
},
+ { "services", services, "stdio rpath", "/etc/services"
},
+ { "shells", shells, "stdio rpath", "/etc/shells"
},
{ NULL, NULL, },
};
@@ -95,13 +96,17 @@ main(int argc, char *argv[])
{
struct getentdb *curdb;
- if (pledge("stdio dns rpath getpw", NULL) == -1)
+ if (pledge("stdio rpath dns getpw unveil", NULL) == -1)
err(1, "pledge");
if (argc < 2)
usage();
for (curdb = databases; curdb->name != NULL; curdb++) {
if (strcmp(curdb->name, argv[1]) == 0) {
+ if (curdb->unveil != NULL) {
+ if (unveil(curdb->unveil, "r") == -1)
+ err(1, "unveil");
+ }
if (pledge(curdb->pledge, NULL) == -1)
err(1, "pledge");
Re: uninitialized variable in if_mue.c
Ouch, of course! Still not enough caffeine in the system!
Unfortunately I don't have such a card to test it, but this is the way it's done
everywhere else, the writes are always done unconditionally and we just need to
ensure that the hash table is initialized to 0 so I prefer to keep the
consistency.
Index: if_mue.c
===
RCS file: /cvs/src/sys/dev/usb/if_mue.c,v
retrieving revision 1.4
diff -u -p -u -r1.4 if_mue.c
--- if_mue.c15 Aug 2018 07:13:51 - 1.4
+++ if_mue.c18 Sep 2018 07:27:27 -
@@ -1016,6 +1016,7 @@ mue_iff(struct mue_softc *sc)
rxfilt = mue_csr_read(sc, reg);
rxfilt &= ~(MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH |
MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST);
+ memset(hashtbl, 0, sizeof(hashtbl));
ifp->if_flags &= ~IFF_ALLMULTI;
/* Always accept broadcast frames. */
@@ -1028,9 +1029,6 @@ mue_iff(struct mue_softc *sc)
rxfilt |= MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST;
} else {
rxfilt |= MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH;
-
- /* Clear hash table. */
- memset(hashtbl, 0, sizeof(hashtbl));
/* Now program new ones. */
ETHER_FIRST_MULTI(step, ac, enm);
On 09:06 Tue 18 Sep , Claudio Jeker wrote:
> On Tue, Sep 18, 2018 at 07:55:43AM +0100, Ricardo Mestre wrote:
> > Hi,
> >
> > In the case that a mue(4) device is put in promiscuous mode then hashtbl
> > will
> > be used uninitialized a little bit down the road so set it 0 like it's done
> > in
> > a lot of other devices. Coverity ID 1473316.
> >
> > OK?
>
> Please also remove the later memset(). There is no need to do it twice.
> It would also be an option to not issue mue_dataport_write(MUE_DP_SEL_VHF)
> if the hash is not used (e.g. moving that up into the else block.
> Which ever option taken it needs to be tested on real hardware.
>
> --
> :wq Claudio
uninitialized variable in if_mue.c
Hi, In the case that a mue(4) device is put in promiscuous mode then hashtbl will be used uninitialized a little bit down the road so set it 0 like it's done in a lot of other devices. Coverity ID 1473316. OK? Index: if_mue.c === RCS file: /cvs/src/sys/dev/usb/if_mue.c,v retrieving revision 1.4 diff -u -p -u -r1.4 if_mue.c --- if_mue.c15 Aug 2018 07:13:51 - 1.4 +++ if_mue.c18 Sep 2018 06:47:54 - @@ -1016,6 +1016,7 @@ mue_iff(struct mue_softc *sc) rxfilt = mue_csr_read(sc, reg); rxfilt &= ~(MUE_RFE_CTL_PERFECT | MUE_RFE_CTL_MULTICAST_HASH | MUE_RFE_CTL_UNICAST | MUE_RFE_CTL_MULTICAST); + memset(hashtbl, 0x00, sizeof(hashtbl)); ifp->if_flags &= ~IFF_ALLMULTI; /* Always accept broadcast frames. */
Re: unveil for audioctl
And of course I missed disabling unveil(2) just right the first call... Index: audioctl.c === RCS file: /cvs/src/usr.bin/audioctl/audioctl.c,v retrieving revision 1.35 diff -u -p -u -r1.35 audioctl.c --- audioctl.c 31 May 2017 04:18:58 - 1.35 +++ audioctl.c 17 Sep 2018 12:26:30 - @@ -217,6 +217,11 @@ main(int argc, char **argv) argc -= optind; argv += optind; + if (unveil(path, "rw") == -1) + err(1, "unveil"); + if (unveil(NULL, NULL) == -1) + err(1, "unveil"); + fd = open(path, O_RDWR); if (fd < 0) err(1, "%s", path); On 13:22 Mon 17 Sep , Ricardo Mestre wrote: > Hi, > > This adds unveil(2) to audioctl(1) which only needs rw to the access control > device, which by default is /dev/audioctl0, but can be manipulated via args. > > OK? >
