Commit 2d151d39073a ("xfrm: Add possibility to set the default to block
if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING
enum item, thus also evading the build-time check
in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper
security permission checks in nlmsg_xfrm_perms. Fix it by placing
XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before
__XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly.
Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy")
References: https://lore.kernel.org/netdev/[email protected]/
Signed-off-by: Eugene Syromiatnikov <[email protected]>
---
v2:
- Updated SELinux nlmsg_xfrm_perms permissions table and selinux_nlmsg_lookup
build-time check accordingly.
v1: https://lore.kernel.org/lkml/[email protected]/
---
include/uapi/linux/xfrm.h | 6 +++---
security/selinux/nlmsgtab.c | 4 +++-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index b96c1ea..26f456b1 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -213,13 +213,13 @@ enum {
XFRM_MSG_GETSPDINFO,
#define XFRM_MSG_GETSPDINFO XFRM_MSG_GETSPDINFO
+ XFRM_MSG_MAPPING,
+#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING
+
XFRM_MSG_SETDEFAULT,
#define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT
XFRM_MSG_GETDEFAULT,
#define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT
-
- XFRM_MSG_MAPPING,
-#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING
__XFRM_MSG_MAX
};
#define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1)
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index d59276f..94ea2a8 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -126,6 +126,8 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] =
{ XFRM_MSG_NEWSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
{ XFRM_MSG_GETSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_READ },
{ XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ },
+ { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
+ { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ },
};
static const struct nlmsg_perm nlmsg_audit_perms[] =
@@ -189,7 +191,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
* structures at the top of this file with the new mappings
* before updating the BUILD_BUG_ON() macro!
*/
- BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING);
+ BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT);
err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms,
sizeof(nlmsg_xfrm_perms));
break;
--
2.1.4
Thanks!
Acked-by: Antony Antony <[email protected]>
-antony
On Sun, Sep 12, 2021 at 14:22:34 +0200, Eugene Syromiatnikov wrote:
> Commit 2d151d39073a ("xfrm: Add possibility to set the default to block
> if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING
> enum item, thus also evading the build-time check
> in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper
> security permission checks in nlmsg_xfrm_perms. Fix it by placing
> XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before
> __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly.
>
> Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy")
> References: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Eugene Syromiatnikov <[email protected]>
> ---
> v2:
> - Updated SELinux nlmsg_xfrm_perms permissions table and selinux_nlmsg_lookup
> build-time check accordingly.
>
> v1: https://lore.kernel.org/lkml/[email protected]/
> ---
> include/uapi/linux/xfrm.h | 6 +++---
> security/selinux/nlmsgtab.c | 4 +++-
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index b96c1ea..26f456b1 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -213,13 +213,13 @@ enum {
> XFRM_MSG_GETSPDINFO,
> #define XFRM_MSG_GETSPDINFO XFRM_MSG_GETSPDINFO
>
> + XFRM_MSG_MAPPING,
> +#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING
> +
> XFRM_MSG_SETDEFAULT,
> #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT
> XFRM_MSG_GETDEFAULT,
> #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT
> -
> - XFRM_MSG_MAPPING,
> -#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING
> __XFRM_MSG_MAX
> };
> #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1)
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index d59276f..94ea2a8 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -126,6 +126,8 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] =
> { XFRM_MSG_NEWSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
> { XFRM_MSG_GETSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_READ },
> { XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ },
> + { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
> + { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ },
> };
>
> static const struct nlmsg_perm nlmsg_audit_perms[] =
> @@ -189,7 +191,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
> * structures at the top of this file with the new mappings
> * before updating the BUILD_BUG_ON() macro!
> */
> - BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING);
> + BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT);
> err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms,
> sizeof(nlmsg_xfrm_perms));
> break;
> --
> 2.1.4
>
Hi,
On Sun, Sep 12, 2021 at 2:23 PM Eugene Syromiatnikov <[email protected]> wrote:
> Commit 2d151d39073a ("xfrm: Add possibility to set the default to block
> if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING
> enum item, thus also evading the build-time check
> in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper
> security permission checks in nlmsg_xfrm_perms. Fix it by placing
> XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before
> __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly.
>
> Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy")
> References: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Eugene Syromiatnikov <[email protected]>
> ---
> v2:
> - Updated SELinux nlmsg_xfrm_perms permissions table and selinux_nlmsg_lookup
> build-time check accordingly.
>
> v1: https://lore.kernel.org/lkml/[email protected]/
> ---
> include/uapi/linux/xfrm.h | 6 +++---
> security/selinux/nlmsgtab.c | 4 +++-
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index b96c1ea..26f456b1 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -213,13 +213,13 @@ enum {
> XFRM_MSG_GETSPDINFO,
> #define XFRM_MSG_GETSPDINFO XFRM_MSG_GETSPDINFO
>
> + XFRM_MSG_MAPPING,
> +#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING
> +
> XFRM_MSG_SETDEFAULT,
> #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT
> XFRM_MSG_GETDEFAULT,
> #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT
> -
> - XFRM_MSG_MAPPING,
> -#define XFRM_MSG_MAPPING XFRM_MSG_MAPPING
Perhaps it would be a good idea to put a comment here to make it less
likely that this repeats in the future. Something like:
/* IMPORTANT: Only insert new entries right above this line, otherwise
you break ABI! */
> __XFRM_MSG_MAX
> };
> #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1)
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index d59276f..94ea2a8 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -126,6 +126,8 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] =
> { XFRM_MSG_NEWSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
> { XFRM_MSG_GETSPDINFO, NETLINK_XFRM_SOCKET__NLMSG_READ },
> { XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ },
> + { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
> + { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ },
> };
>
> static const struct nlmsg_perm nlmsg_audit_perms[] =
> @@ -189,7 +191,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
> * structures at the top of this file with the new mappings
> * before updating the BUILD_BUG_ON() macro!
> */
> - BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MAPPING);
> + BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT);
> err = nlmsg_perm(nlmsg_type, perm, nlmsg_xfrm_perms,
> sizeof(nlmsg_xfrm_perms));
> break;
> --
> 2.1.4
>
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
On Mon, Sep 13, 2021 at 09:16:39AM +0200, Ondrej Mosnacek wrote:
> Perhaps it would be a good idea to put a comment here to make it less
> likely that this repeats in the future. Something like:
>
> /* IMPORTANT: Only insert new entries right above this line, otherwise
> you break ABI! */
Well, this statement is true for (almost) every UAPI-exposed enum, and
netlink is vast and relies on enums heavily. I think it is already
mentioned somewhere in the documentation, and in the end it falls on the
shoulders of the maintainers—to pay additional attention to UAPI changes.
On Mon, Sep 13, 2021 at 12:23 PM Eugene Syromiatnikov <[email protected]> wrote:
> On Mon, Sep 13, 2021 at 09:16:39AM +0200, Ondrej Mosnacek wrote:
> > Perhaps it would be a good idea to put a comment here to make it less
> > likely that this repeats in the future. Something like:
> >
> > /* IMPORTANT: Only insert new entries right above this line, otherwise
> > you break ABI! */
>
> Well, this statement is true for (almost) every UAPI-exposed enum, and
> netlink is vast and relies on enums heavily. I think it is already
> mentioned somewhere in the documentation, and in the end it falls on the
> shoulders of the maintainers—to pay additional attention to UAPI changes.
Ok, fair enough.
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
Le 12/09/2021 à 14:22, Eugene Syromiatnikov a écrit :
> Commit 2d151d39073a ("xfrm: Add possibility to set the default to block
> if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING
> enum item, thus also evading the build-time check
> in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper
> security permission checks in nlmsg_xfrm_perms. Fix it by placing
> XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before
> __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly.
>
> Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy")
> References: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Eugene Syromiatnikov <[email protected]>
Acked-by: Nicolas Dichtel <[email protected]>
On Sun, Sep 12, 2021 at 02:22:34PM +0200, Eugene Syromiatnikov wrote:
> Commit 2d151d39073a ("xfrm: Add possibility to set the default to block
> if we have no policy") broke ABI by changing the value of the XFRM_MSG_MAPPING
> enum item, thus also evading the build-time check
> in security/selinux/nlmsgtab.c:selinux_nlmsg_lookup for presence of proper
> security permission checks in nlmsg_xfrm_perms. Fix it by placing
> XFRM_MSG_SETDEFAULT/XFRM_MSG_GETDEFAULT to the end of the enum, right before
> __XFRM_MSG_MAX, and updating the nlmsg_xfrm_perms accordingly.
>
> Fixes: 2d151d39073a ("xfrm: Add possibility to set the default to block if we have no policy")
> References: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Eugene Syromiatnikov <[email protected]>
Applied, thanks a lot Eugene!