[PATCH] selinux: hooks: Add permission for network MAC address

[PATCH] selinux: hooks: Add permission for network MAC address

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

First time poster to the list. I would appreciate feedback/suggestions
regarding the following patch.

This patch which provides SELinux control over network interface MAC
addresses. This patch allows access to the MAC address to be controlled by
policy. Network MAC addresses are a long lived unique device identifier,
and a security policy may wish to control access to the identifier without
further limiting network use, perhaps for privacy reasons.

The existing SE Linux permissions are too coarse in that they only allow
blanket read/no-read access to this socket ioctl. We would like to consider
both the read/no-read permission as well as an additional permission that
checks the ioctl cmd argument. This allows applications to continue
accessing the IP address, netmask, etc, while being denied access to the
MAC address.

Thanks,
Jeff Vander Stoep

---
 security/selinux/hooks.c            | 7 +++++++
 security/selinux/include/classmap.h | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1e1266b..cb65fd9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3142,6 +3142,13 @@ static int selinux_file_ioctl(struct file *file,
unsigned int cmd,
     SECURITY_CAP_AUDIT);
  break;

+    case SIOCGIFHWADDR:
+        error = file_has_perm(cred, file, FILE__IOCTL);
+        if (error)
+            break;
+        error = file_has_perm(cred, file, SOCKET__GET_HWADDR);
+        break;
+
  /* default case assumes that the command will go
  * to the file's ioctl() function.
  */
diff --git a/security/selinux/include/classmap.h
b/security/selinux/include/classmap.h
index c32ff7b..306f0d2 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,7 @@

 #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
     "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
-    "sendto", "recv_msg", "send_msg", "name_bind"
+    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"

 #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
"read", \
     "write", "associate", "unix_read", "unix_write"
-- 
2.1.0.rc2.206.gedb03e5

[-- Attachment #2: Type: text/html, Size: 3372 bytes --]

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Joshua Brindle]

On Wed, Oct 8, 2014 at 8:41 PM, Jeffrey Vander Stoep <jeffv@google.com> wrote:
> First time poster to the list. I would appreciate feedback/suggestions
> regarding the following patch.
>
> This patch which provides SELinux control over network interface MAC
> addresses. This patch allows access to the MAC address to be controlled by
> policy. Network MAC addresses are a long lived unique device identifier, and
> a security policy may wish to control access to the identifier without
> further limiting network use, perhaps for privacy reasons.
>
> The existing SE Linux permissions are too coarse in that they only allow
> blanket read/no-read access to this socket ioctl. We would like to consider
> both the read/no-read permission as well as an additional permission that
> checks the ioctl cmd argument. This allows applications to continue
> accessing the IP address, netmask, etc, while being denied access to the MAC
> address.
>

If someone has another system on the same network they can just get
the address by pinging it and running arp -a, right?

What use case do you see covering by adding this permission?

> Thanks,
> Jeff Vander Stoep
>
> ---
>  security/selinux/hooks.c            | 7 +++++++
>  security/selinux/include/classmap.h | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1e1266b..cb65fd9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3142,6 +3142,13 @@ static int selinux_file_ioctl(struct file *file,
> unsigned int cmd,
>      SECURITY_CAP_AUDIT);
>   break;
>
> +    case SIOCGIFHWADDR:
> +        error = file_has_perm(cred, file, FILE__IOCTL);
> +        if (error)
> +            break;
> +        error = file_has_perm(cred, file, SOCKET__GET_HWADDR);
> +        break;
> +
>   /* default case assumes that the command will go
>   * to the file's ioctl() function.
>   */
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index c32ff7b..306f0d2 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,7 @@
>
>  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
>      "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
> -    "sendto", "recv_msg", "send_msg", "name_bind"
> +    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
>
>  #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr", "read",
> \
>      "write", "associate", "unix_read", "unix_write"
> --
> 2.1.0.rc2.206.gedb03e5
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
> Selinux-request@tycho.nsa.gov.

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

Hi Joshua,

Thank you for responding.

The intent is not to hide the MAC address from the local-area-network, it
is to restrict which applications on the local machine have access to it.
The use-case is to more tightly sandbox untrusted applications from
accessing persistent hardware identifiers.

