umb(4) WIP diff and questions

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

umb(4) WIP diff and questions

Lee B
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 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?

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?

Thanks,

Lee.


Index: sbin/ifconfig/ifconfig.8
===================================================================
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.343
diff -u -p -u -p -r1.343 ifconfig.8
--- sbin/ifconfig/ifconfig.8 10 Nov 2019 09:10:44 -0000 1.343
+++ sbin/ifconfig/ifconfig.8 14 Jan 2020 11:56:06 -0000
@@ -1914,6 +1914,9 @@ Clear the virtual network identifier.
 .Op Cm pin Ar pin
 .Op Cm puk Ar puk Ar newpin
 .Op Oo Fl Oc Ns Cm roaming
+.Op Oo Fl Oc Ns Cm umbuser Ar user
+.Op Oo Fl Oc Ns Cm umbpasswd Ar password
+.Op Oo Fl Oc Ns Cm umbauth Ar authmethod
 .Ek
 .nr nS 0
 .Pp
@@ -1960,6 +1963,26 @@ to validate the request.
 Enable data roaming.
 .It Cm -roaming
 Disable data roaming.
+.It Cm umbuser Ar user
+Set the authentication username.
+.It Cm -umbuser
+Clear the current username.
+.It Cm umbpasswd Ar password
+Set the authentication password.
+.It Cm -umbpasswd
+Clear the current password.
+.It Cm umbauth Ar authmethod
+Set the authentication method. Valid methods are:
+.Ar none ,
+.Ar pap ,
+.Ar chap ,
+.Ar mschap .
+Specifying an invalid method will cause a value of
+.Ar none
+to be used.
+.It Cm -umbauth
+Clear the authentication method. Equivalent to
+.Cm umbauth none .
 .It Cm up
 As soon as the interface is marked as "up", the
 .Xr umb 4
