diff mbox

Differentiate between Unix Stream Socket and Sequential Packet Socket

Message ID 1471709886.22998.1.camel@trentalancia.net (mailing list archive)
State Superseded
Headers show

Commit Message

Guido Trentalancia Aug. 20, 2016, 4:18 p.m. UTC
Modify the SELinux kernel code so that it is able to differentiate between
a unix_stream_socket and a sequential_packet_socket.

A companion patch has been created for the Reference Policy and it will be
posted to its mailing list.

Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
---
 security/selinux/hooks.c            |    3 ++-
 security/selinux/include/classmap.h |    2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Paul Moore Aug. 20, 2016, 5:17 p.m. UTC | #1
On Sat, Aug 20, 2016 at 12:18 PM, Guido Trentalancia
<guido@trentalancia.net> wrote:
> Modify the SELinux kernel code so that it is able to differentiate between
> a unix_stream_socket and a sequential_packet_socket.
>
> A companion patch has been created for the Reference Policy and it will be
> posted to its mailing list.
>
> Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
> ---
>  security/selinux/hooks.c            |    3 ++-
>  security/selinux/include/classmap.h |    2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)

I'm going to need to hear a better explanation of why we need to make
this change.  What problem does this solve that you can't solve today?

> --- linux-4.7.1-orig/security/selinux/include/classmap.h        2016-08-18 17:39:50.639133429 +0200
> +++ linux-4.7.1/security/selinux/include/classmap.h     2016-08-18 17:52:25.921420278 +0200
> @@ -86,6 +86,8 @@ struct security_class_mapping secclass_m
>           { "ingress", "egress", NULL } },
>         { "netlink_socket",
>           { COMMON_SOCK_PERMS, NULL } },
> +       { "sequential_packet_socket",
> +         { COMMON_SOCK_PERMS, "connectto", NULL } },
>         { "packet_socket",
>           { COMMON_SOCK_PERMS, NULL } },
>         { "key_socket",
> --- linux-4.7.1-orig/security/selinux/hooks.c   2016-08-18 21:47:32.204199470 +0200
> +++ linux-4.7.1/security/selinux/hooks.c        2016-08-18 22:52:53.099296513 +0200
> @@ -1246,8 +1246,9 @@ static inline u16 socket_type_to_securit
>         switch (family) {
>         case PF_UNIX:
>                 switch (type) {
> -               case SOCK_STREAM:
>                 case SOCK_SEQPACKET:
> +                       return SECCLASS_SEQUENTIAL_PACKET_SOCKET;
> +               case SOCK_STREAM:
>                         return SECCLASS_UNIX_STREAM_SOCKET;
>                 case SOCK_DGRAM:
>                         return SECCLASS_UNIX_DGRAM_SOCKET;
Guido Trentalancia Aug. 20, 2016, 5:39 p.m. UTC | #2
Hello Paul,

thanks for getting back on this.

The patch follows a recent discussion with Christopher PeBenito on the Reference Policy mailing list.

Christopher suggested to modify the actual code.

I suppose it provides a better insight during code analysis on the type of socket connections being made and a more fine-grained control of permissions being granted or denied to the policy designer. 

For some reason however, I have seen code using the SOCK_SEQPACKET type and executed immediately after policy load (possibly from initramfs, before switchroot) showing up in the log files as using an unspecified socket type. I have explained already to Christopher that this patch won't change such behavior...

Guido Trentalancia 

On the 20th August 2016 19:17:58 CEST, Paul Moore <paul@paul-moore.com> wrote:
>On Sat, Aug 20, 2016 at 12:18 PM, Guido Trentalancia
><guido@trentalancia.net> wrote:
>> Modify the SELinux kernel code so that it is able to differentiate
>between
>> a unix_stream_socket and a sequential_packet_socket.
>>
>> A companion patch has been created for the Reference Policy and it
>will be
>> posted to its mailing list.
>>
>> Signed-off-by: Guido Trentalancia <guido@trentalancia.net>
>> ---
>>  security/selinux/hooks.c            |    3 ++-
>>  security/selinux/include/classmap.h |    2 ++
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>
>I'm going to need to hear a better explanation of why we need to make
>this change.  What problem does this solve that you can't solve today?
>
>> --- linux-4.7.1-orig/security/selinux/include/classmap.h       
>2016-08-18 17:39:50.639133429 +0200
>> +++ linux-4.7.1/security/selinux/include/classmap.h     2016-08-18
>17:52:25.921420278 +0200
>> @@ -86,6 +86,8 @@ struct security_class_mapping secclass_m
>>           { "ingress", "egress", NULL } },
>>         { "netlink_socket",
>>           { COMMON_SOCK_PERMS, NULL } },
>> +       { "sequential_packet_socket",
>> +         { COMMON_SOCK_PERMS, "connectto", NULL } },
>>         { "packet_socket",
>>           { COMMON_SOCK_PERMS, NULL } },
>>         { "key_socket",
>> --- linux-4.7.1-orig/security/selinux/hooks.c   2016-08-18
>21:47:32.204199470 +0200
>> +++ linux-4.7.1/security/selinux/hooks.c        2016-08-18
>22:52:53.099296513 +0200
>> @@ -1246,8 +1246,9 @@ static inline u16 socket_type_to_securit
>>         switch (family) {
>>         case PF_UNIX:
>>                 switch (type) {
>> -               case SOCK_STREAM:
>>                 case SOCK_SEQPACKET:
>> +                       return SECCLASS_SEQUENTIAL_PACKET_SOCKET;
>> +               case SOCK_STREAM:
>>                         return SECCLASS_UNIX_STREAM_SOCKET;
>>                 case SOCK_DGRAM:
>>                         return SECCLASS_UNIX_DGRAM_SOCKET;
Paul Moore Aug. 20, 2016, 6:44 p.m. UTC | #3
On Sat, Aug 20, 2016 at 1:39 PM, Guido Trentalancia
<guido@trentalancia.net> wrote:
> Hello Paul,
>
> thanks for getting back on this.
>
> The patch follows a recent discussion with Christopher PeBenito on the Reference Policy mailing list.

Which patch/thread (what was the subject line)?  I have seen a lot of
patches and discussion between you and Chris lately (thanks for your
contributions!) but I haven't followed them very closely.

> Christopher suggested to modify the actual code.
>
> I suppose it provides a better insight during code analysis on the type of socket connections being made and a more fine-grained control of permissions being granted or denied to the policy designer.

The only value I can see to this change would be if we needed to
differentiate between AF_UNIX stream and seqpacket connections, and to
be honest I don't see the difference being that important.  As I said
before, we need to understand what you are trying to solve and how it
is only possible with this change.  The unspecified problem you are
seeing below wont be resolved by this patch (as you already
mentioned).

> For some reason however, I have seen code using the SOCK_SEQPACKET type and executed immediately after policy load (possibly from initramfs, before switchroot) showing up in the log files as using an unspecified socket type. I have explained already to Christopher that this patch won't change such behavior...

Yes, that should be unrelated to this change.  Are you able to
reproduce the above problem reliably?
Guido Trentalancia Aug. 20, 2016, 7:09 p.m. UTC | #4
Hello Paul!

The message subject used in the Reference Policy mailing list is: "Update the lvm module" and it's one of the most recent posting. 

I haven't tried yet reproducing the problem outside of the system bootup.

I believe it happens when cryptsetup uses the user-space interface to the kernel Crypto API.

Do you have any idea on the reason why the class is being marked as "socket" instead of "unix_stream_socket" (for sequential packet socket)? 

Best regards, 

Guido 

On the 20th august 2016 20:44:45 CEST, Paul Moore <pmoore@redhat.com> wrote:
>On Sat, Aug 20, 2016 at 1:39 PM, Guido Trentalancia
><guido@trentalancia.net> wrote:
>> Hello Paul,
>>
>> thanks for getting back on this.
>>
>> The patch follows a recent discussion with Christopher PeBenito on
>the Reference Policy mailing list.
>
>Which patch/thread (what was the subject line)?  I have seen a lot of
>patches and discussion between you and Chris lately (thanks for your
>contributions!) but I haven't followed them very closely.
>
>> Christopher suggested to modify the actual code.
>>
>> I suppose it provides a better insight during code analysis on the
>type of socket connections being made and a more fine-grained control
>of permissions being granted or denied to the policy designer.
>
>The only value I can see to this change would be if we needed to
>differentiate between AF_UNIX stream and seqpacket connections, and to
>be honest I don't see the difference being that important.  As I said
>before, we need to understand what you are trying to solve and how it
>is only possible with this change.  The unspecified problem you are
>seeing below wont be resolved by this patch (as you already
>mentioned).
>
>> For some reason however, I have seen code using the SOCK_SEQPACKET
>type and executed immediately after policy load (possibly from
>initramfs, before switchroot) showing up in the log files as using an
>unspecified socket type. I have explained already to Christopher that
>this patch won't change such behavior...
>
>Yes, that should be unrelated to this change.  Are you able to
>reproduce the above problem reliably?
Paul Moore Aug. 21, 2016, 3:24 a.m. UTC | #5
On Sat, Aug 20, 2016 at 3:09 PM, Guido Trentalancia
<guido@trentalancia.net> wrote:
> Hello Paul!
>
> The message subject used in the Reference Policy mailing list is: "Update the lvm module" and it's one of the most recent posting.
>
> I haven't tried yet reproducing the problem outside of the system bootup.
>
> I believe it happens when cryptsetup uses the user-space interface to the kernel Crypto API.
>
> Do you have any idea on the reason why the class is being marked as "socket" instead of "unix_stream_socket" (for sequential packet socket)?

Thanks for the pointer to the thread; that helped.

As far as the socket class is concerned, I wonder if cryptsetup is
using an AF_ALG socket?  Some quick Googling of the cryptsetup source
repo indicates this may be the case.  We don't currently have a
specific object class for the AF_ALG socket family so it would appear
as the generic socket class.
Guido Trentalancia Aug. 21, 2016, 5:31 p.m. UTC | #6
Hello Paul.

On Sat, 20/08/2016 at 23.24 -0400, Paul Moore wrote:
> On Sat, Aug 20, 2016 at 3:09 PM, Guido Trentalancia
> <guido@trentalancia.net> wrote:
> > 
> > Hello Paul!
> > 
> > The message subject used in the Reference Policy mailing list is:
> > "Update the lvm module" and it's one of the most recent posting.
> > 
> > I haven't tried yet reproducing the problem outside of the system
> > bootup.
> > 
> > I believe it happens when cryptsetup uses the user-space interface
> > to the kernel Crypto API.
> > 
> > Do you have any idea on the reason why the class is being marked as
> > "socket" instead of "unix_stream_socket" (for sequential packet
> > socket)?
> 
> Thanks for the pointer to the thread; that helped.
> 
> As far as the socket class is concerned, I wonder if cryptsetup is
> using an AF_ALG socket?  Some quick Googling of the cryptsetup source
> repo indicates this may be the case.  We don't currently have a
> specific object class for the AF_ALG socket family so it would appear
> as the generic socket class.

There has been a misunderstanding between the socket namespace and
style. Indeed, I was missing something !

I have now posted a new version of the patch (v2) which should properly
classify the new socket type.

Best regards,

Guido
Guido Trentalancia Aug. 21, 2016, 5:32 p.m. UTC | #7
Hello Paul.

On Sat, 20/08/2016 at 23.24 -0400, Paul Moore wrote:
> On Sat, Aug 20, 2016 at 3:09 PM, Guido Trentalancia
> <guido@trentalancia.net> wrote:
> > 
> > Hello Paul!
> > 
> > The message subject used in the Reference Policy mailing list is:
> > "Update the lvm module" and it's one of the most recent posting.
> > 
> > I haven't tried yet reproducing the problem outside of the system
> > bootup.
> > 
> > I believe it happens when cryptsetup uses the user-space interface
> > to the kernel Crypto API.
> > 
> > Do you have any idea on the reason why the class is being marked as
> > "socket" instead of "unix_stream_socket" (for sequential packet
> > socket)?
> 
> Thanks for the pointer to the thread; that helped.
> 
> As far as the socket class is concerned, I wonder if cryptsetup is
> using an AF_ALG socket?  Some quick Googling of the cryptsetup source
> repo indicates this may be the case.  We don't currently have a
> specific object class for the AF_ALG socket family so it would appear
> as the generic socket class.

There has been a misunderstanding between the socket namespace and
style. Indeed, I was missing something !

I have now posted a new version of the patch (v2) which should properly
classify the new socket type.

Best regards,

Guido
diff mbox

Patch

--- linux-4.7.1-orig/security/selinux/include/classmap.h	2016-08-18 17:39:50.639133429 +0200
+++ linux-4.7.1/security/selinux/include/classmap.h	2016-08-18 17:52:25.921420278 +0200
@@ -86,6 +86,8 @@  struct security_class_mapping secclass_m
 	  { "ingress", "egress", NULL } },
 	{ "netlink_socket",
 	  { COMMON_SOCK_PERMS, NULL } },
+	{ "sequential_packet_socket",
+	  { COMMON_SOCK_PERMS, "connectto", NULL } },
 	{ "packet_socket",
 	  { COMMON_SOCK_PERMS, NULL } },
 	{ "key_socket",
--- linux-4.7.1-orig/security/selinux/hooks.c	2016-08-18 21:47:32.204199470 +0200
+++ linux-4.7.1/security/selinux/hooks.c	2016-08-18 22:52:53.099296513 +0200
@@ -1246,8 +1246,9 @@  static inline u16 socket_type_to_securit
 	switch (family) {
 	case PF_UNIX:
 		switch (type) {
-		case SOCK_STREAM:
 		case SOCK_SEQPACKET:
+			return SECCLASS_SEQUENTIAL_PACKET_SOCKET;
+		case SOCK_STREAM:
 			return SECCLASS_UNIX_STREAM_SOCKET;
 		case SOCK_DGRAM:
 			return SECCLASS_UNIX_DGRAM_SOCKET;