Consider a web client application that has a legitimate reason to access
the internet and network information in order to retrieve information from
a remote web server. Unbeknownst to the user, the application reads the MAC
address and sends it to the web server to uniquely identify/fingerprint the
hardware that its running on (instead of using more privacy-friendly
non-persistent methods like cookies, certificates, temp files,
username/password). Tracking with persistent identifiers such as the MAC
address is surprisingly common, invisible to the user, and it allows for
tracking that persists across multiple users, different networks, history
wipes, disk wipes, factory resets, username/password changes etc. The
ability to limit access to persistent hardware identifiers like the MAC
address allows for writing policy that enhances user privacy by making it
more difficult for software to tie itself to specific hardware. Many
applications need network access, very few need the MAC address. It would
be useful to be able to write policy that allows access to one without the
other.

On Wed, Oct 8, 2014 at 6:01 PM, Joshua Brindle <brindle@quarksecurity.com>
wrote:

> On Wed, Oct 8, 2014 at 8:41 PM, Jeffrey Vander Stoep <jeffv@google.com>
> wrote:
> > First time poster to the list. I would appreciate feedback/suggestions
> > regarding the following patch.
> >
> > This patch which provides SELinux control over network interface MAC
> > addresses. This patch allows access to the MAC address to be controlled
> by
> > policy. Network MAC addresses are a long lived unique device identifier,
> and
> > a security policy may wish to control access to the identifier without
> > further limiting network use, perhaps for privacy reasons.
> >
> > The existing SE Linux permissions are too coarse in that they only allow
> > blanket read/no-read access to this socket ioctl. We would like to
> consider
> > both the read/no-read permission as well as an additional permission that
> > checks the ioctl cmd argument. This allows applications to continue
> > accessing the IP address, netmask, etc, while being denied access to the
> MAC
> > address.
> >
>
> If someone has another system on the same network they can just get
> the address by pinging it and running arp -a, right?
>
> What use case do you see covering by adding this permission?
>
> > Thanks,
> > Jeff Vander Stoep
> >
> > ---
> >  security/selinux/hooks.c            | 7 +++++++
> >  security/selinux/include/classmap.h | 2 +-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1e1266b..cb65fd9 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3142,6 +3142,13 @@ static int selinux_file_ioctl(struct file *file,
> > unsigned int cmd,
> >      SECURITY_CAP_AUDIT);
> >   break;
> >
> > +    case SIOCGIFHWADDR:
> > +        error = file_has_perm(cred, file, FILE__IOCTL);
> > +        if (error)
> > +            break;
> > +        error = file_has_perm(cred, file, SOCKET__GET_HWADDR);
> > +        break;
> > +
> >   /* default case assumes that the command will go
> >   * to the file's ioctl() function.
> >   */
> > diff --git a/security/selinux/include/classmap.h
> > b/security/selinux/include/classmap.h
> > index c32ff7b..306f0d2 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -7,7 +7,7 @@
> >
> >  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> >      "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
> > -    "sendto", "recv_msg", "send_msg", "name_bind"
> > +    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
> >
> >  #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
> "read",
> > \
> >      "write", "associate", "unix_read", "unix_write"
> > --
> > 2.1.0.rc2.206.gedb03e5
> >
> >
> > _______________________________________________
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> > To get help, send an email containing "help" to
> > Selinux-request@tycho.nsa.gov.
>

[-- Attachment #2: Type: text/html, Size: 6831 bytes --]

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Nick Kralevich]

[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]

Thank you for putting together this patch Jeff.

FWIW, I'm supportive of patches like this. Allowing policy to restrict
access to unique persistent device identifiers without impacting other
ioctl operations feels like a small but important privacy win for me,
especially on distributions designed to run potentially untrustworthy
applications. Hopefully the wider SELinux team agrees with this, and we can
make progress towards producing a patch which works well for everyone.

Joshua: This patch wasn't intended to address off-device leakage of MAC
addresses. That's an important problem, but not something solvable via
SELinux policy.

-- Nick



On Wed, Oct 8, 2014 at 9:09 PM, Jeffrey Vander Stoep <jeffv@google.com>
wrote:

