diff mbox

LSM: Add post accept() hook.

Message ID 201007191325.IDI17618.VJStMOFOOLFFHQ@I-love.SAKURA.ne.jp
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

Tetsuo Handa July 19, 2010, 4:25 a.m. UTC 
David Miller wrote:
> > Eric Dumazet wrote:
> >> I read this patch and could not find out if an SNMP counter was
> >> increased in the case a frame was not delivered but dropped in kernel
> >> land.
> > 
> > UDP_MIB_INDATAGRAMS and UDP_MIB_INERRORS will not be increased
> > if dropped by security_socket_post_recvmsg()'s decision.
> > Should we increment UDP_MIB_INDATAGRAMS and/or UDP_MIB_INERRORS?
> 
> This decision should be guided by what we do for in the case
> of the other existing security hooks.
> 
> I don't think it makes any sense to make the post recvmsg() hook
> behave any differently from the existing hooks in this regard.

I see. Thank you.

I was misunderstanding assumption on select() -> recvmsg() sequence.
I was thinking that:

  If select() said "read operation will not block", the caller of recvmsg() can
  assume that recvmsg() which is preceded by select() will not be blocked.
  (The caller cannot assume that subsequent recvmsg() preceded by previous
  recvmsg() will not be blocked.) Therefore, the kernel must not wait for next
  message if current message was discarded by post recvmsg LSM hook. (And I
  thought that returning error code to the caller is the only way because the
  caller might be assuming that recvmsg() preceded by select() will not be
  blocked.)

But I understood that:

  Even if select() said "read operation will not block", the caller of recvmsg()
  can't assume that recvmsg() which is preceded by select() will not be blocked
  unless MSG_DONTWAIT or O_NONBLOCK was set.
  Therefore, the kernel is allowed to wait for next message if current message
  was discarded by post recvmsg LSM hook unless MSG_DONTWAIT or O_NONBLOCK was
  set.

Now, I'm thinking the same thing for select() -> accept() sequence:

  Even if select() said "read operation will not block", the caller of accept()
  can't assume that accept() which is preceded by select() will not be blocked
  unless MSG_DONTWAIT or O_NONBLOCK was set.
  Therefore, the kernel is allowed to wait for next connection if current
  connection was discarded by post accept LSM hook unless MSG_DONTWAIT or
  O_NONBLOCK was set.