Index: sbin/ifconfig/ifconfig.c
===================================================================
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.417
diff -u -p -u -p -r1.417 ifconfig.c
--- sbin/ifconfig/ifconfig.c 27 Dec 2019 14:34:46 -0000 1.417
+++ sbin/ifconfig/ifconfig.c 14 Jan 2020 11:56:06 -0000
@@ -339,6 +339,9 @@ void umb_chgpin(const char *, const char
 void umb_puk(const char *, const char *);
 void umb_pinop(int, int, const char *, const char *);
 void umb_apn(const char *, int);
+void umb_user(const char *, int);
+void umb_passwd(const char *, int);
+void umb_authentication(const char *, int);
 void umb_setclass(const char *, int);
 void umb_roaming(const char *, int);
 void utf16_to_char(uint16_t *, int, char *, size_t);
@@ -587,6 +590,12 @@ const struct cmd {
  { "puk", NEXTARG2, 0, NULL, umb_puk },
  { "apn", NEXTARG, 0, umb_apn },
  { "-apn", -1, 0, umb_apn },
+ { "umbuser", NEXTARG, 0, umb_user },
+ { "-umbuser", -1, 0, umb_user },
+ { "umbpasswd", NEXTARG, 0, umb_passwd },
+ { "-umbpasswd", -1, 0, umb_passwd },
+ { "umbauth", NEXTARG, 0, umb_authentication },
+ { "-umbauth", -1, 0, umb_authentication },
  { "class", NEXTARG0, 0, umb_setclass },
  { "-class", -1, 0, umb_setclass },
  { "roaming", 1, 0, umb_roaming },
@@ -5628,6 +5637,7 @@ setifpriority(const char *id, int param)
 
 const struct umb_valdescr umb_regstate[] = MBIM_REGSTATE_DESCRIPTIONS;
 const struct umb_valdescr umb_dataclass[] = MBIM_DATACLASS_DESCRIPTIONS;
+const struct umb_valdescr umb_authprot[] = MBIM_AUTHPROT_DESCRIPTIONS;
 const struct umb_valdescr umb_simstate[] = MBIM_SIMSTATE_DESCRIPTIONS;
 const struct umb_valdescr umb_istate[] = UMB_INTERNAL_STATE_DESCRIPTIONS;
 const struct umb_valdescr umb_pktstate[] = MBIM_PKTSRV_STATE_DESCRIPTIONS;
@@ -5665,6 +5675,9 @@ umb_status(void)
  char iccid[UMB_ICCID_MAXLEN+1];
  char apn[UMB_APN_MAXLEN+1];
  char pn[UMB_PHONENR_MAXLEN+1];
+ char user[100];
+ char passwd[100];
+ int authprot;
  int i, n;
 
  memset((char *)&mi, 0, sizeof(mi));
@@ -5817,14 +5830,25 @@ umb_status(void)
 
  utf16_to_char(mi.pn, UMB_PHONENR_MAXLEN, pn, sizeof (pn));
  utf16_to_char(mi.apn, UMB_APN_MAXLEN, apn, sizeof (apn));
+
  if (pn[0] || apn[0]) {
  printf("\t");
  n = 0;
  if (pn[0])
  printf("%sphone# %s", n++ ? " " : "", pn);
- if (apn[0])
+ if (apn[0]) {
  printf("%sAPN %s", n++ ? " " : "", apn);
- printf("\n");
+ if (mi.authprot != MBIM_AUTHPROT_NONE) {
+ utf16_to_char(mi.user, 100, user,
+    sizeof (user));
+ utf16_to_char(mi.passwd, 100, passwd,
+    sizeof (passwd));
+ printf(" User %s Pass %s", user, passwd);
+ }
+ printf(" Auth %s",
+    umb_val2descr(umb_authprot, mi.authprot));
+ printf("\n");
+ }
  }
 
  for (i = 0, n = 0; i < UMB_MAX_DNSSRV; i++) {
@@ -5932,6 +5956,69 @@ umb_apn(const char *apn, int d)
  else if ((mp.apnlen = char_to_utf16(apn, mp.apn,
     sizeof (mp.apn))) == -1)
  errx(1, "APN too long");
+
+ if (ioctl(sock, SIOCSUMBPARAM, (caddr_t)&ifr) == -1)
+ err(1, "SIOCSUMBPARAM");
+}
+
+void
+umb_user(const char *user, int d)
+{
+ struct umb_parameter mp;
+
+ memset(&mp, 0, sizeof (mp));
+ ifr.ifr_data = (caddr_t)&mp;
+ if (ioctl(sock, SIOCGUMBPARAM, (caddr_t)&ifr) == -1)
+ err(1, "SIOCGUMBPARAM");
+
+ if (d != 0) {
+ printf("unsetting user\n");
+ memset(mp.user, 0, sizeof (mp.user));
+ }
+ else if ((mp.userlen = char_to_utf16(user, mp.user,
+    sizeof (mp.user))) == -1)
+ errx(1, "user name too long");
+
+ if (ioctl(sock, SIOCSUMBPARAM, (caddr_t)&ifr) == -1)
+ err(1, "SIOCSUMBPARAM");
+}
+
+void
+umb_passwd(const char *passwd, int d)
+{
+ struct umb_parameter mp;
+
+ memset(&mp, 0, sizeof (mp));
+ ifr.ifr_data = (caddr_t)&mp;
+ if (ioctl(sock, SIOCGUMBPARAM, (caddr_t)&ifr) == -1)
+ err(1, "SIOCGUMBPARAM");
+
+ if (d != 0) {
+ printf("unsetting passwd\n");
+ memset(mp.passwd, 0, sizeof (mp.passwd));
+ }
+ else if ((mp.passwdlen = char_to_utf16(passwd, mp.passwd,
+    sizeof (mp.passwd))) == -1)
+ errx(1, "password too long");
+
+ if (ioctl(sock, SIOCSUMBPARAM, (caddr_t)&ifr) == -1)
+ err(1, "SIOCSUMBPARAM");
+}
+
+void
+umb_authentication(const char *val, int d)
+{
+ struct umb_parameter mp;
+
+ memset(&mp, 0, sizeof (mp));
+ ifr.ifr_data = (caddr_t)&mp;
+ if (ioctl(sock, SIOCGUMBPARAM, (caddr_t)&ifr) == -1)
+ err(1, "SIOCGUMBPARAM");
+
+ if (d != -1)
+ mp.authprot = umb_descr2val(umb_authprot, val);
+ else
+ mp.authprot = MBIM_AUTHPROT_NONE;
 
  if (ioctl(sock, SIOCSUMBPARAM, (caddr_t)&ifr) == -1)
  err(1, "SIOCSUMBPARAM");
Index: share/man/man4/umb.4
===================================================================
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 umb.4
--- share/man/man4/umb.4 23 Nov 2017 20:47:26 -0000 1.9
+++ share/man/man4/umb.4 14 Jan 2020 11:56:06 -0000
@@ -51,6 +51,7 @@ The following devices should work:
 .It Sierra Wireless EM7455
 .It Sierra Wireless EM8805
 .It Sierra Wireless MC8305
+.It Sierra Wireless MC7700
 .El
 .Sh SEE ALSO
 .Xr intro 4 ,
Index: sys/dev/usb/if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 if_umb.c
--- sys/dev/usb/if_umb.c 26 Nov 2019 23:04:28 -0000 1.31
+++ sys/dev/usb/if_umb.c 14 Jan 2020 11:56:07 -0000
@@ -716,10 +716,28 @@ umb_ioctl(struct ifnet *ifp, u_long cmd,
  error = EINVAL;
  break;
  }
+ if (mp.userlen < 0 || mp.userlen > sizeof (sc->sc_info.user)) {
+ error = EINVAL;
+ break;
+ }
+ if (mp.passwdlen < 0 ||
+    mp.passwdlen > sizeof (sc->sc_info.passwd)) {
+ error = EINVAL;
+ break;
+ }
  sc->sc_roaming = mp.roaming ? 1 : 0;
+
  memset(sc->sc_info.apn, 0, sizeof (sc->sc_info.apn));
  memcpy(sc->sc_info.apn, mp.apn, mp.apnlen);
  sc->sc_info.apnlen = mp.apnlen;
+ memset(sc->sc_info.user, 0, sizeof (sc->sc_info.user));
+ memcpy(sc->sc_info.user, mp.user, mp.userlen);
+ sc->sc_info.userlen = mp.userlen;
+ memset(sc->sc_info.passwd, 0, sizeof (sc->sc_info.passwd));
+ memcpy(sc->sc_info.passwd, mp.passwd, mp.passwdlen);
+ sc->sc_info.passwdlen = mp.passwdlen;
+ sc->sc_info.authprot = mp.authprot;
+
  sc->sc_info.preferredclasses = mp.preferredclasses;
  umb_setdataclass(sc);
  break;
@@ -727,6 +745,12 @@ umb_ioctl(struct ifnet *ifp, u_long cmd,
  memset(&mp, 0, sizeof (mp));
  memcpy(mp.apn, sc->sc_info.apn, sc->sc_info.apnlen);
  mp.apnlen = sc->sc_info.apnlen;
+ memcpy(mp.user, sc->sc_info.user, sc->sc_info.userlen);
+ mp.userlen = sc->sc_info.userlen;
+ memcpy(mp.passwd, sc->sc_info.passwd, sc->sc_info.passwdlen);
+ mp.passwdlen = sc->sc_info.passwdlen;
+ mp.authprot = sc->sc_info.authprot;
+
  mp.roaming = sc->sc_roaming;
  mp.preferredclasses = sc->sc_info.preferredclasses;
  error = copyout(&mp, ifr->ifr_data, sizeof (mp));
@@ -2382,15 +2406,21 @@ umb_send_connect(struct umb_softc *sc, i
  c->sessionid = htole32(umb_session_id);
  c->command = htole32(command);
  off = offsetof(struct mbim_cid_connect, data);
+
  if (!umb_addstr(c, sizeof (*c), &off, sc->sc_info.apn,
     sc->sc_info.apnlen, &c->access_offs, &c->access_size))
  goto done;
- /* XXX FIXME: support user name and passphrase */
- c->user_offs = htole32(0);
- c->user_size = htole32(0);
- c->passwd_offs = htole32(0);
- c->passwd_size = htole32(0);
- c->authprot = htole32(MBIM_AUTHPROT_NONE);
+
+ if (sc->sc_info.authprot != MBIM_AUTHPROT_NONE) {
+ if (!umb_addstr(c, sizeof (*c), &off, sc->sc_info.user,
+    sc->sc_info.userlen, &c->user_offs, &c->user_size))
+ goto done;
+
+ if (!umb_addstr(c, sizeof (*c), &off, sc->sc_info.passwd,
+    sc->sc_info.passwdlen, &c->passwd_offs, &c->passwd_size))
+ goto done;
+ }
+ c->authprot = htole32(sc->sc_info.authprot);
  c->compression = htole32(MBIM_COMPRESSION_NONE);
  c->iptype = htole32(MBIM_CONTEXT_IPTYPE_IPV4);
  memcpy(c->context, umb_uuid_context_internet, sizeof (c->context));
Index: sys/dev/usb/if_umb.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.h,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 if_umb.h
--- sys/dev/usb/if_umb.h 26 Aug 2019 15:23:01 -0000 1.5
+++ sys/dev/usb/if_umb.h 14 Jan 2020 11:56:07 -0000
@@ -70,6 +70,13 @@ umb_val2descr(const struct umb_valdescr
  { MBIM_DATACLASS_CUSTOM, "custom" }, \
  { 0, NULL } }
 
+#define MBIM_AUTHPROT_DESCRIPTIONS { \
+ { MBIM_AUTHPROT_NONE, "none" }, \
+ { MBIM_AUTHPROT_PAP, "PAP" }, \
+ { MBIM_AUTHPROT_CHAP, "CHAP" }, \
+ { MBIM_AUTHPROT_MSCHAP, "MSCHAP" }, \
+ { 0, NULL } }
+
 #define MBIM_1TO1_DESCRIPTION(m) { (m), #m }
 #define MBIM_MESSAGES_DESCRIPTIONS { \
  MBIM_1TO1_DESCRIPTION(MBIM_OPEN_MSG), \
@@ -255,6 +262,14 @@ struct umb_parameter {
  uint16_t apn[UMB_APN_MAXLEN];
  int apnlen;
 
+ /* uint16_t user[MBIM_USER_MAXLEN]; */
+ uint16_t user[100];
+ int userlen;
+ /* uint16_t passwd[MBIM_PASSWD_MAXLEN]; */
+ uint16_t passwd[100];
+ int passwdlen;
+ int authprot;
+
  int roaming;
  uint32_t preferredclasses;
 };
@@ -300,6 +315,16 @@ struct umb_info {
 
  uint16_t apn[UMB_APN_MAXLEN];
  int apnlen;
+
+ /* uint16_t user[MBIM_USER_MAXLEN]; */
+ uint16_t user[100];
+ int userlen;
+
+ /* uint16_t passwd[MBIM_PASSWD_MAXLEN]; */
+ uint16_t passwd[100];
+ int passwdlen;
+
+ int authprot;
 
 #define UMB_VALUE_UNKNOWN -999
  int rssi;

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Stefan Sperling-5
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.

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Claudio Jeker
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

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Theo de Raadt-2
In reply to this post by Lee B
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.

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Stuart Henderson
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.

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Theo de Raadt-2
Stuart Henderson <[hidden email]> 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"

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Theo de Raadt-2
Theo de Raadt <[hidden email]> wrote:

> Stuart Henderson <[hidden email]> 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?

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Stefan Sperling-5
In reply to this post by Theo de Raadt-2
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 <sys/socket.h>
 #include <sys/ioctl.h>
 #include <sys/time.h>
+#include <sys/stat.h>
 
 #include <net/if.h>
 #include <net/if_dl.h>
@@ -106,6 +107,7 @@
 #include <resolv.h>
 #include <util.h>
 #include <ifaddrs.h>
+#include <fcntl.h>
 
 #ifndef SMALL
 #include <dev/usb/mbim.h>
@@ -211,6 +213,7 @@ void setifwpaakms(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 struct cmd {
  { "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)&wpa) == -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, &sb) == -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


Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Abel Abraham Camarillo Ojeda-2
On Tue, Jan 14, 2020 at 5:11 PM Stefan Sperling <[hidden email]> 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 <sys/socket.h>
>  #include <sys/ioctl.h>
>  #include <sys/time.h>
> +#include <sys/stat.h>
>
>  #include <net/if.h>
>  #include <net/if_dl.h>
> @@ -106,6 +107,7 @@
>  #include <resolv.h>
>  #include <util.h>
>  #include <ifaddrs.h>
> +#include <fcntl.h>
>
>  #ifndef SMALL
>  #include <dev/usb/mbim.h>
> @@ -211,6 +213,7 @@ void        setifwpaakms(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 struct        cmd {
>         { "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)&wpa) == -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, &sb) == -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
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Lee B
In reply to this post by Stefan Sperling-5
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.

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Lee B
In reply to this post by Theo de Raadt-2
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?


Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Ricardo Mestre-2
In reply to this post by Theo de Raadt-2
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.c 26 Nov 2019 23:04:28 -0000 1.31
+++ if_umb.c 21 Jan 2020 23:23:43 -0000
@@ -699,6 +699,8 @@ umb_ioctl(struct ifnet *ifp, u_long cmd,
  usb_add_task(sc->sc_udev, &sc->sc_umb_task);
  break;
  case SIOCGUMBINFO:
+ if ((error = suser(p)) != 0)
+ break;
  error = copyout(&sc->sc_info, ifr->ifr_data,
     sizeof (sc->sc_info));
  break;

On 12:40 Tue 14 Jan     , Theo de Raadt wrote:

> Theo de Raadt <[hidden email]> wrote:
>
> > Stuart Henderson <[hidden email]> 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?
>

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Lee B
In reply to this post by Claudio Jeker
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.

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Claudio Jeker
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

Reply | Threaded
Open this post in threaded view
|

Re: umb(4) WIP diff and questions

Lee B
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.