> Hi Joshua,
>
> Thank you for responding.
>
> The intent is not to hide the MAC address from the local-area-network, it
> is to restrict which applications on the local machine have access to it.
> The use-case is to more tightly sandbox untrusted applications from
> accessing persistent hardware identifiers.
>
> Consider a web client application that has a legitimate reason to access
> the internet and network information in order to retrieve information from
> a remote web server. Unbeknownst to the user, the application reads the MAC
> address and sends it to the web server to uniquely identify/fingerprint the
> hardware that its running on (instead of using more privacy-friendly
> non-persistent methods like cookies, certificates, temp files,
> username/password). Tracking with persistent identifiers such as the MAC
> address is surprisingly common, invisible to the user, and it allows for
> tracking that persists across multiple users, different networks, history
> wipes, disk wipes, factory resets, username/password changes etc. The
> ability to limit access to persistent hardware identifiers like the MAC
> address allows for writing policy that enhances user privacy by making it
> more difficult for software to tie itself to specific hardware. Many
> applications need network access, very few need the MAC address. It would
> be useful to be able to write policy that allows access to one without the
> other.
>
> On Wed, Oct 8, 2014 at 6:01 PM, Joshua Brindle <brindle@quarksecurity.com>
> wrote:
>
>> On Wed, Oct 8, 2014 at 8:41 PM, Jeffrey Vander Stoep <jeffv@google.com>
>> wrote:
>> > First time poster to the list. I would appreciate feedback/suggestions
>> > regarding the following patch.
>> >
>> > This patch which provides SELinux control over network interface MAC
>> > addresses. This patch allows access to the MAC address to be controlled
>> by
>> > policy. Network MAC addresses are a long lived unique device
>> identifier, and
>> > a security policy may wish to control access to the identifier without
>> > further limiting network use, perhaps for privacy reasons.
>> >
>> > The existing SE Linux permissions are too coarse in that they only allow
>> > blanket read/no-read access to this socket ioctl. We would like to
>> consider
>> > both the read/no-read permission as well as an additional permission
>> that
>> > checks the ioctl cmd argument. This allows applications to continue
>> > accessing the IP address, netmask, etc, while being denied access to
>> the MAC
>> > address.
>> >
>>
>> If someone has another system on the same network they can just get
>> the address by pinging it and running arp -a, right?
>>
>> What use case do you see covering by adding this permission?
>>
>> > Thanks,
>> > Jeff Vander Stoep
>> >
>> > ---
>> >  security/selinux/hooks.c            | 7 +++++++
>> >  security/selinux/include/classmap.h | 2 +-
>> >  2 files changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index 1e1266b..cb65fd9 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -3142,6 +3142,13 @@ static int selinux_file_ioctl(struct file *file,
>> > unsigned int cmd,
>> >      SECURITY_CAP_AUDIT);
>> >   break;
>> >
>> > +    case SIOCGIFHWADDR:
>> > +        error = file_has_perm(cred, file, FILE__IOCTL);
>> > +        if (error)
>> > +            break;
>> > +        error = file_has_perm(cred, file, SOCKET__GET_HWADDR);
>> > +        break;
>> > +
>> >   /* default case assumes that the command will go
>> >   * to the file's ioctl() function.
>> >   */
>> > diff --git a/security/selinux/include/classmap.h
>> > b/security/selinux/include/classmap.h
>> > index c32ff7b..306f0d2 100644
>> > --- a/security/selinux/include/classmap.h
>> > +++ b/security/selinux/include/classmap.h
>> > @@ -7,7 +7,7 @@
>> >
>> >  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
>> >      "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
>> > -    "sendto", "recv_msg", "send_msg", "name_bind"
>> > +    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
>> >
>> >  #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
>> "read",
>> > \
>> >      "write", "associate", "unix_read", "unix_write"
>> > --
>> > 2.1.0.rc2.206.gedb03e5
>> >
>> >
>> > _______________________________________________
>> > Selinux mailing list
>> > Selinux@tycho.nsa.gov
>> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> > To get help, send an email containing "help" to
>> > Selinux-request@tycho.nsa.gov.
>>
>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
> Selinux-request@tycho.nsa.gov.
>



-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037

[-- Attachment #2: Type: text/html, Size: 8650 bytes --]

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Joshua Brindle]

