Helo again.
I need to get back to this issue, we still don't have a fix for socketpair()
and UDS sockets.
I was wondering if the fix i posted is ok (apart from the variables beeing
named inode), as you
can see this fields in the structure are initialized in smack_sk_alloc_security
(to cover
those sockets that won't be connected like those created by socketpair() ) and
in smack_unix_stream_connect()
for all other situations.
Do you have some other ideas about this issue, maybe there could be a more
generic way to fix this
in smack ? I was also wondering about a situation were a descriptor to a socket
would be passed
around more then once from a process to process, in the end it could actually
end up in the original
sending process, should we somehow take into account the path such a descriptor
would take in the
process tree ?
bes regards
On 09/25/2015 09:09 PM, Casey Schaufler wrote:
> On 9/23/2015 7:07 AM, Stephen Smalley wrote:
>> On 09/23/2015 05:00 AM, Roman Kubiak wrote:
>>> We found a lot of scenarios where the smack_file_receive() function might
>>> fail
>>>
>>> 1) for pipes a different patch is needed for the function to work
>>> (otherwise all pipe descriptors
>>> get the "_" label), patch below:
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index a143328..0ebfb07 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -3099,6 +3099,9 @@ static void smack_d_instantiate(struct dentry
>>> *opt_dentry, struct inode *inode)
>>> */
>>> isp->smk_inode = smk_of_current();
>>> break;
>>> + case PIPEFS_MAGIC:
>>> + isp->smk_inode = smk_of_current();
>>> + break;
>> We have the equivalent of this in SELinux already, by way of specifying
>> fs_use_task for pipefs in policy and the SECURITY_FS_USE_TASK case in
>> selinux inode_doinit_with_dentry().
>
> This looks like a fix regardless of the file_receive issue.
>
>>
>>> default:
>>> isp->smk_inode = sbsp->smk_root;
>>> break;
>>>
>>> 2) for sockets it's a complicated issue with connected/unconnected sockets,
>>> also connected means they might be server/client sockets (those with
>>> accept/listen and those with only connect), socketpair is another issue. I
>>> tried to cover at least one case, where the descriptor is a connected UDS
>>> socket (it has a peer), patch below:
>> I don't understand the issue here; for SELinux, we just always call
>> file_has_perm() in selinux_file_receive(), and it does not matter
>> whether the open file refers to a file, a pipe, a socket, or whatever.
>
> There's a difference in how SELinux and Smack treat socket based
> communications. I won't try to describe exactly how SELinux does
> it, because I always get it wrong. Smack treats the communication
> as a write operation from the sending process to the receiving
> process. The information about Smack label of the receiving process
> is stored in the socket structure (which is not itself an object in
> the Smack model) and the check is made against that. The Smack label
> attached to a UDS inode is not relevant. If the file structure for
> a UDS lacks an inode it shouldn't matter at all to Smack.
>
> For file_receive of a UDS what matters is whether the receiving
> process can write to the process at the other end. That has nothing
> to do with the inode.
>
>
>> If the open file refers to a socket, whether connected, unconnected,
>> local, network, or whatever, that socket has an associated inode that
>> was created when the socket was created (and that typically has the same
>> label as the process that created the socket), and I can check whether
>> the current process (the recipient) is allowed to read and write to
>> sockets with that label.
>
> But Smack doesn't really care about the inode. The UDS file system
> object is not interesting to Smack.
>
>> If you want to permit that, then you can allow it via a rule. If not,
>> then don't. What's the problem? You don't need to replicate the actual
>> checking that gets done on socket send at this point. You are just
>> checking whether the recipient can use the socket at all, not whether it
>> can talk to the peer. That should get mediated at send time.
>
> The check is to see if using the descriptor as is would violate
> the system policy. If the receiving process does not have write
> access to the process on the far end it shouldn't be using the
> descriptor. Unless, of course, the intent is to pass access to
> the receiving process that it shouldn't have. Fred being able
> to write to Wilma and Betty shouldn't allow Fred to give Wilma
> the right to write to Betty.
>
>>
>>> diff --git a/security/smack/smack.h b/security/smack/smack.h
>>> index 67ccb7b..7b1fbad 100644
>>> --- a/security/smack/smack.h
>>> +++ b/security/smack/smack.h
>>> @@ -79,9 +79,11 @@ struct superblock_smack {
>>> };
>>>
>>> struct socket_smack {
>>> - struct smack_known *smk_out; /* outbound label */
>>> - struct smack_known *smk_in; /* inbound label */
>>> - struct smack_known *smk_packet; /* TCP peer label */
>>> + struct smack_known *smk_out; /* outbound label */
>>> + struct smack_known *smk_in; /* inbound label */
>>> + struct smack_known *smk_packet; /* TCP peer label */
>>> + struct smack_known *smk_inode_out;
>>> + struct smack_known *smk_inode_in;
>>> };
>
> This does not look at all right to me. You should never need to
> look at the inode for a UDS.
>
>>>
>>> /*
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index db46ddb..4d55b6b 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -34,6 +34,7 @@
>>> #include <net/cipso_ipv4.h>
>>> #include <net/ip.h>
>>> #include <net/ipv6.h>
>>> +#include <net/af_unix.h>
>>> #include <linux/audit.h>
>>> #include <linux/magic.h>
>>> #include <linux/dcache.h>
>>> @@ -202,6 +203,73 @@ static int smk_bu_credfile(const struct cred *cred,
>>> struct file *file,
>>> #endif
>>>
>>> /**
>>> + * Helper function when a descriptor passed in a socket
>>> + * is a socket
>>> + */
>>> +static int smk_socket_receive(struct socket *socket, struct smk_audit_info
>>> *ad)
>>> +{
>>> + int rc = -EACCES;
>>> + struct socket_smack *ssk;
>>> + struct socket_smack *psk = NULL;
>>> + struct sock *sk = socket->sk;
>>> + struct sock *peer_sock;
>>> + struct task_smack *tsp;
>>> +
>>> + lock_sock(socket->sk);
>>> +
>>> + /*
>>> + * Check if this is a AF_UNIX connected socket
>>> + */
>>> + if (socket->state == SS_CONNECTED && sk->sk_family == PF_LOCAL) {
>>> + peer_sock = unix_peer_get(socket->sk);
>>> +
>>> + if (peer_sock != NULL) {
>>> + psk = peer_sock->sk_security;
>>> + if (psk->smk_inode_in != NULL
>>> + && psk->smk_inode_out != NULL) {
>>> + ssk = psk;
>>> + } else {
>>> + ssk = socket->sk->sk_security;
>>> + }
>>> + } else {
>>> + /*
>>> + * This should never happen
>>> + * a connected socket without
>>> + * a peer ??
>>> + */
>>> + release_sock(socket->sk);
>>> + return -EACCES;
>>> + }
>>> +
>>> + if (!smack_privileged(CAP_MAC_OVERRIDE)) {
>>> + tsp = current_security();
>>> +
>>> + rc = smk_access(tsp->smk_task,
>>> + ssk->smk_inode_in,
>>> + MAY_WRITE, ad);
>>> + rc = smk_bu_note("UDS fd receive",
>>> + tsp->smk_task,
>>> + ssk->smk_inode_in,
>>> + MAY_WRITE, rc);
>>> +
>>> + if (rc == 0) {
>>> + rc = smk_access(ssk->smk_inode_out,
>>> + tsp->smk_task,
>>> + MAY_WRITE, ad);
>>> + rc = smk_bu_note("UDS fd receive",
>>> + ssk->smk_inode_out,
>>> + tsp->smk_task,
>>> + MAY_WRITE, rc);
>>> + }
>>> + }
>>> + }
>>> +
>>> + release_sock(socket->sk);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +/**
>>> * smk_fetch - Fetch the smack label from a file.
>>> * @name: type of the label (attribute)
>>> * @ip: a pointer to the inode
>>> @@ -1638,9 +1706,19 @@ static int smack_file_receive(struct file *file)
>>> int may = 0;
>>> struct smk_audit_info ad;
>>> struct inode *inode = file_inode(file);
>>> + struct socket *socket;
>>>
>>> smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
>>> smk_ad_setfield_u_fs_path(&ad, file->f_path);
>>> +
>>> + if (unlikely(S_ISSOCK(inode->i_mode))) {
>>> + socket = SOCKET_I(inode);
>>> +
>>> + if (socket != NULL)
>>> + return smk_socket_receive(socket, &ad);
>>> + else
>>> + return -EACCES;
>>> + }
>>> /*
>>> * This code relies on bitmasks.
>>> */
>>> @@ -2078,6 +2156,8 @@ static int smack_sk_alloc_security(struct sock *sk,
>>> int family, gfp_t gfp_flags)
>>> ssp->smk_in = skp;
>>> ssp->smk_out = skp;
>>> ssp->smk_packet = NULL;
>>> + ssp->smk_inode_out = skp;
>>> + ssp->smk_inode_in = skp;
>>>
>>> sk->sk_security = ssp;
>>>
>>> @@ -3341,13 +3421,14 @@ static int smack_unix_stream_connect(struct sock
>>> *sock,
>>> MAY_WRITE, rc);
>>> }
>>> }
>>> -
>>> /*
>>> * Cross reference the peer labels for SO_PEERSEC.
>>> */
>>> if (rc == 0) {
>>> nsp->smk_packet = ssp->smk_out;
>>> ssp->smk_packet = osp->smk_out;
>>> + nsp->smk_inode_in = osp->smk_in;
>>> + nsp->smk_inode_out = osp->smk_out;
>>> }
>>>
>>> return rc;
>>>
>>> 3) We don't know what to do with unconnected sockets, we don't know what to
>>> do with network sockets (foreign peer on different machines), we don't know
>>> what to do if the descriptor is passed between multiple processes, and
>>> might end back in the originating process
>>>
>>> I can send those patches to the list if they look ok, however they only
>>> cover those two cases mentioned above.
>>>
>>> best regards
>>> Roman Kubiak
>>>
>>> On 09/18/2015 06:46 PM, Stephen Smalley wrote:
>>>> On 09/18/2015 12:15 PM, Casey Schaufler wrote:
>>>>> On 9/17/2015 6:38 AM, Stephen Smalley wrote:
>>>>>> On 09/16/2015 05:14 PM, Casey Schaufler wrote:
>>>>>>> On 9/9/2015 3:46 AM, Roman Kubiak wrote:
>>>>>>>> Helo (sorry for the long topic but it's hard to describe the problem
>>>>>>>> in short).
>>>>>>>>
>>>>>>>> The situation is as follows
>>>>>>>>
>>>>>>>> - create a socket connection between two processes (unix socket)
>>>>>>>> - one of the processes creates a socketpair
>>>>>>>> - this process sends one of the descriptors created by socketpair
>>>>>>>> to the other process
>>>>>>>> - the socket gets received by the other process
>>>>>>>>
>>>>>>>> Now the problem is that we can't label the sockets as if they were
>>>>>>>> connected,
>>>>>>>> connect() is never called on them, and the proper LSM functios don't
>>>>>>>> get called.
>>>>>>>> So when the LSM hook for file_receive gets called, all the security
>>>>>>>> fields in
>>>>>>>> the sockets are NULL.
>>>>>>> The problem here for Smack, which I assume is the LSM in question,
>>>>>>> is that the receive check is being done against the file_inode()
>>>>>>> of the file, and that makes no sense at all for the spawn of socketpair.
>>>>>>> This came about as part of a global change to the use of the f_security
>>>>>>> blob in Smack. The smack_file_receive check was changed to use the
>>>>>>> file_inode(), and shouldn't have been. The fix is:
>>>>>>>
>>>>>>> replace:
>>>>>>> struct inode *inode = file_inode(file);
>>>>>>> with:
>>>>>>> struct smack_known *skp = (struct smack_known
>>>>>>> *)file->f_security;
>>>>>>>
>>>>>>> replace:
>>>>>>> rc = smk_curacc(smk_of_inode(inode), may, &ad);
>>>>>>> with:
>>>>>>> rc = smk_curacc(skp, may, &ad);
>>>>>>>
>>>>>>> Give that a try and if you like the results, submit a patch.
>>>>>>> If you don't like the results, let me know why.
>>>>>> With that change, you'll be checking read/write to the label of the
>>>>>> process that opened the file, not to the label of the file (inode)
>>>>>> itself. I doubt that is what you want to do.
>>>>> You're correct for the case where "file" refers to a file.
>>>>> The suggested change should only apply to the case where "file"
>>>>> refers to UDS. A correct patch has to take the difference into
>>>>> account.
>>>>>
>>>>> I'll blame my error on jet lag this once.
>>>> It should make no difference; your current smack_file_receive() logic
>>>> should work as is for both regular files and UDS.
>>>>
>>>> The only issue here AFAICS is SO_PEERSEC support on socketpair()-created
>>>> sockets.
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-security-module" in
>>>> the body of a message to majord...@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-security-module" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
--
--------------
Roman Kubiak
--------------
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