Although "security_socket_post_accept() without retry loop" was proposed
in the past ( http://lkml.org/lkml/2010/3/2/297 ), I think I can propose
"security_socket_post_accept() with retry loop" (patch attached below)
if select() -> accept() case I wrote above is correct.

I can live with "security_socket_post_accept() without retry loop" by assigning
magic value to SOCK_INODE("struct socket *")->i_security field
( tomoyo_dead_sock() in http://lkml.org/lkml/2009/10/4/56 ) but below patch is
better for me because TOMOYO will not require the i_security field (which will
make it easier to realize LSM stacking/chaining) and will not need to implement
all LSM hooks for socket operations only for checking the i_security field.

May I have your opinion for below version?

Regards.
----------------------------------------
>From 54bc4ffee7998423e8c2d3a5cc9dfc647d5a892b Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 17 Jul 2010 12:04:18 +0900
Subject: [PATCH] LSM: Add post accept() hook.

Current pre accept hook (i.e. security_socket_accept()) has two problems.

One is that it will cause eating 100% of CPU time if the caller does not
close() the socket when accept() failed due to security_socket_accept(), for
subsequent select() notifies the caller of readiness for accept() since the
connection which would have been already picked up if security_socket_accept()
did not return error is remaining in the queue.

The other is that it is racy if LSM module wants to do filtering based on
"which process can pick up connections from which source" because the process
which picks up the connection is not known until sock->ops->accept() and lock
is not held between security_socket_accept() and sock->ops->accept.

This patch introduces post accept hook (i.e. security_socket_post_accept()) in
order to solve above problems at the cost of ability to pick up the connection
which would have been picked up if preceding security_socket_post_accept() did
not return error.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 include/linux/security.h |   21 +++++++++++++++++++++
 net/socket.c             |    7 +++++++
 security/capability.c    |    6 ++++++
 security/security.c      |    5 +++++
 4 files changed, 39 insertions(+), 0 deletions(-)

Comments

Paul Moore July 19, 2010, 10:15 p.m. UTC | #1 
On Monday, July 19, 2010 12:25:25 am Tetsuo Handa wrote:
> Current pre accept hook (i.e. security_socket_accept()) has two problems.
> 
> One is that it will cause eating 100% of CPU time if the caller does not
> close() the socket when accept() failed due to security_socket_accept(),
> for subsequent select() notifies the caller of readiness for accept()
> since the connection which would have been already picked up if
> security_socket_accept() did not return error is remaining in the queue.
> 
> The other is that it is racy if LSM module wants to do filtering based on
> "which process can pick up connections from which source" because the
> process which picks up the connection is not known until
> sock->ops->accept() and lock is not held between security_socket_accept()
> and sock->ops->accept.
> 
> This patch introduces post accept hook (i.e. security_socket_post_accept())
> in order to solve above problems at the cost of ability to pick up the
> connection which would have been picked up if preceding
> security_socket_post_accept() did not return error.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

I think you need to show how you plan to use this hook in an LSM before we can 
consider merging it with mainline.  What you are proposing here is giving an 
LSM the ability to drop a connection _after_ allowing it to be established in 
the first place; this seems very wrong to me and I want to make sure everyone 
else is aware of that before accepting this code into the kernel.  I 
understand that TOMOYO's security model does not allow it to reject incoming 
connections at the beginning of the connection request like some of the LSMs 
currently in use, but I'm just not very happy with the idea of finishing a 
connection handshake only to later drop the connection on the floor.

> ---
>  include/linux/security.h |   21 +++++++++++++++++++++
>  net/socket.c             |    7 +++++++
>  security/capability.c    |    6 ++++++
>  security/security.c      |    5 +++++
>  4 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 409c44d..2ed73c1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -866,6 +866,19 @@ static inline void security_free_mnt_opts(struct
> security_mnt_opts *opts) *	@sock contains the listening socket structure.
>   *	@newsock contains the newly created server socket for connection.
>   *	Return 0 if permission is granted.
> + * @socket_post_accept:
> + *	Check permission after accepting a new connection.
> + *	The connection is discarded if permission is not granted.
> + *	Return 0 after updating security information on the socket if you 
want
> + *	to restrict some of socket syscalls on the connection (e.g. forbid 
only
> + *	sending data). But you can't use this hook for updating security
> + *	information of the socket for preventing the connection from 
receiving
> + *	incoming data, for the kernel already started receiving incoming data
> + *	before accept() syscall. Return error if updating security 
information
> + *	failed or you want to forbid all of socket syscalls on the 
connection.
> + *	@sock contains the listening socket structure.
> + *	@newsock contains the accepted socket structure.
> + *	Return 0 if permission is granted.
>   * @socket_sendmsg:
>   *	Check permission before transmitting a message to another socket.
>   *	@sock contains the socket structure.
> @@ -1577,6 +1590,7 @@ struct security_operations {
>  			       struct sockaddr *address, int addrlen);
>  	int (*socket_listen) (struct socket *sock, int backlog);
>  	int (*socket_accept) (struct socket *sock, struct socket *newsock);
> +	int (*socket_post_accept) (struct socket *sock, struct socket *newsock);
>  	int (*socket_sendmsg) (struct socket *sock,
>  			       struct msghdr *msg, int size);
>  	int (*socket_recvmsg) (struct socket *sock,
> @@ -2530,6 +2544,7 @@ int security_socket_bind(struct socket *sock, struct
> sockaddr *address, int addr int security_socket_connect(struct socket
> *sock, struct sockaddr *address, int addrlen); int
> security_socket_listen(struct socket *sock, int backlog);
>  int security_socket_accept(struct socket *sock, struct socket *newsock);
> +int security_socket_post_accept(struct socket *sock, struct socket
> *newsock); int security_socket_sendmsg(struct socket *sock, struct msghdr
> *msg, int size); int security_socket_recvmsg(struct socket *sock, struct
> msghdr *msg, int size, int flags);
> @@ -2612,6 +2627,12 @@ static inline int security_socket_accept(struct
> socket *sock, return 0;
>  }
> 
> +static inline int security_socket_post_accept(struct socket *sock,
> +					      struct socket *newsock)
> +{
> +	return 0;
> +}
> +
>  static inline int security_socket_sendmsg(struct socket *sock,
>  					  struct msghdr *msg, int size)
>  {
> diff --git a/net/socket.c b/net/socket.c
> index 367d547..97d644c 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1473,6 +1473,7 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr
> __user *, upeer_sockaddr, if (!sock)
>  		goto out;
> 
> + retry:
>  	err = -ENFILE;
>  	if (!(newsock = sock_alloc()))
>  		goto out_put;
> @@ -1500,6 +1501,12 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr
> __user *, upeer_sockaddr, err = sock->ops->accept(sock, newsock,
> sock->file->f_flags);
>  	if (err < 0)
>  		goto out_fd;
> +	err = security_socket_post_accept(sock, newsock);
> +	if (unlikely(err)) {
> +		fput(newfile);
> +		put_unused_fd(newfd);
> +		goto retry;
> +	}
> 
>  	if (upeer_sockaddr) {
>  		if (newsock->ops->getname(newsock, (struct sockaddr *)&address,
> diff --git a/security/capability.c b/security/capability.c
> index 709aea3..1fb88f5 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -586,6 +586,11 @@ static int cap_socket_accept(struct socket *sock,
> struct socket *newsock) return 0;
>  }
> 
> +static int cap_socket_post_accept(struct socket *sock, struct socket
> *newsock) +{
> +	return 0;
> +}
> +
>  static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int
> size) {
>  	return 0;
> @@ -1004,6 +1009,7 @@ void __init security_fixup_ops(struct
> security_operations *ops) set_to_cap_if_null(ops, socket_connect);
>  	set_to_cap_if_null(ops, socket_listen);
>  	set_to_cap_if_null(ops, socket_accept);
> +	set_to_cap_if_null(ops, socket_post_accept);
>  	set_to_cap_if_null(ops, socket_sendmsg);
>  	set_to_cap_if_null(ops, socket_recvmsg);
>  	set_to_cap_if_null(ops, socket_post_recvmsg);
> diff --git a/security/security.c b/security/security.c
> index 4291bd7..5c9ab0a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1026,6 +1026,11 @@ int security_socket_accept(struct socket *sock,
> struct socket *newsock) return security_ops->socket_accept(sock, newsock);
>  }
> 
> +int security_socket_post_accept(struct socket *sock, struct socket
> *newsock) +{
> +	return security_ops->socket_post_accept(sock, newsock);
> +}
> +
>  int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int
> size) {
>  	return security_ops->socket_sendmsg(sock, msg, size);
Tetsuo Handa July 20, 2010, 1:36 a.m. UTC | #2 
Paul Moore wrote:
> I think you need to show how you plan to use this hook in an LSM before we can 
> consider merging it with mainline.  What you are proposing here is giving an 
> LSM the ability to drop a connection _after_ allowing it to be established in 
> the first place; this seems very wrong to me and I want to make sure everyone 
> else is aware of that before accepting this code into the kernel.  I 
> understand that TOMOYO's security model does not allow it to reject incoming 
> connections at the beginning of the connection request like some of the LSMs 
> currently in use, but I'm just not very happy with the idea of finishing a 
> connection handshake only to later drop the connection on the floor.

Yes. I'm planning to use security_socket_post_accept() for two purposes.



One is for dropping connections from unwanted hosts. Administrators define
policy before enabling enforcing mode (the mode which connections are dropped
if operation was not granted by policy). Administrators specify acceptable
hosts (i.e. hosts which this host needs to communicate with) and unacceptable
hosts (i.e. hosts which this host needn't to communicate with).
Dropping connections would happen if some process was hijacked and the process
attempted to communicate with other processes using TCP connections. But
dropping connections should not happen in normal circumstance.



The other is for updating process's state variable upon accept() operation.
LKM version of TOMOYO has per a task_struct variable that is used for
implementing stateful permissions. (As of now, not implemented for LSM version
of TOMOYO.) For example,

  allow_network TCP accept 10.0.0.0-10.255.255.255 1024-65535 ; set task.state[0]=1
  allow_network TCP accept 192.168.0.1-192.168.255.255 1024-65535 ; set task.state[0]=2

will change current thread's task state variable to 1 if current thread
accepted TCP connection from 10.0.0.0-10.255.255.255 and change it to 2 if from
192.168.0.1-192.168.255.255 . This variable is used for giving different
permissions for subsequent operations. For example,

  allow_execute /bin/bash if task.state[0]=1
  allow_execute /bin/tcsh if task.state[0]=2

will allow execution of /bin/bash if current thread is dealing connections from
10.0.0.0-10.255.255.255 and allow execution of /bin/tcsh if current thread is
dealing connections from 192.168.0.1-192.168.255.255 . Another example,

  allow_network TCP accept 0.0.0.0-255.255.255.255 1024-65535 ; set task.state[0]=3
  allow_network TCP accept 0.0.0.0-255.255.255.255 1-1023 ; set task.state[0]=4

will change it to 3 if from unprivileged port and change it to 4 if from
privileged port.

  allow_execute /bin/rbash if task.state[0]=3
  allow_execute /bin/bash if task.state[0]=4

will allow execution of /bin/rbash if dealing connections from unprivileged
ports and allow execution of /bin/bash if dealing connections from privileged
ports.

LSM hooks called before sock->ops->accept() cannot change current thread's task
state variable because it is racy, and LSM hook called after sock->ops->accept()
is missing.

Strictly speaking, it could be possible to update current thread's task state
variable in LSM hooks called by subsequent operations (e.g.
security_dentry_open(), security_bprm_set_creds()) by doing similar approach
done by tomoyo_dead_sock(), but updating it can fail (e.g. -ENOMEM) since
credentials are COW. If updating it failed, I want to drop the accept()ed
connection, but that is impossible from LSM hooks called by subsequent
operations. Killing current thread when updating it failed is possible, but
that looks worse for me than dropping connections upon accept() time (because
such action resembles OOM killer and likely gives larger damage to the caller).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Moore July 20, 2010, 7:52 p.m. UTC | #3 
On Monday, July 19, 2010 09:36:31 pm Tetsuo Handa wrote:
> Paul Moore wrote:
> > I think you need to show how you plan to use this hook in an LSM before
> > we can consider merging it with mainline.  What you are proposing here
> > is giving an LSM the ability to drop a connection _after_ allowing it to
> > be established in the first place; this seems very wrong to me and I
> > want to make sure everyone else is aware of that before accepting this
> > code into the kernel.  I understand that TOMOYO's security model does
> > not allow it to reject incoming connections at the beginning of the
> > connection request like some of the LSMs currently in use, but I'm just
> > not very happy with the idea of finishing a connection handshake only to
> > later drop the connection on the floor.
> 
> Yes. I'm planning to use security_socket_post_accept() for two purposes.
> 
> One is for dropping connections from unwanted hosts. Administrators define
> policy before enabling enforcing mode (the mode which connections are
> dropped if operation was not granted by policy). Administrators specify
> acceptable hosts (i.e. hosts which this host needs to communicate with)
> and unacceptable hosts (i.e. hosts which this host needn't to communicate
> with).

You can enforce per-host access controls without the need for a post-accept() 
hooks, e.g. security_sock_rcv_skb() and the netfilter hooks 
(NF_INET_POST_ROUTING, NF_INET_FORWARD, NF_INET_LOCAL_OUT).  Or are you 
interested in controlling which hosts an _application_ can communicate with?

> Dropping connections would happen if some process was hijacked and the
> process attempted to communicate with other processes using TCP
> connections. But dropping connections should not happen in normal
> circumstance.

It doesn't matter if dropping connections is normal or not, what matters is 
that it can happen.

> The other is for updating process's state variable upon accept() operation.
> LKM version of TOMOYO has per a task_struct variable that is used for
> implementing stateful permissions. (As of now, not implemented for LSM
> version of TOMOYO.)

I'm open to re-introducing a post-accept() hook that does not have a return 
value, in other words, a hook that can only be used to update LSM state and 
not affect the connection.  Although I do think you could probably achieve the 
same thing using some of the existing LSM hooks (look at how SELinux updates 
its state upon accept()) but that is something you would have to look it and 
see if it works for TOMOYO.
Tetsuo Handa July 21, 2010, 2 a.m. UTC | #4 
Paul Moore wrote:
> On Monday, July 19, 2010 09:36:31 pm Tetsuo Handa wrote:
> > One is for dropping connections from unwanted hosts. Administrators define
> > policy before enabling enforcing mode (the mode which connections are
> > dropped if operation was not granted by policy). Administrators specify
> > acceptable hosts (i.e. hosts which this host needs to communicate with)
> > and unacceptable hosts (i.e. hosts which this host needn't to communicate
> > with).
> 
> You can enforce per-host access controls without the need for a post-accept() 
> hooks, e.g. security_sock_rcv_skb() and the netfilter hooks 
> (NF_INET_POST_ROUTING, NF_INET_FORWARD, NF_INET_LOCAL_OUT).  Or are you 
> interested in controlling which hosts an _application_ can communicate with?

I'm interested in controlling which ports on which hosts a _process_ can
communicate with. In TOMOYO's words, "processes that belong to which TOMOYO's
domain can communicate with which ports on which hosts".

TOMOYO's rules are

  Processes that belong to FOO domain can open /etc/fstab for reading.
     ( allow_read /etc/fstab )

  Processes that belong to FOO domain can create /tmp/file with mode 0600.
     ( allow_create /tmp/file 0600 )

  Processes that belong to FOO domain can connect to port 80 on host
  10.20.30.40 using TCP protocol.
     ( allow_network TCP connect 10.20.30.40 80 )

and so on. But currently,

  Processes that belong to FOO domain can accept TCP connections from port 1024
  on host 10.20.30.40.
     ( allow_network TCP accept 10.20.30.40 1024 )

  Processes that belong to FOO domain can receive UDP messages from port 65535
  on host 100.200.10.20.
     ( allow_network UDP connect 100.200.10.20 65535 )

are impossible.

Regarding outgoing connections/datagrams, we can specify address/port
parameters from the point of view of _process_ who actually sends requests.
But regarding incoming connections/datagrams, we cannot specify address/port
parameters from the point of view of _process_ who actually receives requests.

We can enforce per-host access controls using iptables.
But we can't use iptables for controlling address/port parameters for incoming
connections/datagrams because the process who actually receives requests
(ServewrApp2 in below example) is not always the same as the process who
created the socket (ServerApp1 in below example).

> > Dropping connections would happen if some process was hijacked and the
> > process attempted to communicate with other processes using TCP
> > connections. But dropping connections should not happen in normal
> > circumstance.
> 
> It doesn't matter if dropping connections is normal or not, what matters is 
> that it can happen.
> 
> > The other is for updating process's state variable upon accept() operation.
> > LKM version of TOMOYO has per a task_struct variable that is used for
> > implementing stateful permissions. (As of now, not implemented for LSM
> > version of TOMOYO.)
> 
> I'm open to re-introducing a post-accept() hook that does not have a return 
> value, in other words, a hook that can only be used to update LSM state and 
> not affect the connection.  Although I do think you could probably achieve the 
> same thing using some of the existing LSM hooks (look at how SELinux updates 
> its state upon accept()) but that is something you would have to look it and 
> see if it works for TOMOYO.

I can't figure out why the hook must not affect the connection.
Is it possible to clarify using below players?

Server1 and Client1 are hosts which are connected on TCP/IP network.
ServerApp1 and ServerApp2 are applications running on Server1 which might call
socket(), bind(), listen(), accept(), send(), recv(), shutdown(), close() and
execute().
ClientApp1 and ClientApp2 are applications running on Client1 which might call
socket(), connect(), send(), recv(), shutdown(), close().
Router1 and Router2 are routers which exist between Server1 and Client1.

  +-------+   +-------+   +-------+   +-------+
  |Server1|---|Router1|---|Router2|---|Client1|
  +-------+   +-------+   +-------+   +-------+

Event sequences:

Server1                       Client1

  ServerApp1 creates a socket using socket().

  ServerApp1 binds to an address using bind().

  ServerApp1 listens to the address using listen().

                                ClientApp1 creates a socket using socket().

                                ClientApp1 issues connect() request.

                                  Sends SYN.

    Receives SYN.

    Sends SYN/ACK.

                                  Receives SYN/ACK.

                                  Sends ACK.

    Receives ACK.

                                ClientApp1 issues send() request.

                                  Sends data.

    Receives data.

    Sends ACK.

                                  Receives ACK.

                                ClientApp1 issues send() request.

                                  Sends data.

    Receives data.

    Sends ACK.

                                  Receives ACK.

  ServerApp1 calls execve("ServerApp2").

  ServerApp2 issues accept() request.

    security_socket_accept() is called.

    sock->ops->accept() is called.

    security_socket_post_accept() is called. (*3)

    newsock->ops->getname() is called. (*1)

    move_addr_to_user() is called. (*2)

    fd_install() is called.

  ServerApp2 issues some requests.

    Some LSM hooks will be called.




*1: This may fail and the connection is discarded if failed.
    Thus, newsock->ops->getname() affects the connection.
    This is not fault of ServerApp2. Maybe this is fault of ClientApp1 or
    Router1 or Router2, but discarding already established connection is
    justified.

*2: This may fail and the connection is discarded if failed.
    Thus, move_addr_to_user() affects the connection.
    Is this the fault of ServerApp2?
    If the upeer_sockaddr supplied by ServerApp2 was bad, this is the fault of
    ServerApp2. Thus, discarding already established connection is justified.
    If the upeer_sockaddr supplied by ServerApp2 was good but physical RAM was
    not yet assigned for the upeer_sockaddr, and OOM killer was invoked when
    attempted to write to upeer_sockaddr and OOM killer chose ServerApp2, and
    the ServerApp2 is killed. This is not fault of ServerApp2. But discarding
    already established connection is justified.

*3: newsock->ops->getname() and move_addr_to_user() already affects the
    connection. They discard already established connections even if the cause
    is not ServerApp2's fault. Why security_socket_post_accept() affecting the
    connection cannot be justified?

Router1 and Router2 can inject RST into the already established connections
at any time (if they are IDS/IPS or broken or malicious).
How does security_socket_post_accept() returning an error differs from these
routers injecting RST?

Regards.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Moore July 21, 2010, 4:06 p.m. UTC | #5 
On 07/20/10 22:00, Tetsuo Handa wrote:
> Paul Moore wrote:
>> On Monday, July 19, 2010 09:36:31 pm Tetsuo Handa wrote:
>>> One is for dropping connections from unwanted hosts. Administrators define
>>> policy before enabling enforcing mode (the mode which connections are
>>> dropped if operation was not granted by policy). Administrators specify
>>> acceptable hosts (i.e. hosts which this host needs to communicate with)
>>> and unacceptable hosts (i.e. hosts which this host needn't to communicate
>>> with).
>>
>> You can enforce per-host access controls without the need for a post-accept() 
>> hooks, e.g. security_sock_rcv_skb() and the netfilter hooks 
>> (NF_INET_POST_ROUTING, NF_INET_FORWARD, NF_INET_LOCAL_OUT).  Or are you 
>> interested in controlling which hosts an _application_ can communicate with?
> 
> I'm interested in controlling which ports on which hosts a _process_ can
> communicate with. In TOMOYO's words, "processes that belong to which TOMOYO's
> domain can communicate with which ports on which hosts".

...

>>> Dropping connections would happen if some process was hijacked and the
>>> process attempted to communicate with other processes using TCP
>>> connections. But dropping connections should not happen in normal
>>> circumstance.
>>
>> It doesn't matter if dropping connections is normal or not, what matters is 
>> that it can happen.
>>
>>> The other is for updating process's state variable upon accept() operation.
>>> LKM version of TOMOYO has per a task_struct variable that is used for
>>> implementing stateful permissions. (As of now, not implemented for LSM
>>> version of TOMOYO.)
>>
>> I'm open to re-introducing a post-accept() hook that does not have a return 
>> value, in other words, a hook that can only be used to update LSM state and 
>> not affect the connection.  Although I do think you could probably achieve the 
>> same thing using some of the existing LSM hooks (look at how SELinux updates 
>> its state upon accept()) but that is something you would have to look it and 
>> see if it works for TOMOYO.
> 
> I can't figure out why the hook must not affect the connection.
> Is it possible to clarify using below players?

I understand what you are trying to accomplish and my concern is that
(using the example below) ClientApp1 has it's connection with Server1
dropped suddenly _after_ Server1 successfully completes the TCP
handshake.  I've stated this concern several times.  I would much prefer
you reject the incoming connection during the initial TCP handshake.

> Server1 and Client1 are hosts which are connected on TCP/IP network.
> ServerApp1 and ServerApp2 are applications running on Server1 which might call
> socket(), bind(), listen(), accept(), send(), recv(), shutdown(), close() and
> execute().
> ClientApp1 and ClientApp2 are applications running on Client1 which might call
> socket(), connect(), send(), recv(), shutdown(), close().
> Router1 and Router2 are routers which exist between Server1 and Client1.
> 
>   +-------+   +-------+   +-------+   +-------+
>   |Server1|---|Router1|---|Router2|---|Client1|
>   +-------+   +-------+   +-------+   +-------+

...

> Router1 and Router2 can inject RST into the already established connections
> at any time (if they are IDS/IPS or broken or malicious).
> How does security_socket_post_accept() returning an error differs from these
> routers injecting RST?

We can't control all of the different intermediate nodes on a given
network path and it is true that some of these intermediate nodes
sometimes do "Bad Things" to network traffic (due either to limitations
in the design, placement in the network or poor implementation).
However, just because other nodes do "Bad Things" does not mean we need
to allow "Bad Things" to happen in the Linux end nodes.
diff mbox

Patch

diff --git a/include/linux/security.h b/include/linux/security.h
index 409c44d..2ed73c1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -866,6 +866,19 @@  static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@sock contains the listening socket structure.
  *	@newsock contains the newly created server socket for connection.
  *	Return 0 if permission is granted.
+ * @socket_post_accept:
+ *	Check permission after accepting a new connection.
+ *	The connection is discarded if permission is not granted.
+ *	Return 0 after updating security information on the socket if you want
+ *	to restrict some of socket syscalls on the connection (e.g. forbid only
+ *	sending data). But you can't use this hook for updating security
+ *	information of the socket for preventing the connection from receiving
+ *	incoming data, for the kernel already started receiving incoming data
+ *	before accept() syscall. Return error if updating security information
+ *	failed or you want to forbid all of socket syscalls on the connection.
+ *	@sock contains the listening socket structure.
+ *	@newsock contains the accepted socket structure.
+ *	Return 0 if permission is granted.
  * @socket_sendmsg:
  *	Check permission before transmitting a message to another socket.
  *	@sock contains the socket structure.
@@ -1577,6 +1590,7 @@  struct security_operations {
 			       struct sockaddr *address, int addrlen);
 	int (*socket_listen) (struct socket *sock, int backlog);
 	int (*socket_accept) (struct socket *sock, struct socket *newsock);
+	int (*socket_post_accept) (struct socket *sock, struct socket *newsock);
 	int (*socket_sendmsg) (struct socket *sock,
 			       struct msghdr *msg, int size);
 	int (*socket_recvmsg) (struct socket *sock,
@@ -2530,6 +2544,7 @@  int security_socket_bind(struct socket *sock, struct sockaddr *address, int addr
 int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen);
 int security_socket_listen(struct socket *sock, int backlog);
 int security_socket_accept(struct socket *sock, struct socket *newsock);
+int security_socket_post_accept(struct socket *sock, struct socket *newsock);
 int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size);
 int security_socket_recvmsg(struct socket *sock, struct msghdr *msg,
 			    int size, int flags);
@@ -2612,6 +2627,12 @@  static inline int security_socket_accept(struct socket *sock,
 	return 0;
 }
 
+static inline int security_socket_post_accept(struct socket *sock,
+					      struct socket *newsock)
+{
+	return 0;
+}
+
 static inline int security_socket_sendmsg(struct socket *sock,
 					  struct msghdr *msg, int size)
 {
diff --git a/net/socket.c b/net/socket.c
index 367d547..97d644c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1473,6 +1473,7 @@  SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	if (!sock)
 		goto out;
 
+ retry:
 	err = -ENFILE;
 	if (!(newsock = sock_alloc()))
 		goto out_put;
@@ -1500,6 +1501,12 @@  SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	err = sock->ops->accept(sock, newsock, sock->file->f_flags);
 	if (err < 0)
 		goto out_fd;
+	err = security_socket_post_accept(sock, newsock);
+	if (unlikely(err)) {
+		fput(newfile);
+		put_unused_fd(newfd);
+		goto retry;
+	}
 
 	if (upeer_sockaddr) {
 		if (newsock->ops->getname(newsock, (struct sockaddr *)&address,
diff --git a/security/capability.c b/security/capability.c
index 709aea3..1fb88f5 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -586,6 +586,11 @@  static int cap_socket_accept(struct socket *sock, struct socket *newsock)
 	return 0;
 }
 
+static int cap_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+	return 0;
+}
+
 static int cap_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
 {
 	return 0;
@@ -1004,6 +1009,7 @@  void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, socket_connect);
 	set_to_cap_if_null(ops, socket_listen);
 	set_to_cap_if_null(ops, socket_accept);
+	set_to_cap_if_null(ops, socket_post_accept);
 	set_to_cap_if_null(ops, socket_sendmsg);
 	set_to_cap_if_null(ops, socket_recvmsg);
 	set_to_cap_if_null(ops, socket_post_recvmsg);
diff --git a/security/security.c b/security/security.c
index 4291bd7..5c9ab0a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1026,6 +1026,11 @@  int security_socket_accept(struct socket *sock, struct socket *newsock)
 	return security_ops->socket_accept(sock, newsock);
 }
 
+int security_socket_post_accept(struct socket *sock, struct socket *newsock)
+{
+	return security_ops->socket_post_accept(sock, newsock);
+}
+
 int security_socket_sendmsg(struct socket *sock, struct msghdr *msg, int size)
 {
 	return security_ops->socket_sendmsg(sock, msg, size);