Nick Kralevich wrote:
> Thank you for putting together this patch Jeff.
>
> FWIW, I'm supportive of patches like this. Allowing policy to restrict
> access to unique persistent device identifiers without impacting other
> ioctl operations feels like a small but important privacy win for me,
> especially on distributions designed to run potentially untrustworthy
> applications. Hopefully the wider SELinux team agrees with this, and we
> can make progress towards producing a patch which works well for everyone.
>
> Joshua: This patch wasn't intended to address off-device leakage of MAC
> addresses. That's an important problem, but not something solvable via
> SELinux policy.
>

I understand now and agree that it is something desirable.

> -- Nick
>
>
>
> On Wed, Oct 8, 2014 at 9:09 PM, Jeffrey Vander Stoep <jeffv@google.com
> <mailto:jeffv@google.com>> wrote:
>
>     Hi Joshua,
>
>
>     Thank you for responding.
>
>
>     The intent is not to hide the MAC address from the
>     local-area-network, it is to restrict which applications on the
>     local machine have access to it. The use-case is to more tightly
>     sandbox untrusted applications from accessing persistent hardware
>     identifiers.
>
>
>     Consider a web client application that has a legitimate reason to
>     access the internet and network information in order to retrieve
>     information from a remote web server. Unbeknownst to the user, the
>     application reads the MAC address and sends it to the web server to
>     uniquely identify/fingerprint the hardware that its running on
>     (instead of using more privacy-friendly non-persistent methods like
>     cookies, certificates, temp files, username/password). Tracking with
>     persistent identifiers such as the MAC address is surprisingly
>     common, invisible to the user, and it allows for tracking that
>     persists across multiple users, different networks, history wipes,
>     disk wipes, factory resets, username/password changes etc. The
>     ability to limit access to persistent hardware identifiers like the
>     MAC address allows for writing policy that enhances user privacy by
>     making it more difficult for software to tie itself to specific
>     hardware. Many applications need network access, very few need the
>     MAC address. It would be useful to be able to write policy that
>     allows access to one without the other.
>
>     On Wed, Oct 8, 2014 at 6:01 PM, Joshua Brindle
>     <brindle@quarksecurity.com <mailto:brindle@quarksecurity.com>> wrote:
>
>         On Wed, Oct 8, 2014 at 8:41 PM, Jeffrey Vander Stoep
>         <jeffv@google.com <mailto:jeffv@google.com>> wrote:
>         >  First time poster to the list. I would appreciate
>         feedback/suggestions
>         >  regarding the following patch.
>         >
>         >  This patch which provides SELinux control over network
>         interface MAC
>         >  addresses. This patch allows access to the MAC address to be
>         controlled by
>         >  policy. Network MAC addresses are a long lived unique device
>         identifier, and
>         >  a security policy may wish to control access to the identifier
>         without
>         >  further limiting network use, perhaps for privacy reasons.
>         >
>         >  The existing SE Linux permissions are too coarse in that they
>         only allow
>         >  blanket read/no-read access to this socket ioctl. We would
>         like to consider
>         >  both the read/no-read permission as well as an additional
>         permission that
>         >  checks the ioctl cmd argument. This allows applications to
>         continue
>         >  accessing the IP address, netmask, etc, while being denied
>         access to the MAC
>         >  address.
>         >
>
>         If someone has another system on the same network they can just get
>         the address by pinging it and running arp -a, right?
>
>         What use case do you see covering by adding this permission?
>
>          > Thanks,
>          > Jeff Vander Stoep
>          >
>          > ---
>          >  security/selinux/hooks.c            | 7 +++++++
>          >  security/selinux/include/classmap.h | 2 +-
>          >  2 files changed, 8 insertions(+), 1 deletion(-)
>          >
>          > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>          > index 1e1266b..cb65fd9 100644
>          > --- a/security/selinux/hooks.c
>          > +++ b/security/selinux/hooks.c
>          > @@ -3142,6 +3142,13 @@ static int selinux_file_ioctl(struct
>         file *file,
>          > unsigned int cmd,
>          >      SECURITY_CAP_AUDIT);
>          >   break;
>          >
>          > +    case SIOCGIFHWADDR:
>          > +        error = file_has_perm(cred, file, FILE__IOCTL);
>          > +        if (error)
>          > +            break;
>          > +        error = file_has_perm(cred, file, SOCKET__GET_HWADDR);
>          > +        break;
>          > +
>          >   /* default case assumes that the command will go
>          >   * to the file's ioctl() function.
>          >   */
>          > diff --git a/security/selinux/include/classmap.h
>          > b/security/selinux/include/classmap.h
>          > index c32ff7b..306f0d2 100644
>          > --- a/security/selinux/include/classmap.h
>          > +++ b/security/selinux/include/classmap.h
>          > @@ -7,7 +7,7 @@
>          >
>          >  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind",
>         "connect", \
>          > "listen", "accept", "getopt", "setopt", "shutdown",
>         "recvfrom",  \
>          > - "sendto", "recv_msg", "send_msg", "name_bind"
>          > + "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
>          >
>          >  #define COMMON_IPC_PERMS "create", "destroy", "getattr",
>         "setattr", "read",
>          > \
>          > "write", "associate", "unix_read", "unix_write"
>          > --
>          > 2.1.0.rc2.206.gedb03e5
>          >
>          >
>          > _______________________________________________
>          > Selinux mailing list
>          > Selinux@tycho.nsa.gov <mailto:Selinux@tycho.nsa.gov>
>          > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov
>         <mailto:Selinux-leave@tycho.nsa.gov>.
>          > To get help, send an email containing "help" to
>          > Selinux-request@tycho.nsa.gov
>         <mailto:Selinux-request@tycho.nsa.gov>.
>
>
>
>     _______________________________________________
>     Selinux mailing list
>     Selinux@tycho.nsa.gov <mailto:Selinux@tycho.nsa.gov>
>     To unsubscribe, send email to Selinux-leave@tycho.nsa.gov
>     <mailto:Selinux-leave@tycho.nsa.gov>.
>     To get help, send an email containing "help" to
>     Selinux-request@tycho.nsa.gov <mailto:Selinux-request@tycho.nsa.gov>.
>
>
>
>
> --
> Nick Kralevich | Android Security | nnk@google.com
> <mailto:nnk@google.com> | 650.214.4037

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Stephen Smalley]

