| Message ID | 52E7899C.4010009@canonical.com |
|---|---|
| State | New |
| Headers | show |
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@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. rtg
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@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.
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@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
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; } /**
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@canonical.com> --- security/apparmor/lsm.c | 48 ++++++++++++------------------------------------ 1 file changed, 12 insertions(+), 36 deletions(-)