[PATCH] tty: n_gsm: require CAP_NET_ADMIN to attach N_GSM0710 ldisc

[PATCH] tty: n_gsm: require CAP_NET_ADMIN to attach N_GSM0710 ldisc

[Thadeu Lima de Souza Cascardo]

Any unprivileged user can attach N_GSM0710 ldisc, but it requires
CAP_NET_ADMIN to create a GSM network anyway.

Require initial namespace CAP_NET_ADMIN to do that.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 drivers/tty/n_gsm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1cdefac4dd1b..c7a787f10a9c 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3576,6 +3576,9 @@ static int gsmld_open(struct tty_struct *tty)
 {
 	struct gsm_mux *gsm;
 
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
 	if (tty->ops->write == NULL)
 		return -EINVAL;
 
-- 
2.34.1

Re: [PATCH] tty: n_gsm: require CAP_NET_ADMIN to attach N_GSM0710 ldisc

[Greg Kroah-Hartman]

On Mon, Jul 31, 2023 at 03:59:42PM -0300, Thadeu Lima de Souza Cascardo wrote:
> Any unprivileged user can attach N_GSM0710 ldisc, but it requires
> CAP_NET_ADMIN to create a GSM network anyway.
> 
> Require initial namespace CAP_NET_ADMIN to do that.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

What commit id does this fix?  Or has this always been a problem?

thanks,

greg k-h

Re: [PATCH] tty: n_gsm: require CAP_NET_ADMIN to attach N_GSM0710 ldisc

[Thadeu Lima de Souza Cascardo]

On Tue, Aug 01, 2023 at 06:53:30AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 31, 2023 at 03:59:42PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > Any unprivileged user can attach N_GSM0710 ldisc, but it requires
> > CAP_NET_ADMIN to create a GSM network anyway.
> > 
> > Require initial namespace CAP_NET_ADMIN to do that.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> What commit id does this fix?  Or has this always been a problem?
> 
> thanks,
> 
> greg k-h

This has always been like this. It is not really fixing a specific commit, but
introducing further restriction on access.

Cascardo.

Re: [PATCH] tty: n_gsm: require CAP_NET_ADMIN to attach N_GSM0710 ldisc

[Greg Kroah-Hartman]

On Tue, Aug 01, 2023 at 10:02:16AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Aug 01, 2023 at 06:53:30AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jul 31, 2023 at 03:59:42PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > Any unprivileged user can attach N_GSM0710 ldisc, but it requires
> > > CAP_NET_ADMIN to create a GSM network anyway.
> > > 
> > > Require initial namespace CAP_NET_ADMIN to do that.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > 
> > What commit id does this fix?  Or has this always been a problem?
> > 
> > thanks,
> > 
> > greg k-h
> 
> This has always been like this. It is not really fixing a specific commit, but
> introducing further restriction on access.

So by restricting access, will this now break existing userspace tools
that do not have this permission?  I'm all for tightening up
permissions, but we can't break existing workflows without a good
reason.

thanks,

greg k-h

Re: [PATCH] tty: n_gsm: require CAP_NET_ADMIN to attach N_GSM0710 ldisc

[Thadeu Lima de Souza Cascardo]

On Thu, Aug 03, 2023 at 09:48:24AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Aug 01, 2023 at 10:02:16AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, Aug 01, 2023 at 06:53:30AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 31, 2023 at 03:59:42PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > > Any unprivileged user can attach N_GSM0710 ldisc, but it requires
> > > > CAP_NET_ADMIN to create a GSM network anyway.
> > > > 
> > > > Require initial namespace CAP_NET_ADMIN to do that.
> > > > 
> > > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> > > 
> > > What commit id does this fix?  Or has this always been a problem?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > This has always been like this. It is not really fixing a specific commit, but
> > introducing further restriction on access.
> 
> So by restricting access, will this now break existing userspace tools
> that do not have this permission?  I'm all for tightening up
> permissions, but we can't break existing workflows without a good
> reason.
> 
> thanks,
> 
> greg k-h

Yes, this will break any userspace trying to attach this without those
permissions.

I was under the impression that some operations on the line discipline also
required those same permissions, but they are actually operations on the
virtual demux ttys. So, at least we should change that on the commit
message.

The good reason to do it is reducing attack surface, given known bugs
in this code (see
https://lore.kernel.org/all/CA+UBctCZok5FSQ=LPRA+A-jocW=L8FuMVZ_7MNqhh483P5yN8A@mail.gmail.com/T/#u).

This has been done for N_HCI too
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c05731d0c6bd9a625e27ea5c5157ebf1303229e0).

The only significant user I found looking at codesearch.debian.net was
ofono, but I am having trouble finding out if the project is still active.
I am copying their list anyway here, in case it finds anyone who could tell
us that they are fine requiring such privileges.

Cascardo.