On 10/08/2014 08:41 PM, Jeffrey Vander Stoep wrote:
> First time poster to the list. I would appreciate feedback/suggestions
> regarding the following patch.
> 
> This patch which provides SELinux control over network interface MAC
> addresses. This patch allows access to the MAC address to be controlled by
> policy. Network MAC addresses are a long lived unique device identifier,
> and a security policy may wish to control access to the identifier without
> further limiting network use, perhaps for privacy reasons.
> 
> The existing SE Linux permissions are too coarse in that they only allow
> blanket read/no-read access to this socket ioctl. We would like to consider
> both the read/no-read permission as well as an additional permission that
> checks the ioctl cmd argument. This allows applications to continue
> accessing the IP address, netmask, etc, while being denied access to the
> MAC address.

There was some earlier discussion of filtering ioctls via SELinux.  If
you want to do that in general, this approach won't get you there - it
cannot scale to deal with more than a handful of ioctl commands.  If
however you truly only want this control for this particular ioctl
command, see below for comments on the code.  Otherwise, if you want to
explore a generic facility for ioctl filtering, I can send you pointers
to the earlier discussions.

> 
> Thanks,
> Jeff Vander Stoep
> 
> ---
>  security/selinux/hooks.c            | 7 +++++++
>  security/selinux/include/classmap.h | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1e1266b..cb65fd9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3142,6 +3142,13 @@ static int selinux_file_ioctl(struct file *file,
> unsigned int cmd,
>      SECURITY_CAP_AUDIT);
>   break;
> 
> +    case SIOCGIFHWADDR:
> +        error = file_has_perm(cred, file, FILE__IOCTL);
> +        if (error)
> +            break;
> +        error = file_has_perm(cred, file, SOCKET__GET_HWADDR);
> +        break;
> +

You can't safely call file_has_perm() with a SOCKET permission as the
file may reference an object with a non-socket security class.  So for
example, if a process called ioctl with this command value on a file,
you'd end up getting a very different permission check - whatever that
permission bit corresponds to in the file security class (I guess
nothing at this point, but could potentially be filled in the future).
So I think you need to first check that the file represents a socket
before performing the second check.  The other alternative would be to
define the permission in COMMON_FILE_SOCK_PERMS so that it is defined
for all objects that can be presented by struct file, but that would
waste a permission bit for the file classes.

For the first option, something along the lines of:
struct inode *inode = file->f_path.dentry->d_inode;
struct socket *socket;
if (inode->i_sb->s_magic != SOCKFS_MAGIC)
	break;
