Re: dhcpleased(8): ignore servers / parts of lease
On Sun, Aug 08, 2021 at 12:37:54PM +0200, Florian Obser wrote:
> This implements ignoring of nameservers and / or routes in leases as
> well as completely ignoring servers (you cannot block rogue DHCP servers
> in pf because bpf sees packets before pf).
>
> Various people voiced the need for these features.
> Tests, OKs?
>
> diff --git dhcpleased.c dhcpleased.c
> index 36a4a21c21a..e005d7de9ae 100644
> --- dhcpleased.c
> +++ dhcpleased.c
> @@ -569,6 +569,20 @@ main_dispatch_engine(int fd, short event, void *bula)
> sizeof(imsg_interface.if_index));
> break;
> }
> + case IMSG_WITHDRAW_ROUTES: {
> + struct imsg_configure_interface imsg_interface;
> + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
> + fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
> + "length: %lu", __func__,
> + IMSG_DATA_SIZE(imsg));
> + memcpy(_interface, imsg.data,
> + sizeof(imsg_interface));
> + if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
> + fatalx("%s: too many routes in imsg", __func__);
> + if (imsg_interface.routes_len > 0)
> + configure_routes(RTM_DELETE, _interface);
> + break;
> + }
> case IMSG_PROPOSE_RDNS: {
> struct imsg_propose_rdns rdns;
> if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
> diff --git dhcpleased.conf.5 dhcpleased.conf.5
> index 9e6846f899e..b856113bac1 100644
> --- dhcpleased.conf.5
> +++ dhcpleased.conf.5
> @@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults:
> .Ic interface
> options are as follows:
> .Bl -tag -width Ds
> +.It Ic ignore dns
> +Ignore nameservers from leases on this interface.
> +The default is to not ignore nameservers.
> +.It Ic ignore routes
> +Ignore routes from leases on this interface.
> +The default is to not ignore routes.
> +.It Ic ignore Ar server-ip
> +Ignore leases from
> +.Ar server-ip .
> +This option can be listed multiple times.
> +The default is to not ignore servers.
> .It Ic send client id Ar client-id
> Send the dhcp client identifier option with a value of
> .Ar client-id .
> diff --git dhcpleased.h dhcpleased.h
> index 7f6ec87ad19..1d20b77dbd3 100644
> --- dhcpleased.h
> +++ dhcpleased.h
> @@ -151,6 +151,12 @@
> #define DHCPRELEASE 7
> #define DHCPINFORM 8
>
> +/* Ignore parts of DHCP lease */
> +#define IGN_ROUTES 1
> +#define IGN_DNS 2
> +
> +#define MAX_SERVERS 16 /* max servers that can be ignores per
> if */
Typo in comment: ignored (not ignores)
Should there be a mention of a limitation in the man page where
it states the option can be listed multiple times?
--patrick
> +
> #define IMSG_DATA_SIZE(imsg)((imsg).hdr.len - IMSG_HEADER_SIZE)
> #define DHCP_SNAME_LEN 64
> #define DHCP_FILE_LEN 128
> @@ -216,6 +222,7 @@ enum imsg_type {
> IMSG_DECONFIGURE_INTERFACE,
> IMSG_PROPOSE_RDNS,
> IMSG_WITHDRAW_RDNS,
> + IMSG_WITHDRAW_ROUTES,
> IMSG_REPROPOSE_RDNS,
> IMSG_REQUEST_REBOOT,
> };
> @@ -246,6 +253,9 @@ struct iface_conf {
> int vc_id_len;
> uint8_t *c_id;
> int c_id_len;
> + int ignore;
> + struct in_addr ignore_servers[MAX_SERVERS];
> + int ignore_servers_len;
> };
>
> struct dhcpleased_conf {
> @@ -304,6 +314,8 @@ const char*sin_to_str(struct sockaddr_in *);
>
> /* frontend.c */
> struct iface_conf*find_iface_conf(struct iface_conf_head *, char *);
> +int *changed_ifaces(struct dhcpleased_conf *, struct
> + dhcpleased_conf *);
>
> /* printconf.c */
> void print_config(struct dhcpleased_conf *);
> diff --git engine.c engine.c
> index 076a57e9ba6..67693c358ee 100644
> --- engine.c
> +++ engine.c
> @@ -139,6 +139,7 @@ void
> send_configure_interface(struct dhcpleased_iface *);
> void send_rdns_proposal(struct dhcpleased_iface *);
> void send_deconfigure_interface(struct dhcpleased_iface *);
> void send_rdns_withdraw(struct dhcpleased_iface *);
> +void send_routes_withdraw(struct dhcpleased_iface *);
> void parse_lease(struct dhcpleased_iface *,
>struct imsg_ifinfo *);
> int engine_imsg_compose_main(int, pid_t, void *, uint16_t);
> @@ -506,13 +507,37 @@ engine_dispatch_main(int fd, short event, void *bula)
> IMSG_DATA_SIZE(imsg));
>
Re: dhcpleased(8): ignore servers / parts of lease
On Sun 08/08/2021 12:37, Florian Obser wrote: > This implements ignoring of nameservers and / or routes in leases as > well as completely ignoring servers (you cannot block rogue DHCP servers > in pf because bpf sees packets before pf). > > Various people voiced the need for these features. > Tests, OKs? Tested the 3 ignore options, all work as advertised. It would be really nice to see this committed.
Re: Fix unsafe snmpd defaults
On 2021/08/08 10:05, Martijn van Duren wrote: > > +++ etc/examples/snmpd.conf 7 Aug 2021 21:45:44 - > > @@ -1,24 +1,26 @@ > > # $OpenBSD: snmpd.conf,v 1.1 2014/07/11 21:20:10 deraadt Exp $ > > > > -listen_addr="127.0.0.1" > > +# Default is to listen to all addresses for SNMPv3 only; "listen on" > > +# can be used multiple times. See snmpd.conf(5) for more options. > > +#listen on 0.0.0.0 snmpv2c # All IPv4 addresses with SNMPv2c > > +#listen on :: snmpv2c snmpv3 # All IPv6 addresses, both v2c + v3 > > +#listen on 127.0.0.1 # IPv4 localhost only, v3 > > This is probably is a bad example. > Reading it like this: you're correct that we listen on all interfaces > by default, but that's not listed in snmpd.conf(5). So that should > probably be fixed (including mentioning that setting one "listen on" > disables the all interfaces default). Let's handle that separately. (it would be convenient to support "any" to mean any v4+v6 as well). > Second, your examples enable snmpv2c on all interfaces, while you > enable an implicit snmpv3 on 127.0.0.1. This should probably be the I wasn't intending that they should all be uncommented at once, just showing some common options. And actually it seems snmpd doesn't allow listening to 0.0.0.0 as well as a specific v4 address (and similarly for :: and v6) so while it's a convenient idea to allow v2c on localhost for quick testing while using v3 for external traffic, it doesn't actually work. > other way around, or replace 127.0.0.1 with something like > "listen on 192.168.0.2 snmpv2c" to map with source-address by trap > receiver. And an additional > "listen on 0.0.0.0 > listen on ::" > to make it clear that snmpv2c should only be enabled on internal > networks, but snmpv3 is less of a problem. For users who haven't got round to figuring out SNMPv3 yet, they'll need to know how to listen with v2c, so I think that's still going to be quite a common use case. Hence including that specifically. > > -# Specify a number of trap receivers > > -#trap receiver nms.localdomain.local > > +# Enable SNMPv3 USM with authentication, encryption and two defined users > > This sentence feels wrong, it is enabled by default. > Also, since we only enable SNMPv3 by default, maybe also make it > clear that at least one user should be created? Something like: > # SNMPv3 USM users. At least one user should be created to use SNMPv3. > > +#seclevel enc > > This is the default and if we would ever change this default back (I > can't imagine we ever will, but who knows) it still wouldn't break. > So maybe just leave seclevel out of the example, or show how to use > it for e.g. authNoPriv. Makes sense. Let's leave it out for simplicity. > > +#user "user1" auth hmac-sha1 authkey "password123" enc aes enckey > > "321drowssap" > > +#user "user2" auth hmac-sha256 authkey "password456" enc aes enckey > > "654drowssap" > > + > > +# Specify one or more trap receivers with optional parameters > > +#trap receiver nms.localdomain.local community PAV9kpE02gDPvAi > > source-address 192.0.2.1 > > At this time (with future additions in mind) I think that oid would be > more useful than source-address. > One example I can think of that could be possible if I manage to get > agentx back up and running and we add agentx support to vmd (I have a > working PoC). Mischa could send traps to individual users for > OpenBSD.amsterdam. Just a random example that's top of my mind, not > claiming any realism. I'll skip source-address in the trap receiver example then. When trap does something more than it does now we could add oid in if it's going to be useful to more than just niche users. (fwiw the traps that I'd find most useful internal to snmpd are interface up/down; and with external hooks traps for bgp peers dropping out would be pretty great). > > # Adjust the local system information > > #system contact "Charlie Root (r...@myhost.example.com)" > > #system description "Powered by OpenBSD" > > #system location "Rack A1-24, Room 13" > > -system services 74 > > > > -# Provide static user-defined SNMP OIDs > > -oid 1.3.6.1.4.1.30155.42.3.1 name testStringValue read-only string "Test" > > -oid 1.3.6.1.4.1.30155.42.3.4 name testIntValue read-write integer 1 > > I fully agree that these should go. This whole oid keyword is just > clutch (limited data types, assuming an oid should be turned into a > scalar by appending a zero) > > - > > -# Enable SNMPv3 USM with authentication, encryption and two defined users > > -#seclevel enc > > -#user "user1" authkey "password123" enc aes enckey "321drowssap" > > -#user "user2" authkey "password456" enckey "654drowssap" > > +# Required by some management software > > +#system services 74 > > Maybe you can explain how admins can change the value to something > correct for their environment? :-) > Or maybe we should add something like > system services physical|datalink|internet|endtoend|applications > to parse.y and let snmpd(8) create the
Re: libedit: read__fixio cleanup
On Sun, 2021-08-08 at 13:42 +0200, Ingo Schwarze wrote:
> Hi,
>
> deraadt@ recently reported to me that the editline(3) library, while
> line editing is active - for example inside el_gets(3) - ignores
> the first SIGINT it receives, for example the the first Ctrl-C the
> user presses. I consider that a bug in the editline(3) library.
> Some programs, for example our old ftp(1) implementation, work
> around the bug in a horrible way by using setjmp(3)/longjmp(3).
>
> The root cause of the problem is in libedit/read.c, in the interaction
> of the functions read_char() and read__fixio(). Before fixing the bug
> can reasonably be considered, the function read__fixio() direly needs
> cleanup. As it stands, it is utterly unreadable.
>
> So here is a patch to make it clear what the function does, with
> no functional change intended yet (-37 +5 LOC).
>
> There will be one or more follow-up patches. If you want to receive
> them, please reply to me, i won't necessarily send them all to
> tech@.
>
> I see some value in avoiding gratuitious divergence from NetBSD,
> but getting rid of this horrible mess is not gratuitious.
>
> Rationale:
> * Do not mark an argument as unused that is actually used.
> * errno cannot possibly be -1. Even is it were, treating it as
> EAGAIN makes no sense, treating it as the most severe error
> imaginable makes more sense to me.
> * We define EWOULDBLOCK to be the same as EAGAIN, so no need
> to handle it separately.
> * No need to #define TRY_AGAIN to use it just once.
> * Don't do the same thing twice. We do support the FIONBIO ioctl(2),
> so the the indirection using the F_GETFL fcntl(2) can be deleted.
> * No need to play confusing games with the "e" variable.
> Just return -1 for error or 0 for success in a straightforward
> manner.
>
> OK?
> Ingo
>
> P.S.
> I also considered whether this FIONBIO dance should better be done
> at the initialization stage rather than after EAGAIN already happened.
> But maybe not. This is a library. The application program might
> set the fd to non-blocking mode at any time and then call el_gets(3)
> again, in which case the library needs to restore blocking I/O to
> do its work.
>
Hopefully without getting dragged too much into this one:
Your reasoning and diff look fine to me.
OK martijn@
>
> Index: read.c
> ===
> RCS file: /cvs/src/lib/libedit/read.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 read.c
> --- read.c 25 May 2016 09:36:21 - 1.44
> +++ read.c 8 Aug 2021 10:30:06 -
> @@ -39,9 +39,10 @@
> * read.c: Clean this junk up! This is horrible code.
> * Terminal read functions
> */
> +#include
> +
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -134,55 +135,16 @@ el_read_getfn(struct el_read_t *el_read)
> /* read__fixio():
> * Try to recover from a read error
> */
> -/* ARGSUSED */
> static int
> -read__fixio(int fd __attribute__((__unused__)), int e)
> +read__fixio(int fd, int e)
> {
> + int zero = 0;
>
> switch (e) {
> - case -1:/* Make sure that the code is reachable */
> -
> -#ifdef EWOULDBLOCK
> - case EWOULDBLOCK:
> -#ifndef TRY_AGAIN
> -#define TRY_AGAIN
> -#endif
> -#endif /* EWOULDBLOCK */
> -
> -#if defined(POSIX) && defined(EAGAIN)
> -#if defined(EWOULDBLOCK) && EWOULDBLOCK != EAGAIN
> case EAGAIN:
> -#ifndef TRY_AGAIN
> -#define TRY_AGAIN
> -#endif
> -#endif /* EWOULDBLOCK && EWOULDBLOCK != EAGAIN */
> -#endif /* POSIX && EAGAIN */
> -
> - e = 0;
> -#ifdef TRY_AGAIN
> -#if defined(F_SETFL) && defined(O_NDELAY)
> - if ((e = fcntl(fd, F_GETFL)) == -1)
> + if (ioctl(fd, FIONBIO, ) == -1)
> return -1;
> -
> - if (fcntl(fd, F_SETFL, e & ~O_NDELAY) == -1)
> - return -1;
> - else
> - e = 1;
> -#endif /* F_SETFL && O_NDELAY */
> -
> -#ifdef FIONBIO
> - {
> - int zero = 0;
> -
> - if (ioctl(fd, FIONBIO, ) == -1)
> - return -1;
> - else
> - e = 1;
> - }
> -#endif /* FIONBIO */
> -
> -#endif /* TRY_AGAIN */
> - return e ? 0 : -1;
> + return 0;
>
> case EINTR:
> return 0;
>
libedit: read__fixio cleanup
Hi,
deraadt@ recently reported to me that the editline(3) library, while
line editing is active - for example inside el_gets(3) - ignores
the first SIGINT it receives, for example the the first Ctrl-C the
user presses. I consider that a bug in the editline(3) library.
Some programs, for example our old ftp(1) implementation, work
around the bug in a horrible way by using setjmp(3)/longjmp(3).
The root cause of the problem is in libedit/read.c, in the interaction
of the functions read_char() and read__fixio(). Before fixing the bug
can reasonably be considered, the function read__fixio() direly needs
cleanup. As it stands, it is utterly unreadable.
So here is a patch to make it clear what the function does, with
no functional change intended yet (-37 +5 LOC).
There will be one or more follow-up patches. If you want to receive
them, please reply to me, i won't necessarily send them all to
tech@.
I see some value in avoiding gratuitious divergence from NetBSD,
but getting rid of this horrible mess is not gratuitious.
Rationale:
* Do not mark an argument as unused that is actually used.
* errno cannot possibly be -1. Even is it were, treating it as
EAGAIN makes no sense, treating it as the most severe error
imaginable makes more sense to me.
* We define EWOULDBLOCK to be the same as EAGAIN, so no need
to handle it separately.
* No need to #define TRY_AGAIN to use it just once.
* Don't do the same thing twice. We do support the FIONBIO ioctl(2),
so the the indirection using the F_GETFL fcntl(2) can be deleted.
* No need to play confusing games with the "e" variable.
Just return -1 for error or 0 for success in a straightforward
manner.
OK?
Ingo
P.S.
I also considered whether this FIONBIO dance should better be done
at the initialization stage rather than after EAGAIN already happened.
But maybe not. This is a library. The application program might
set the fd to non-blocking mode at any time and then call el_gets(3)
again, in which case the library needs to restore blocking I/O to
do its work.
Index: read.c
===
RCS file: /cvs/src/lib/libedit/read.c,v
retrieving revision 1.44
diff -u -p -r1.44 read.c
--- read.c 25 May 2016 09:36:21 - 1.44
+++ read.c 8 Aug 2021 10:30:06 -
@@ -39,9 +39,10 @@
* read.c: Clean this junk up! This is horrible code.
*Terminal read functions
*/
+#include
+
#include
#include
-#include
#include
#include
#include
@@ -134,55 +135,16 @@ el_read_getfn(struct el_read_t *el_read)
/* read__fixio():
* Try to recover from a read error
*/
-/* ARGSUSED */
static int
-read__fixio(int fd __attribute__((__unused__)), int e)
+read__fixio(int fd, int e)
{
+ int zero = 0;
switch (e) {
- case -1:/* Make sure that the code is reachable */
-
-#ifdef EWOULDBLOCK
- case EWOULDBLOCK:
-#ifndef TRY_AGAIN
-#define TRY_AGAIN
-#endif
-#endif /* EWOULDBLOCK */
-
-#if defined(POSIX) && defined(EAGAIN)
-#if defined(EWOULDBLOCK) && EWOULDBLOCK != EAGAIN
case EAGAIN:
-#ifndef TRY_AGAIN
-#define TRY_AGAIN
-#endif
-#endif /* EWOULDBLOCK && EWOULDBLOCK != EAGAIN */
-#endif /* POSIX && EAGAIN */
-
- e = 0;
-#ifdef TRY_AGAIN
-#if defined(F_SETFL) && defined(O_NDELAY)
- if ((e = fcntl(fd, F_GETFL)) == -1)
+ if (ioctl(fd, FIONBIO, ) == -1)
return -1;
-
- if (fcntl(fd, F_SETFL, e & ~O_NDELAY) == -1)
- return -1;
- else
- e = 1;
-#endif /* F_SETFL && O_NDELAY */
-
-#ifdef FIONBIO
- {
- int zero = 0;
-
- if (ioctl(fd, FIONBIO, ) == -1)
- return -1;
- else
- e = 1;
- }
-#endif /* FIONBIO */
-
-#endif /* TRY_AGAIN */
- return e ? 0 : -1;
+ return 0;
case EINTR:
return 0;
Re: dhcpleased(8): ignore servers / parts of lease
On Sun, Aug 08, 2021 at 12:37:54PM +0200, Florian Obser wrote: > This implements ignoring of nameservers and / or routes in leases as > well as completely ignoring servers (you cannot block rogue DHCP servers > in pf because bpf sees packets before pf). > > Various people voiced the need for these features. > Tests, OKs? > > diff --git dhcpleased.conf.5 dhcpleased.conf.5 > index 9e6846f899e..b856113bac1 100644 > --- dhcpleased.conf.5 > +++ dhcpleased.conf.5 > @@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults: > .Ic interface > options are as follows: > .Bl -tag -width Ds > +.It Ic ignore dns > +Ignore nameservers from leases on this interface. > +The default is to not ignore nameservers. > +.It Ic ignore routes > +Ignore routes from leases on this interface. > +The default is to not ignore routes. > +.It Ic ignore Ar server-ip > +Ignore leases from > +.Ar server-ip . > +This option can be listed multiple times. > +The default is to not ignore servers. hi. you probably want .It Ic ignore Ar server-ip ... then you can either remove the "multiple times" text to shorten the text block, or leave it in to be explicit. the diff reads fine. jmc > .It Ic send client id Ar client-id > Send the dhcp client identifier option with a value of > .Ar client-id .
dhcpleased(8): ignore servers / parts of lease
This implements ignoring of nameservers and / or routes in leases as
well as completely ignoring servers (you cannot block rogue DHCP servers
in pf because bpf sees packets before pf).
Various people voiced the need for these features.
Tests, OKs?
diff --git dhcpleased.c dhcpleased.c
index 36a4a21c21a..e005d7de9ae 100644
--- dhcpleased.c
+++ dhcpleased.c
@@ -569,6 +569,20 @@ main_dispatch_engine(int fd, short event, void *bula)
sizeof(imsg_interface.if_index));
break;
}
+ case IMSG_WITHDRAW_ROUTES: {
+ struct imsg_configure_interface imsg_interface;
+ if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
+ fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
+ "length: %lu", __func__,
+ IMSG_DATA_SIZE(imsg));
+ memcpy(_interface, imsg.data,
+ sizeof(imsg_interface));
+ if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
+ fatalx("%s: too many routes in imsg", __func__);
+ if (imsg_interface.routes_len > 0)
+ configure_routes(RTM_DELETE, _interface);
+ break;
+ }
case IMSG_PROPOSE_RDNS: {
struct imsg_propose_rdns rdns;
if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
diff --git dhcpleased.conf.5 dhcpleased.conf.5
index 9e6846f899e..b856113bac1 100644
--- dhcpleased.conf.5
+++ dhcpleased.conf.5
@@ -57,6 +57,17 @@ A list of interfaces to overwrite defaults:
.Ic interface
options are as follows:
.Bl -tag -width Ds
+.It Ic ignore dns
+Ignore nameservers from leases on this interface.
+The default is to not ignore nameservers.
+.It Ic ignore routes
+Ignore routes from leases on this interface.
+The default is to not ignore routes.
+.It Ic ignore Ar server-ip
+Ignore leases from
+.Ar server-ip .
+This option can be listed multiple times.
+The default is to not ignore servers.
.It Ic send client id Ar client-id
Send the dhcp client identifier option with a value of
.Ar client-id .
diff --git dhcpleased.h dhcpleased.h
index 7f6ec87ad19..1d20b77dbd3 100644
--- dhcpleased.h
+++ dhcpleased.h
@@ -151,6 +151,12 @@
#defineDHCPRELEASE 7
#defineDHCPINFORM 8
+/* Ignore parts of DHCP lease */
+#defineIGN_ROUTES 1
+#defineIGN_DNS 2
+
+#defineMAX_SERVERS 16 /* max servers that can be ignores per
if */
+
#defineIMSG_DATA_SIZE(imsg)((imsg).hdr.len - IMSG_HEADER_SIZE)
#defineDHCP_SNAME_LEN 64
#defineDHCP_FILE_LEN 128
@@ -216,6 +222,7 @@ enum imsg_type {
IMSG_DECONFIGURE_INTERFACE,
IMSG_PROPOSE_RDNS,
IMSG_WITHDRAW_RDNS,
+ IMSG_WITHDRAW_ROUTES,
IMSG_REPROPOSE_RDNS,
IMSG_REQUEST_REBOOT,
};
@@ -246,6 +253,9 @@ struct iface_conf {
int vc_id_len;
uint8_t *c_id;
int c_id_len;
+ int ignore;
+ struct in_addr ignore_servers[MAX_SERVERS];
+ int ignore_servers_len;
};
struct dhcpleased_conf {
@@ -304,6 +314,8 @@ const char *sin_to_str(struct sockaddr_in *);
/* frontend.c */
struct iface_conf *find_iface_conf(struct iface_conf_head *, char *);
+int*changed_ifaces(struct dhcpleased_conf *, struct
+dhcpleased_conf *);
/* printconf.c */
void print_config(struct dhcpleased_conf *);
diff --git engine.c engine.c
index 076a57e9ba6..67693c358ee 100644
--- engine.c
+++ engine.c
@@ -139,6 +139,7 @@ void
send_configure_interface(struct dhcpleased_iface *);
voidsend_rdns_proposal(struct dhcpleased_iface *);
voidsend_deconfigure_interface(struct dhcpleased_iface *);
voidsend_rdns_withdraw(struct dhcpleased_iface *);
+voidsend_routes_withdraw(struct dhcpleased_iface *);
voidparse_lease(struct dhcpleased_iface *,
struct imsg_ifinfo *);
int engine_imsg_compose_main(int, pid_t, void *, uint16_t);
@@ -506,13 +507,37 @@ engine_dispatch_main(int fd, short event, void *bula)
IMSG_DATA_SIZE(imsg));
iface_conf->c_id_len = IMSG_DATA_SIZE(imsg);
break;
- case IMSG_RECONF_END:
+ case IMSG_RECONF_END: {
+ struct dhcpleased_iface *iface;
+ int *ifaces;
+ int i, if_index;
+
Re: usb: don't pass USBD_EXCLUSIVE_USE to usbd_open_pipe_intr()
On Sun, 8 Aug 2021 14:38:57 +1000
Jonathan Matthew wrote:
> While working on a new driver, I noticed we have a few places where we
> pass USBD_EXCLUSIVE_USE as the flags parameter to
> usbd_open_pipe_intr(), which is wrong.
>
> The interrupt pipe is always opened exclusively, and the flags
> parameter is actually passed to usbd_setup_xfer(), where it means
> USBD_NO_COPY, so any data written by the transfer is not copied to
> the buffer where the driver expects it.
>
> I don't have hardware supported by any these drivers, but most of them
> don't look at the transferred data, and in a couple of them, the
> interrupt pipe code is #if 0'd out, so I think there is little chance
> this changes anything.
>
> ok?
Yes, agree, that's wrong, and it's documented in the according man
pages of usbd_open_pipe_intr(9) and usbd_open_pipe(9) as well. If it
should break one of those drivers because they really need
'USBD_NO_COPY', I would suggest to set that explicitly in
usbd_open_pipe_intr(9). But I doubt.
So OK.
> Index: if_aue.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
> retrieving revision 1.111
> diff -u -p -u -p -r1.111 if_aue.c
> --- if_aue.c 31 Jul 2020 10:49:32 - 1.111
> +++ if_aue.c 8 Aug 2021 03:25:19 -
> @@ -1355,7 +1355,7 @@ aue_openpipes(struct aue_softc *sc)
> return (EIO);
> }
> err = usbd_open_pipe_intr(sc->aue_iface,
> sc->aue_ed[AUE_ENDPT_INTR],
> - USBD_EXCLUSIVE_USE, >aue_ep[AUE_ENDPT_INTR], sc,
> + 0, >aue_ep[AUE_ENDPT_INTR], sc,
> >aue_cdata.aue_ibuf, AUE_INTR_PKTLEN, aue_intr,
> AUE_INTR_INTERVAL);
> if (err) {
> Index: if_udav.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_udav.c,v
> retrieving revision 1.84
> diff -u -p -u -p -r1.84 if_udav.c
> --- if_udav.c 31 Jul 2020 10:49:32 - 1.84
> +++ if_udav.c 8 Aug 2021 03:25:19 -
> @@ -769,7 +769,7 @@ udav_openpipes(struct udav_softc *sc)
> /* XXX: interrupt endpoint is not yet supported */
> /* Open Interrupt pipe */
> err = usbd_open_pipe_intr(sc->sc_ctl_iface, sc->sc_intrin_no,
> - USBD_EXCLUSIVE_USE,
> >sc_pipe_intr, sc,
> + 0, >sc_pipe_intr, sc,
> >sc_cdata.udav_ibuf,
> UDAV_INTR_PKGLEN, udav_intr, UDAV_INTR_INTERVAL);
> if (err) {
> Index: if_ugl.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_ugl.c,v
> retrieving revision 1.26
> diff -u -p -u -p -r1.26 if_ugl.c
> --- if_ugl.c 31 Jul 2020 10:49:32 - 1.26
> +++ if_ugl.c 8 Aug 2021 03:25:20 -
> @@ -681,7 +681,7 @@ ugl_openpipes(struct ugl_softc *sc)
> return (EIO);
> }
> err = usbd_open_pipe_intr(sc->sc_iface,
> sc->sc_ed[UGL_ENDPT_INTR],
> - USBD_EXCLUSIVE_USE, >sc_ep[UGL_ENDPT_INTR], sc,
> + 0, >sc_ep[UGL_ENDPT_INTR], sc,
> sc->sc_ibuf, UGL_INTR_PKTLEN, ugl_intr,
> UGL_INTR_INTERVAL);
> if (err) {
> Index: if_upl.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_upl.c,v
> retrieving revision 1.78
> diff -u -p -u -p -r1.78 if_upl.c
> --- if_upl.c 31 Jul 2020 10:49:32 - 1.78
> +++ if_upl.c 8 Aug 2021 03:25:20 -
> @@ -661,7 +661,7 @@ upl_openpipes(struct upl_softc *sc)
> return (EIO);
> }
> err = usbd_open_pipe_intr(sc->sc_iface,
> sc->sc_ed[UPL_ENDPT_INTR],
> - USBD_EXCLUSIVE_USE, >sc_ep[UPL_ENDPT_INTR], sc,
> + 0, >sc_ep[UPL_ENDPT_INTR], sc,
> >sc_ibuf, UPL_INTR_PKTLEN, upl_intr,
> UPL_INTR_INTERVAL);
> if (err) {
> Index: if_url.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_url.c,v
> retrieving revision 1.88
> diff -u -p -u -p -r1.88 if_url.c
> --- if_url.c 31 Jul 2020 10:49:33 - 1.88
> +++ if_url.c 8 Aug 2021 03:25:20 -
> @@ -635,7 +635,7 @@ url_openpipes(struct url_softc *sc)
> /* XXX: interrupt endpoint is not yet supported */
> /* Open Interrupt pipe */
> err = usbd_open_pipe_intr(sc->sc_ctl_iface, sc->sc_intrin_no,
> - USBD_EXCLUSIVE_USE,
> >sc_pipe_intr, sc,
> + 0, >sc_pipe_intr, sc,
> >sc_cdata.url_ibuf,
> URL_INTR_PKGLEN, url_intr, URL_INTR_INTERVAL);
> if (err) {
> Index: if_wi_usb.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_wi_usb.c,v
> retrieving revision 1.73
> diff -u -p -u -p -r1.73 if_wi_usb.c
> --- if_wi_usb.c 31 Jul 2020 10:49:33 - 1.73
> +++ if_wi_usb.c 8 Aug 2021 03:25:21 -
> @@ -1233,7 +1233,7 @@ wi_usb_open_pipes(struct wi_usb_softc *s
>
> /* is this used? */
>
Re: Fix unsafe snmpd defaults
On Sat, 2021-08-07 at 22:47 +0100, Stuart Henderson wrote: > On 2021/08/07 15:17, Martijn van Duren wrote: > > Let me give one final pushback, if this doesn't convince you then feel > > free to commit sthen's diff without my OK, but make sure it stays in > > sync with snmp(1). > > I was convinced enough to try it, hence okaying your previous diff, but > practical experience after updating some production machines got me to > reconsider. What happened in my case: updated machine running snmpd, > stopped getting monitoring in one place, reconfigured that to use sha256 > (which either involves click-click-click in a web interface or poking > a database table), noticed some other stats weren't working too, spent > quarter of an hour figuring out how to tell the collector to use sha256, > eventually working out that it's not possible, revert the snmpd config, > revert clicky click, ended up with the same algorithms as before but > wasted time. Then I updated another one enough later that I'd forgotten > about the first until partway through the cycle again... > > > First let me sum up what defaults have changed since 6.9: > > - community authentication disabled (snmpv1/snmpv2c) > > - no default community name set set > > So this one breaks old SNMPv2 configs unless config is changed. > Between the widely abused and scanned-for UDP amplification vector on > "public", the hidden default "private" RW community that I bet many > people have enabled even if they changed public, and old snmpd still > accepting v2 sessions when v3 was configured (as I discovered thanks > to shadowserver after I reconfigured it to use v3 "to improve > security"), I think that was absolutely the right thing to do. > > > - seclevel moved from none to enc > > I will honestly be very surprised if this affects anyone, you have to > set the username/keys to configure SNMPv3 at all so you're highly likely > to configure them into the management station too. Possibly fixes an > amplification vector if you have a poorly chosen username but not all > that likely. > > > - enc moved from des to aes > > Might affect some users but I find it hard to believe it will be many. > Which OpenBSD sysadmin going to the trouble of configure SNMPv3 is going > to voluntarily pick des when aes is on offer, shown in the example config > file, and widely supported? That's the problem with defaults for these non-negotiated parameters. If someone went with the defaults was it a voluntary pick or "I have no idea what I'm doing, but it works, or maybe even an "I had no idea that option was there"? > > > - auth moved from hmac-sha1 to hmac-sha256 > > ...whereas there are good reasons why someone would use hmac-sha1 rather > than hmac-sha256, and auth is the only part of the usm settings not > shown in examples/snmpd.conf, > > # Enable SNMPv3 USM with authentication, encryption and two defined users > #seclevel enc > #user "user1" authkey "password123" enc aes enckey "321drowssap" > #user "user2" authkey "password456" enckey "654drowssap" > > (The thing I'm most concerned about with snmpd-provided SNMP service is > easily guessed defaults, especially with regard to the UDP amp vector. > It would be better to keep SNMP-provided stats more private, but that's > further down the list, and totally off the list for anyone currently > running with v2c and "public"). VACM is somewhere (way) down on my roadmap. > > > > Updated diff below. I've rearranged examples/snmpd.conf some more, > explictly listed auth/enc algs for all usm examples I agree > , got rid of the oid > settings that are specialist enough that someone wanting them can read > the manual, and shown setting the v2c community if needed. (I pondered > getting rid of "trap receiver" as well as it doesn't do a lot in snmpd > yet, only coldStart, but it has potential so I showed some options > instead). I have a diff floating around on tech@ adding snmpv3 support to it[0], which would require an additional user specified for v3. So that example should probably be changed (didn't notice it, because I usually test the example as is. I agree that it's not really useful as is, but I'm hopefully going to focus on a new "application" layer (what snmpd(8) currently calls mps) during k2k21, which should be fully async, which would allow us to reintroduce agentx support and thus get more use out of it. I also have a diff with OK floating around on tech@ for adding notification support to libagentx, but I haven't committed it yet because of lack of agentx support in snmpd(8) and got some second thoughts about the API. > > > (And now that I realise "the complicated services alg" is actually > "bitwise OR of 2^(OSI layer number) for all supported layers" I am now > going to step away from the computer and try to forget it ;) And now you got me to remember it... :( > > Index: etc/examples/snmpd.conf > === > RCS file:
