Re: umb(4) WIP diff and questions
On Thu Jan 23, 2020 at 3:05 AM, Claudio Jeker wrote: > On Thu, Jan 23, 2020 at 10:48:06AM +0900, Lee B wrote: > > > > OK, the umb_softc part was straightforward enough, thanks. I'd like > > some advice on how to handle the ifconfig(8) changes to accomodate > > this though. I see the wifi code appears to use a separate ioctl pair > > to handle the authentication credentials (WPA/PSK). Is this the right > > way to go, or have I missed an easier solution? > > > > > No that is the best way. Create a new ioctl to set the data. You can > decide if you want to pass the data together ot split in two commands. I > think a single ioctl has benefits for the driver but is more annoying to > handle in ifconfig. > Great, thank you. I'll hopefully get some time to finish this over the weekend. Lee.
Re: umb(4) WIP diff and questions
On Thu, Jan 23, 2020 at 10:48:06AM +0900, Lee B wrote: > On Tue Jan 14, 2020 at 5:59 PM, Claudio Jeker wrote: > > > > Since the credentials should not be passed back to userland I would not > > add them to struct umb_parameter but instead to struct umb_softc. > > Then you don't need to use struct umb_parameter for the ioctl and > > instead > > could just pass the (utf16) string to the kernel. > > Apart form that it looks good. > > > > > > OK, the umb_softc part was straightforward enough, thanks. I'd like > some advice on how to handle the ifconfig(8) changes to accomodate > this though. I see the wifi code appears to use a separate ioctl pair > to handle the authentication credentials (WPA/PSK). Is this the right > way to go, or have I missed an easier solution? > No that is the best way. Create a new ioctl to set the data. You can decide if you want to pass the data together ot split in two commands. I think a single ioctl has benefits for the driver but is more annoying to handle in ifconfig. -- :wq Claudio
Re: umb(4) WIP diff and questions
On Tue Jan 14, 2020 at 5:59 PM, Claudio Jeker wrote: > > Since the credentials should not be passed back to userland I would not > add them to struct umb_parameter but instead to struct umb_softc. > Then you don't need to use struct umb_parameter for the ioctl and > instead > could just pass the (utf16) string to the kernel. > Apart form that it looks good. > > OK, the umb_softc part was straightforward enough, thanks. I'd like some advice on how to handle the ifconfig(8) changes to accomodate this though. I see the wifi code appears to use a separate ioctl pair to handle the authentication credentials (WPA/PSK). Is this the right way to go, or have I missed an easier solution? Lee.
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: umb(4) WIP diff and questions
On Tue Jan 14, 2020 at 12:40 PM, Theo de Raadt wrote: > > > > 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? > > Right, seems like I should leave ifconfig(8) alone for the moment. I'll carry on with umb and the suggestions from Stefan and Claudio though if that's OK?
Re: umb(4) WIP diff and questions
Hi Stefan, On Tue Jan 14, 2020 at 2:40 PM, Stefan Sperling wrote: > <... lots of useful stuff ...> > That was exactly the sort of thing I was looking for. Thanks! It was seeing your device drivers presentation on Youtube a week or so ago that originally inspired me to get stuck in, so thanks for that, too. Lee.
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 5:11 PM Stefan Sperling wrote: > On Tue, Jan 14, 2020 at 12:34:29PM -0700, Theo de Raadt wrote: > > Channeling a conversation from 15 years ago: "How about wpakeyfile" > > ifconfig wpakeyfile would be trivial to add if we really want it. > But how will hostname.if will work when using join in netstart, one would need to: # cat /etc/hostname.iwm0 join ssid1 wpakeyfile /etc/wpa/ssd1-wpa.key join ssd2 wpakeyfile /etc/wpa/ssd2-wpa.key [etc...] ? > > The downside is loss of unveil, here handled the same way as for the > bridge rulesfile. Looks like unveil(argv[i], "r") is considered bad > practice even for an 'i' that should contain a path? > > diff a7540b3fac3fd3a71fd4134709ac4d4f71a3b5a4 /usr/src > blob - 3fb0780ba7cf1333894f5c3485a95e71885fbd6d > file + sbin/ifconfig/ifconfig.8 > --- sbin/ifconfig/ifconfig.8 > +++ sbin/ifconfig/ifconfig.8 > @@ -940,6 +940,7 @@ will begin advertising as master. > .Op Cm wpaciphers Ar cipher,cipher,... > .Op Cm wpagroupcipher Ar cipher > .Op Oo Fl Oc Ns Cm wpakey Ar passphrase | hexkey > +.Op Cm wpakeyfile Ar path > .Op Cm wpaprotos Ar proto,proto,... > .Ek > .nr nS 0 > @@ -990,6 +991,7 @@ the > .Cm join > list will record > .Cm wpakey , > +.Cm wpakeyfile , > .Cm wpaprotos , > or > .Cm nwkey > @@ -1209,6 +1211,8 @@ The default value is > .Dq psk > can only be used if a pre-shared key is configured using the > .Cm wpakey > +or > +.Cm wpakeyfile > option. > .It Cm wpaciphers Ar cipher,cipher,... > Set the comma-separated list of allowed pairwise ciphers. > @@ -1268,6 +1272,10 @@ or > option must first be specified, since > .Nm > will hash the nwid along with the passphrase to create the key. > +.It Cm wpakeyfile Ar path > +Set the WPA key contained in the file at the specified > +.Ar path . > +Trailing whitespace is ignored. > .It Cm -wpakey > Delete the pre-shared WPA key and disable WPA. > .It Cm wpaprotos Ar proto,proto,... > blob - f242d72cd73e8d50ccf1dd3d96ac62e35fe7025b > file + sbin/ifconfig/ifconfig.c > --- sbin/ifconfig/ifconfig.c > +++ sbin/ifconfig/ifconfig.c > @@ -63,6 +63,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -106,6 +107,7 @@ > #include > #include > #include > +#include > > #ifndef SMALL > #include > @@ -211,6 +213,7 @@ voidsetifwpaakms(const char *, int); > void setifwpaciphers(const char *, int); > void setifwpagroupcipher(const char *, int); > void setifwpakey(const char *, int); > +void setifwpakeyfile(const char *, int); > void setifchan(const char *, int); > void setifscan(const char *, int); > void setifnwflag(const char *, int); > @@ -415,6 +418,7 @@ const structcmd { > { "wpagroupcipher", NEXTARG,0, > setifwpagroupcipher }, > { "wpaprotos", NEXTARG,0, setifwpaprotos }, > { "wpakey", NEXTARG,0, setifwpakey }, > + { "wpakeyfile", NEXTARG,0, setifwpakeyfile }, > { "-wpakey",-1, 0, setifwpakey }, > { "chan", NEXTARG0, 0, setifchan }, > { "-chan", -1, 0, setifchan }, > @@ -728,7 +732,7 @@ main(int argc, char *argv[]) > int create = 0; > int Cflag = 0; > int gflag = 0; > - int found_rulefile = 0; > + int found_rulefile = 0, found_wpakeyfile = 0, wpafileidx = 0; > int i; > > /* If no args at all, print all interfaces. */ > @@ -785,9 +789,13 @@ main(int argc, char *argv[]) > found_rulefile = 1; > break; > } > + if (strcmp(argv[i], "wpakeyfile") == 0) { > + found_wpakeyfile = 1; > + break; > + } > } > > - if (!found_rulefile) { > + if (!found_rulefile && !found_wpakeyfile) { > if (unveil(_PATH_RESCONF, "r") == -1) > err(1, "unveil"); > if (unveil(_PATH_HOSTS, "r") == -1) > @@ -2240,6 +2248,40 @@ setifwpakey(const char *val, int d) > wpa.i_enabled = psk.i_enabled; > if (ioctl(sock, SIOCS80211WPAPARMS, (caddr_t)) == -1) > err(1, "SIOCS80211WPAPARMS"); > +} > + > +void > +setifwpakeyfile(const char *val, int d) > +{ > + char *wpakey; > + int fd; > + struct stat sb; > + ssize_t n; > + > + fd = open(val, O_RDONLY); > + if (fd == -1) > + err(1, "open %s", val); > + > + if (fstat(fd, ) == -1) > + err(1, "fstat %s", val); > + > + wpakey = malloc(sb.st_size); > + if (wpakey == NULL) > + err(1, "malloc"); > + > + n = read(fd, wpakey, sb.st_size); > + if (n == -1) > + err(1, "read %s", val); > + if (n != sb.st_size) > + errx(1, "failed to read from file %s", val); > + close(fd); > +
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 12:34:29PM -0700, Theo de Raadt wrote: > Channeling a conversation from 15 years ago: "How about wpakeyfile" ifconfig wpakeyfile would be trivial to add if we really want it. The downside is loss of unveil, here handled the same way as for the bridge rulesfile. Looks like unveil(argv[i], "r") is considered bad practice even for an 'i' that should contain a path? diff a7540b3fac3fd3a71fd4134709ac4d4f71a3b5a4 /usr/src blob - 3fb0780ba7cf1333894f5c3485a95e71885fbd6d file + sbin/ifconfig/ifconfig.8 --- sbin/ifconfig/ifconfig.8 +++ sbin/ifconfig/ifconfig.8 @@ -940,6 +940,7 @@ will begin advertising as master. .Op Cm wpaciphers Ar cipher,cipher,... .Op Cm wpagroupcipher Ar cipher .Op Oo Fl Oc Ns Cm wpakey Ar passphrase | hexkey +.Op Cm wpakeyfile Ar path .Op Cm wpaprotos Ar proto,proto,... .Ek .nr nS 0 @@ -990,6 +991,7 @@ the .Cm join list will record .Cm wpakey , +.Cm wpakeyfile , .Cm wpaprotos , or .Cm nwkey @@ -1209,6 +1211,8 @@ The default value is .Dq psk can only be used if a pre-shared key is configured using the .Cm wpakey +or +.Cm wpakeyfile option. .It Cm wpaciphers Ar cipher,cipher,... Set the comma-separated list of allowed pairwise ciphers. @@ -1268,6 +1272,10 @@ or option must first be specified, since .Nm will hash the nwid along with the passphrase to create the key. +.It Cm wpakeyfile Ar path +Set the WPA key contained in the file at the specified +.Ar path . +Trailing whitespace is ignored. .It Cm -wpakey Delete the pre-shared WPA key and disable WPA. .It Cm wpaprotos Ar proto,proto,... blob - f242d72cd73e8d50ccf1dd3d96ac62e35fe7025b file + sbin/ifconfig/ifconfig.c --- sbin/ifconfig/ifconfig.c +++ sbin/ifconfig/ifconfig.c @@ -63,6 +63,7 @@ #include #include #include +#include #include #include @@ -106,6 +107,7 @@ #include #include #include +#include #ifndef SMALL #include @@ -211,6 +213,7 @@ voidsetifwpaakms(const char *, int); void setifwpaciphers(const char *, int); void setifwpagroupcipher(const char *, int); void setifwpakey(const char *, int); +void setifwpakeyfile(const char *, int); void setifchan(const char *, int); void setifscan(const char *, int); void setifnwflag(const char *, int); @@ -415,6 +418,7 @@ const structcmd { { "wpagroupcipher", NEXTARG,0, setifwpagroupcipher }, { "wpaprotos", NEXTARG,0, setifwpaprotos }, { "wpakey", NEXTARG,0, setifwpakey }, + { "wpakeyfile", NEXTARG,0, setifwpakeyfile }, { "-wpakey",-1, 0, setifwpakey }, { "chan", NEXTARG0, 0, setifchan }, { "-chan", -1, 0, setifchan }, @@ -728,7 +732,7 @@ main(int argc, char *argv[]) int create = 0; int Cflag = 0; int gflag = 0; - int found_rulefile = 0; + int found_rulefile = 0, found_wpakeyfile = 0, wpafileidx = 0; int i; /* If no args at all, print all interfaces. */ @@ -785,9 +789,13 @@ main(int argc, char *argv[]) found_rulefile = 1; break; } + if (strcmp(argv[i], "wpakeyfile") == 0) { + found_wpakeyfile = 1; + break; + } } - if (!found_rulefile) { + if (!found_rulefile && !found_wpakeyfile) { if (unveil(_PATH_RESCONF, "r") == -1) err(1, "unveil"); if (unveil(_PATH_HOSTS, "r") == -1) @@ -2240,6 +2248,40 @@ setifwpakey(const char *val, int d) wpa.i_enabled = psk.i_enabled; if (ioctl(sock, SIOCS80211WPAPARMS, (caddr_t)) == -1) err(1, "SIOCS80211WPAPARMS"); +} + +void +setifwpakeyfile(const char *val, int d) +{ + char *wpakey; + int fd; + struct stat sb; + ssize_t n; + + fd = open(val, O_RDONLY); + if (fd == -1) + err(1, "open %s", val); + + if (fstat(fd, ) == -1) + err(1, "fstat %s", val); + + wpakey = malloc(sb.st_size); + if (wpakey == NULL) + err(1, "malloc"); + + n = read(fd, wpakey, sb.st_size); + if (n == -1) + err(1, "read %s", val); + if (n != sb.st_size) + errx(1, "failed to read from file %s", val); + close(fd); + + while (n > 0 && isspace(wpakey[n - 1])) { + wpakey[n - 1] = '\0'; + n--; + } + + setifwpakey(wpakey, d); } void
Re: umb(4) WIP diff and questions
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: umb(4) WIP diff and questions
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"
Re: umb(4) WIP diff and questions
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. 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.
Re: umb(4) WIP diff and questions
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.
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 02:40:45PM +0100, Stefan Sperling wrote: > On Tue, Jan 14, 2020 at 09:51:05PM +0900, leeb wrote: > > Hello again tech@ > > > > I've included diffs of what I've got so far at the bottom > > of this mail, but first a couple of questions: > > > > - Using the full 510-character limits for username and > > passphrase specified in the MBIM spec, kernel compilation > > fails due to tripping the 2047-byte stack frame warning > > when compiling the driver. That's the reason for the '100' > > magic numbers that I put in there temporarily. > > > > Any hints on the best way to handle this would be appreciated. > > I would allocate mp dynamically instead of storing it on the stack. > > In umb_ioctl(), you could change mp to a pointer type: > > struct umb_parameter *mp; > > The ioctl is allowed to sleep until memory is available (M_WAITOK), so > this allocation cannot fail and you don't need to check for NULL: > > mp = malloc(sizeof(*mp), M_DEVBUF, M_ZERO | M_WAITOK); > > free mp before umb_ioctl returns: > > free(mp, M_DEVBUF, sizeof(*mp)); > > See 'man 9 malloc' for details. > > > - I included the username/passphrase fields in the ifconfig > > output, as it seems to me that APN settings are generally > > made public so end-users can configure their own devices > > If needed I'll take it out, or perhaps change it to display > > '*set*' (or similar) if you think it should be hidden? > > Genrally, the kernel should not return credentials to userland. > So these shouldn't just be hidden or obscured in ifconfig. Rather, > umb_ioctl should not copy such data out to any userland program. > > This rule also applies to wifi and carp interfaces, for instance. > > > A couple of other points: > > > > - Wireless providers where I am all seem to require a > > username/password with the APN. So I'm unable to confirm > > whether or not this breaks non-authenticated connections. > > > > - I've been building my kernel using the documented > > procedure, and have no problems there. I ran through a > > base system build and that (eventually) completed OK too. > > When compiling ifconfig(8), I've been doing 'make includes' > > in /usr/src (after installing and booting my new kernel), > > and using 'make ifconfig' and 'make install' in > > /usr/src/sbin/ifconfig. Is this OK? or should I be doing > > something else instead? > > You should verify that 'make release' works after having completed a full > system build. The ramdisks use a special build of ifconfig so a check that > this still compiles is required. See 'man release' for details. > Since the credentials should not be passed back to userland I would not add them to struct umb_parameter but instead to struct umb_softc. Then you don't need to use struct umb_parameter for the ioctl and instead could just pass the (utf16) string to the kernel. Apart form that it looks good. -- :wq Claudio
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 09:51:05PM +0900, leeb wrote: > Hello again tech@ > > I've included diffs of what I've got so far at the bottom > of this mail, but first a couple of questions: > > - Using the full 510-character limits for username and > passphrase specified in the MBIM spec, kernel compilation > fails due to tripping the 2047-byte stack frame warning > when compiling the driver. That's the reason for the '100' > magic numbers that I put in there temporarily. > > Any hints on the best way to handle this would be appreciated. I would allocate mp dynamically instead of storing it on the stack. In umb_ioctl(), you could change mp to a pointer type: struct umb_parameter *mp; The ioctl is allowed to sleep until memory is available (M_WAITOK), so this allocation cannot fail and you don't need to check for NULL: mp = malloc(sizeof(*mp), M_DEVBUF, M_ZERO | M_WAITOK); free mp before umb_ioctl returns: free(mp, M_DEVBUF, sizeof(*mp)); See 'man 9 malloc' for details. > - I included the username/passphrase fields in the ifconfig > output, as it seems to me that APN settings are generally > made public so end-users can configure their own devices > If needed I'll take it out, or perhaps change it to display > '*set*' (or similar) if you think it should be hidden? Genrally, the kernel should not return credentials to userland. So these shouldn't just be hidden or obscured in ifconfig. Rather, umb_ioctl should not copy such data out to any userland program. This rule also applies to wifi and carp interfaces, for instance. > A couple of other points: > > - Wireless providers where I am all seem to require a > username/password with the APN. So I'm unable to confirm > whether or not this breaks non-authenticated connections. > > - I've been building my kernel using the documented > procedure, and have no problems there. I ran through a > base system build and that (eventually) completed OK too. > When compiling ifconfig(8), I've been doing 'make includes' > in /usr/src (after installing and booting my new kernel), > and using 'make ifconfig' and 'make install' in > /usr/src/sbin/ifconfig. Is this OK? or should I be doing > something else instead? You should verify that 'make release' works after having completed a full system build. The ramdisks use a special build of ifconfig so a check that this still compiles is required. See 'man release' for details.