socket = SOCKET_I(inode);
error = sock_has_perm(current, socket->sk, SOCKET__GET_HWADDR);
break;


>   /* default case assumes that the command will go
>   * to the file's ioctl() function.
>   */
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index c32ff7b..306f0d2 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,7 @@
> 
>  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
>      "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
> -    "sendto", "recv_msg", "send_msg", "name_bind"
> +    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
> 
>  #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
> "read", \
>      "write", "associate", "unix_read", "unix_write"

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 4201 bytes --]

On Thu, Oct 9, 2014 at 7:48 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>
> There was some earlier discussion of filtering ioctls via SELinux.  If
> you want to do that in general, this approach won't get you there - it
> cannot scale to deal with more than a handful of ioctl commands.  If
> however you truly only want this control for this particular ioctl
> command, see below for comments on the code.  Otherwise, if you want to
> explore a generic facility for ioctl filtering, I can send you pointers
> to the earlier discussions.
>

That makes sense. I am a bit conflicted. This is my only use-case, but it
sounds like regular filtering of ioctl commands would be generally useful.
I understand that my current approach would not scale and would lead to an
explosion in permissions. So I am weighing the benefits of this particular
permission which seems broadly useful with the risk of either waiting on a
general approach or the risk of implementing something that doesn't get
mainlined. I would appreciate your thoughts on this.  Can you point me
towards the earlier discussions?

>
> You can't safely call file_has_perm() with a SOCKET permission as the
> file may reference an object with a non-socket security class.  So for
> example, if a process called ioctl with this command value on a file,
> you'd end up getting a very different permission check - whatever that
> permission bit corresponds to in the file security class (I guess
> nothing at this point, but could potentially be filled in the future).
> So I think you need to first check that the file represents a socket
> before performing the second check.  The other alternative would be to
> define the permission in COMMON_FILE_SOCK_PERMS so that it is defined
> for all objects that can be presented by struct file, but that would
> waste a permission bit for the file classes.
>
> For the first option, something along the lines of:
> struct inode *inode = file->f_path.dentry->d_inode;
> struct socket *socket;
> if (inode->i_sb->s_magic != SOCKFS_MAGIC)
>         break;
> socket = SOCKET_I(inode);
> error = sock_has_perm(current, socket->sk, SOCKET__GET_HWADDR);
> break;
>
>
>
>
Thank you for catching that. Here is the patch updated with your
recommendations:

---
 security/selinux/hooks.c            | 14 ++++++++++++++
 security/selinux/include/classmap.h |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1e1266b..5105341 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3104,10 +3104,14 @@ static void selinux_file_free_security(struct file
*file)
  file_free_security(file);
 }

