[patch][trusty] Revert "UBUNTU: SAUCE: apparmor: fix unix domain sockets to be mediated on connection"

[Tim Gardner]

On 01/28/2014 11:25 AM, John Johansen wrote:
> On 01/28/2014 03:08 AM, Tim Gardner wrote:
>> On 01/28/2014 10:42 AM, John Johansen wrote:
>>> This reverts commit 059c1f0963799ae6ac778863a82ba117e8041b54.
>>>
>>> http://bugs.launchpad.net/bugs/1270215
>>>
>>> Precise policy was not setup to deal with mediation of unix
>>> domain sockets at connection, as such this patch causes policy
>>> failures on precise. This bug could be fixed by updating policy
>>> but that would still cause custom policy to break, so as with
>>> lts-saucy this feature should be removed for lts-trusty on
>>> precise.
>>>
>>> Signed-off-by: John Johansen <john.johansen at canonical.com> ---
>>> security/apparmor/lsm.c | 48
>>> ++++++++++++------------------------------------ 1 file changed,
>>> 12 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>> index b83e92b..b320317 100644 --- a/security/apparmor/lsm.c +++
>>> b/security/apparmor/lsm.c @@ -787,29 +787,10 @@ do { \ //
>>> sk->sk_socket is NULL when orphaned/being shutdown // socket->sk
>>> set on graft, and sock_init_data if (socket exists)
>>>
>>> -#define UNIX_ANONYMOUS(U) (!unix_sk(U)->addr) -#define
>>> UNIX_FS(U) (!UNIX_ANONYMOUS(U) &&
>>> unix_sk(U)->addr->name->sun_path[0]) - -static int
>>> unix_fs_perm(int op, struct aa_label *label, struct sock *sk, -
>>> u32 mask) -{ -    if (!unconfined(label) && UNIX_FS(sk)) { -
>>> struct unix_sock *u = unix_sk(sk); - -        /* the sunpath may
>>> not be valid for this ns so use the path */ -        struct
>>> path_cond cond = { u->path.dentry->d_inode->i_uid, -
>>> u->path.dentry->d_inode->i_mode -        }; - -        return
>>> aa_path_perm(op, label, &u->path, 0, mask, &cond); -    } -
>>> return 0; -} - /** * apparmor_unix_stream_connect - check perms
>>> before making unix domain conn * - * other is locked when this
>>> hook is called + * only used for alt unix socket namespace ???
>>> */ static int apparmor_unix_stream_connect(struct sock *sock,
>>> struct sock *other, struct sock *newsk) @@ -817,16 +798,16 @@
>>> static int apparmor_unix_stream_connect(struct sock *sock, struct
>>> sock *other, struct aa_sk_cxt *sock_cxt = SK_CXT(sock); struct
>>> aa_sk_cxt *other_cxt = SK_CXT(other); struct aa_sk_cxt *new_cxt =
>>> SK_CXT(newsk); -    struct aa_label *label =
>>> __aa_get_current_label();
>>>
>>> -    int error = unix_fs_perm(OP_CONNECT, label, other, -
>>> MAY_READ | MAY_WRITE); -    __aa_put_current_label(label);
>>>
>>> -    if (error) +#if 0 +    if (!perms to connect sock to other)
>>> + return error; +#endif
>>>
>>> -    /* Cross reference the peer labels for SO_PEERSEC */ +// ???
>>> label not updated after connection??? it would be good if the
>>> label +// was updated as the task labeling is updated if
>>> (new_cxt->peer) { //printk("%s: new_cxt->peer\n", __FUNCTION__);
>>> aa_put_label(new_cxt->peer); @@ -849,21 +830,16 @@ static int
>>> apparmor_unix_stream_connect(struct sock *sock, struct sock
>>> *other, /** * apparmor_unix_may_send - check perms before conn or
>>> sending unix dgrams * - * other is locked when this hook is
>>> called + * Only used for alt unix socket namespace ???? */ static
>>> int apparmor_unix_may_send(struct socket *sock, struct socket
>>> *other) { -    struct aa_sk_cxt *other_cxt = SK_CXT(other->sk); -
>>> struct aa_label *label = __aa_get_current_label(); -    int e,
>>> error ; +  //  ??? how do these play in with regular perm checks,
>>> conditional?
>>>
>>> -    error = unix_fs_perm(OP_SENDMSG, label, other->sk,
>>> MAY_WRITE); -    e = unix_fs_perm(OP_SENDMSG, other_cxt->label,
>>> sock->sk, MAY_READ); -    if (e) -        error = e; -
>>> __aa_put_current_label(label); +//    print_sk(sock->sk); +//
>>> print_sk(other->sk);
>>>
>>> -    return error; +    return 0; }
>>>
>>> /**
>>>
>>
>> John - I assume this is really intended for application against the
>> LTS branch only ? Is there a simpler patch such as a config option
>> that is only turned on for the LTS version ? That way I don't have
>> to carry a big delta between mainline and the LTS rebase.
>>
> sorry yes, this is meant for lts-trust on precise
>
> There is not a config option to disable this in the code atm. For the
> next sync of apparmor to trusty I can include a config option to
> disable new features that are not detected via policy load.
>

I can wait until the next sync (as long as it is the next couple weeks). 
Please implement the config option to disable new features.

rtg
-- 
Tim Gardner tim.gardner at canonical.com