+static int sock_has_perm(struct task_struct *task, struct sock *sk, u32
perms);
+
 static int selinux_file_ioctl(struct file *file, unsigned int cmd,
       unsigned long arg)
 {
  const struct cred *cred = current_cred();
+    struct inode *inode = file->f_path.dentry->d_inode;
+    struct socket *socket;
  int error = 0;

  switch (cmd) {
@@ -3142,6 +3146,16 @@ static int selinux_file_ioctl(struct file *file,
unsigned int cmd,
     SECURITY_CAP_AUDIT);
  break;

+    case SIOCGIFHWADDR:
+        error = file_has_perm(cred, file, FILE__IOCTL);
+        if (error)
+            break;
+        if (inode->i_sb->s_magic != SOCKFS_MAGIC)
+            break;
+        socket = SOCKET_I(inode);
+        error = sock_has_perm(current, socket->sk, SOCKET__GET_HWADDR);
+        break;
+
  /* default case assumes that the command will go
  * to the file's ioctl() function.
  */
diff --git a/security/selinux/include/classmap.h
b/security/selinux/include/classmap.h
index c32ff7b..306f0d2 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -7,7 +7,7 @@

 #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
     "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
-    "sendto", "recv_msg", "send_msg", "name_bind"
+    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"

 #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
"read", \
     "write", "associate", "unix_read", "unix_write"
-- 
2.1.0.rc2.206.gedb03e5

[-- Attachment #2: Type: text/html, Size: 6447 bytes --]

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Stephen Smalley]

On 10/09/2014 02:48 PM, Jeffrey Vander Stoep wrote:
> Thank you for catching that. Here is the patch updated with your
> recommendations:
> 
> ---
>  security/selinux/hooks.c            | 14 ++++++++++++++
>  security/selinux/include/classmap.h |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1e1266b..5105341 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3104,10 +3104,14 @@ static void selinux_file_free_security(struct file
> *file)
>   file_free_security(file);
>  }
> 
> +static int sock_has_perm(struct task_struct *task, struct sock *sk, u32
> perms);

Don't know if Paul agrees, but I'd tend to just move the entire
sock_has_perm() function up to before the beginning of any of the hook
functions (i.e. before the /* Hook functions begin here. */ comment)
rather than having a separate function prototype.  That's what we've
done for other helper functions that get called from various hooks.

Paul: Also wondering if sock_has_perm() ought to take a cred instead of
a task pointer these days (noticing the discrepancy between
file_has_perm and sock_has_perm below). And whether it even needs to
take the task/cred argument at all or could just use current_sid()
always given that all of its callers appear to pass current to it,
although I suppose it is more general/parallel to leave it.

Otherwise, I think the code is ok.  The patch was whitespace-damaged /
mangled by your mail client, so you'll want to fix that and disable html
mail when posting a final version (after addressing Paul's question
about any other hardware identifiers that might need similar controls).

> +
>  static int selinux_file_ioctl(struct file *file, unsigned int cmd,
>        unsigned long arg)
>  {
>   const struct cred *cred = current_cred();
> +    struct inode *inode = file->f_path.dentry->d_inode;
> +    struct socket *socket;
>   int error = 0;
> 
>   switch (cmd) {
> @@ -3142,6 +3146,16 @@ static int selinux_file_ioctl(struct file *file,
> unsigned int cmd,
>      SECURITY_CAP_AUDIT);
>   break;
> 
> +    case SIOCGIFHWADDR:
> +        error = file_has_perm(cred, file, FILE__IOCTL);
> +        if (error)
> +            break;
> +        if (inode->i_sb->s_magic != SOCKFS_MAGIC)
> +            break;
> +        socket = SOCKET_I(inode);
> +        error = sock_has_perm(current, socket->sk, SOCKET__GET_HWADDR);
> +        break;
> +
>   /* default case assumes that the command will go
>   * to the file's ioctl() function.
>   */
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index c32ff7b..306f0d2 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -7,7 +7,7 @@
> 
>  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
>      "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
> -    "sendto", "recv_msg", "send_msg", "name_bind"
> +    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
> 
>  #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
> "read", \
>      "write", "associate", "unix_read", "unix_write"
> 

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Paul Moore]

On Friday, October 10, 2014 02:22:51 PM Stephen Smalley wrote:
> On 10/09/2014 02:48 PM, Jeffrey Vander Stoep wrote:
> > Thank you for catching that. Here is the patch updated with your
> > recommendations:
> > 
> > ---
> > 
> >  security/selinux/hooks.c            | 14 ++++++++++++++
> >  security/selinux/include/classmap.h |  2 +-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1e1266b..5105341 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3104,10 +3104,14 @@ static void selinux_file_free_security(struct file
> > *file)
> > 
> >   file_free_security(file);
> >  
> >  }
> > 
> > +static int sock_has_perm(struct task_struct *task, struct sock *sk, u32
> > perms);
> 
> Don't know if Paul agrees, but I'd tend to just move the entire
> sock_has_perm() function up to before the beginning of any of the hook
> functions (i.e. before the /* Hook functions begin here. */ comment)
> rather than having a separate function prototype.  That's what we've
> done for other helper functions that get called from various hooks.

I agree with Stephen.  In my opinion separate function prototypes tend to be 
forgotten and diverge from the implementation, causing problems.

We could probably organize everything under security/selinux a bit better than 
what we have now, but that is another issue entirely and also a low priority 
one.

> Paul: Also wondering if sock_has_perm() ought to take a cred instead of
> a task pointer these days (noticing the discrepancy between
> file_has_perm and sock_has_perm below). And whether it even needs to
> take the task/cred argument at all or could just use current_sid()
> always given that all of its callers appear to pass current to it,
> although I suppose it is more general/parallel to leave it.

Good point.  I think I'd just assume remove the parameter entirely; if it 
wasn't for flush_unauthorized_files() we could do the same for 
file_has_perm().

I just put together a quick patch for this, as soon as it finishes building 
I'll post it.

> Otherwise, I think the code is ok.  The patch was whitespace-damaged /
> mangled by your mail client, so you'll want to fix that and disable html
> mail when posting a final version (after addressing Paul's question
> about any other hardware identifiers that might need similar controls).
> 
> > +
> > 
> >  static int selinux_file_ioctl(struct file *file, unsigned int cmd,
> >  
> >        unsigned long arg)
> >  
> >  {
> >  
> >   const struct cred *cred = current_cred();
> > 
> > +    struct inode *inode = file->f_path.dentry->d_inode;
> > +    struct socket *socket;
> > 
> >   int error = 0;
> >   
> >   switch (cmd) {
> > 
> > @@ -3142,6 +3146,16 @@ static int selinux_file_ioctl(struct file *file,
> > unsigned int cmd,
> > 
> >      SECURITY_CAP_AUDIT);
> >   
> >   break;
> > 
> > +    case SIOCGIFHWADDR:
> > +        error = file_has_perm(cred, file, FILE__IOCTL);
> > +        if (error)
> > +            break;
> > +        if (inode->i_sb->s_magic != SOCKFS_MAGIC)
> > +            break;
> > +        socket = SOCKET_I(inode);
> > +        error = sock_has_perm(current, socket->sk, SOCKET__GET_HWADDR);
> > +        break;
> > +
> > 
> >   /* default case assumes that the command will go
> >   * to the file's ioctl() function.
> >   */
> > 
> > diff --git a/security/selinux/include/classmap.h
> > b/security/selinux/include/classmap.h
> > index c32ff7b..306f0d2 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -7,7 +7,7 @@
> > 
> >  #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \
> >  
> >      "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom",  \
> > 
> > -    "sendto", "recv_msg", "send_msg", "name_bind"
> > +    "sendto", "recv_msg", "send_msg", "name_bind", "get_hwaddr"
> > 
> >  #define COMMON_IPC_PERMS "create", "destroy", "getattr", "setattr",
> > 
> > "read", \
> > 
> >      "write", "associate", "unix_read", "unix_write"

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Paul Moore]

On Wednesday, October 08, 2014 05:41:42 PM Jeffrey Vander Stoep wrote:
> First time poster to the list. I would appreciate feedback/suggestions
> regarding the following patch.
> 
> This patch which provides SELinux control over network interface MAC
> addresses. This patch allows access to the MAC address to be controlled by
> policy. Network MAC addresses are a long lived unique device identifier,
> and a security policy may wish to control access to the identifier without
> further limiting network use, perhaps for privacy reasons.

While I'm not opposed to such access controls, I'd like to see a more 
comprehensive patchset before we commit to support something like this in the 
mainline kernel.  Surely network MAC addresses aren't the only hardware 
identifier that one would be concerned about, right?

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: hooks: Add permission for network MAC address

[Jeffrey Vander Stoep]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

Fair enough. I will get back to you regarding other hardware identifiers
that we are considering filtering via ioctl commands.

>From our initial look, it appears that most other hardware identifiers can
be protected with existing selinux permissions. MAC is (thus far) unique in
that many applications need access to the other information provided by
that ioctl.

On Thu, Oct 9, 2014 at 1:20 PM, Paul Moore <paul@paul-moore.com> wrote:

> On Wednesday, October 08, 2014 05:41:42 PM Jeffrey Vander Stoep wrote:
> > First time poster to the list. I would appreciate feedback/suggestions
> > regarding the following patch.
> >
> > This patch which provides SELinux control over network interface MAC
> > addresses. This patch allows access to the MAC address to be controlled
> by
> > policy. Network MAC addresses are a long lived unique device identifier,
> > and a security policy may wish to control access to the identifier
> without
> > further limiting network use, perhaps for privacy reasons.
>
> While I'm not opposed to such access controls, I'd like to see a more
> comprehensive patchset before we commit to support something like this in
> the
> mainline kernel.  Surely network MAC addresses aren't the only hardware
> identifier that one would be concerned about, right?
>
> --
> paul moore
> www.paul-moore.com
>
>

[-- Attachment #2: Type: text/html, Size: 1920 bytes --]