xfrm: fix xfrm security context management

The data structure of choice when adding/processing a security context
for xfrm is struct xfrm(nl)_user_sec_ctx. The previous code did however
use the (also exported) struct xfrm(nl)_sec_ctx. While sizeof(struct
xfrm(nl)_*sec_ctx) yields the same result, the interpretation of one of
the data structures as the other one messes up the contents.
With this fix, the wrong data structure has been replaced with the
correct one. Also -- since the size of the context string is not known
-- one can now call xfrmnl_sa_get_sec_ctx with ctx_str being NULL, thus
retrieving the length of the context string.
A new capability has been introduced, to test whether libnl3 supports
the modified semantics of this function.

Signed-off-by: Thomas Egerer <thomas.egerer@secunet.com>

diff --git a/include/netlink-private/types.h b/include/netlink-private/types.h
index e80c8a14ddd5985ef3e1b53ea4b8f76c9228739d..e72c8630b7cc7df049df3c0cfa1583938dcf2e9b 100644 (file)
--- a/include/netlink-private/types.h
+++ b/include/netlink-private/types.h
@@ -1155,6 +1155,7 @@ struct xfrmnl_encap_tmpl {
        struct nl_addr* encap_oa;
 };
 
+/* kernel space structure, don't use to retrieve/pass sec ctx attribute! */
 struct xfrmnl_sec_ctx {
        uint8_t         ctx_doi;
        uint8_t         ctx_alg;
@@ -1186,7 +1187,7 @@ struct xfrmnl_sa {
        uint32_t                        tfcpad;
        struct nl_addr*                 coaddr;
        struct xfrmnl_mark              mark;
-       struct xfrmnl_sec_ctx*          sec_ctx;
+       struct xfrmnl_user_sec_ctx*     sec_ctx;
        uint32_t                        replay_maxage;
        uint32_t                        replay_maxdiff;
        struct xfrmnl_replay_state      replay_state;

diff --git a/include/netlink/utils.h b/include/netlink/utils.h
index 4e2a90ae240ed3f9f3697f7a42d5d9049db1884b..6501b0d1cd975038eaf508e0701a574468549508 100644 (file)
--- a/include/netlink/utils.h
+++ b/include/netlink/utils.h
@@ -231,6 +231,13 @@ enum {
        NL_CAPABILITY_NL_ADDR_FILL_SOCKADDR = 21,
 #define NL_CAPABILITY_NL_ADDR_FILL_SOCKADDR NL_CAPABILITY_NL_ADDR_FILL_SOCKADDR
 
+       /**
+        * Support omitting @ctx_str argument to xfrmnl_sa_get_sec_ctx() to check
+        * for required buffer size for context string.
+        */
+       NL_CAPABILITY_XFRM_SEC_CTX_LEN = 22,
+#define NL_CAPABILITY_XFRM_SEC_CTX_LEN NL_CAPABILITY_XFRM_SEC_CTX_LEN
+
        __NL_CAPABILITY_MAX,
        NL_CAPABILITY_MAX = (__NL_CAPABILITY_MAX - 1),
 #define NL_CAPABILITY_MAX NL_CAPABILITY_MAX

diff --git a/lib/utils.c b/lib/utils.c
index c1c1b7242019938cf32670930ec2bc036f691f43..7d339ae4cb5612c72aa4f44fbb267965e8439cf6 100644 (file)
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1190,7 +1190,7 @@ int nl_has_capability (int capability)
                        NL_CAPABILITY_VERSION_3_2_28,
                        NL_CAPABILITY_RTNL_ADDR_PEER_ID_FIX,
                        NL_CAPABILITY_NL_ADDR_FILL_SOCKADDR,
-                       0,
+                       NL_CAPABILITY_XFRM_SEC_CTX_LEN,
                        0,
                        0),
                /* IMPORTANT: these capability numbers are intended to be universal and stable

diff --git a/lib/xfrm/sa.c b/lib/xfrm/sa.c
index 9e2c353caad49a46cde3537f77b4d2009831a5fc..06a1e60ceee1db63d54bccb2be5852afddf4d684 100644 (file)
--- a/lib/xfrm/sa.c
+++ b/lib/xfrm/sa.c
@@ -191,7 +191,7 @@ static int xfrm_sa_clone(struct nl_object *_dst, struct nl_object *_src)
 
        if (src->sec_ctx)
        {
-               len = sizeof (struct xfrmnl_sec_ctx) + src->sec_ctx->ctx_len;
+               len = sizeof (*src->sec_ctx) + src->sec_ctx->ctx_len;
                if ((dst->sec_ctx = calloc (1, len)) == NULL)
                        return -NLE_NOMEM;
                memcpy ((void *)dst->sec_ctx, (void *)src->sec_ctx, len);
@@ -257,8 +257,7 @@ static uint64_t xfrm_sa_compare(struct nl_object *_a, struct nl_object *_b,
        diff |= XFRM_SA_DIFF(SECCTX,((a->sec_ctx->ctx_doi != b->sec_ctx->ctx_doi) ||
                                    (a->sec_ctx->ctx_alg != b->sec_ctx->ctx_alg) ||
                                    (a->sec_ctx->ctx_len != b->sec_ctx->ctx_len) ||
-                                   (a->sec_ctx->ctx_sid != b->sec_ctx->ctx_sid) ||
-                                   strcmp(a->sec_ctx->ctx_str, b->sec_ctx->ctx_str)));
+                                   strcmp(a->sec_ctx->ctx, b->sec_ctx->ctx)));
        diff |= XFRM_SA_DIFF(REPLAY_MAXAGE,a->replay_maxage != b->replay_maxage);
        diff |= XFRM_SA_DIFF(REPLAY_MAXDIFF,a->replay_maxdiff != b->replay_maxdiff);
        diff |= XFRM_SA_DIFF(EXPIRE,a->hard != b->hard);
@@ -504,8 +503,8 @@ static void xfrm_sa_dump_line(struct nl_object *a, struct nl_dump_params *p)
                nl_dump_line(p, "\tMark mask: 0x%x Mark value: 0x%x\n", sa->mark.m, sa->mark.v);
 
        if (sa->ce_mask & XFRM_SA_ATTR_SECCTX)
-               nl_dump_line(p, "\tDOI: %d Algo: %d Len: %u SID: %u ctx: %s\n", sa->sec_ctx->ctx_doi,
-                            sa->sec_ctx->ctx_alg, sa->sec_ctx->ctx_len, sa->sec_ctx->ctx_sid, sa->sec_ctx->ctx_str);
+               nl_dump_line(p, "\tDOI: %d Algo: %d Len: %u ctx: %s\n", sa->sec_ctx->ctx_doi,
+                            sa->sec_ctx->ctx_alg, sa->sec_ctx->ctx_len, sa->sec_ctx->ctx);
 
        nl_dump_line(p, "\treplay info: \n");
        nl_dump_line(p, "\t\tmax age %u max diff %u \n", sa->replay_maxage, sa->replay_maxdiff);
@@ -868,14 +867,14 @@ int xfrmnl_sa_parse(struct nlmsghdr *n, struct xfrmnl_sa **result)
        }
 
        if (tb[XFRMA_SEC_CTX]) {
-               struct xfrm_sec_ctx* sec_ctx = nla_data(tb[XFRMA_SEC_CTX]);
-               len = sizeof (struct xfrmnl_sec_ctx) + sec_ctx->ctx_len;
+               struct xfrm_user_sec_ctx* sec_ctx = nla_data(tb[XFRMA_SEC_CTX]);
+               len = sizeof (struct xfrmnl_user_sec_ctx) + sec_ctx->ctx_len;
                if ((sa->sec_ctx = calloc (1, len)) == NULL)
                {
                        err = -NLE_NOMEM;
                        goto errout;
                }
-               memcpy ((void *)sa->sec_ctx, (void *)sec_ctx, len);
+               memcpy (sa->sec_ctx, sec_ctx, len);
                sa->ce_mask     |= XFRM_SA_ATTR_SECCTX;
        }
 
@@ -1938,15 +1937,38 @@ int xfrmnl_sa_set_mark (struct xfrmnl_sa* sa, unsigned int value, unsigned int m
        return 0;
 }
 
-int xfrmnl_sa_get_sec_ctx (struct xfrmnl_sa* sa, unsigned int* doi, unsigned int* alg, unsigned int* len, unsigned int* sid, char* ctx_str)
+/**
+ * Get the security context.
+ *
+ * @arg sa              The xfrmnl_sa object.
+ * @arg doi             An optional output value for the security context domain of interpretation.
+ * @arg alg             An optional output value for the security context algorithm.
+ * @arg len             An optional output value for the security context length, including the
+ *                      terminating null byte ('\0').
+ * @arg sid             Unused parameter.
+ * @arg ctx_str         An optional buffer large enough for the security context string. It must
+ *                      contain at least @len bytes.
+ *
+ * Warning: you must ensure that @ctx_str is large enough. If you don't know the length before-hand,
+ * call xfrmnl_sa_get_sec_ctx() without @ctx_str argument to query only the required buffer size.
+ * This modified API is available in all versions of libnl3 that support the capability
+ * @def NL_CAPABILITY_XFRM_SEC_CTX_LEN (@see nl_has_capability for further information).
+ *
+ * @return 0 on success or a negative error code.
+ */
+int xfrmnl_sa_get_sec_ctx (struct xfrmnl_sa* sa, unsigned int* doi, unsigned int* alg,
+               unsigned int* len, unsigned int* sid, char* ctx_str)
 {
        if (sa->ce_mask & XFRM_SA_ATTR_SECCTX)
        {
-               *doi    =   sa->sec_ctx->ctx_doi;
-               *alg    =   sa->sec_ctx->ctx_alg;
-               *len    =   sa->sec_ctx->ctx_len;
-               *sid    =   sa->sec_ctx->ctx_sid;
-               memcpy ((void *)ctx_str, (void *)sa->sec_ctx->ctx_str, sizeof (uint8_t) * sa->sec_ctx->ctx_len);
+               if (doi)
+                       *doi = sa->sec_ctx->ctx_doi;
+               if (alg)
+                       *alg = sa->sec_ctx->ctx_alg;
+               if (len)
+                       *len = sa->sec_ctx->ctx_len;
+               if (ctx_str)
+                       memcpy (ctx_str, sa->sec_ctx->ctx, sa->sec_ctx->ctx_len);
        }
        else
                return -1;
@@ -1954,20 +1976,35 @@ int xfrmnl_sa_get_sec_ctx (struct xfrmnl_sa* sa, unsigned int* doi, unsigned int
        return 0;
 }
 
-int xfrmnl_sa_set_sec_ctx (struct xfrmnl_sa* sa, unsigned int doi, unsigned int alg, unsigned int len, unsigned int sid, const char* ctx_str)
+/**
+ * Set the security context.
+ *
+ * @arg sa              The xfrmnl_sa object.
+ * @arg doi             Parameter for the security context domain of interpretation.
+ * @arg alg             Parameter for the security context algorithm.
+ * @arg len             Parameter for the length of the security context string containing
+ *                      the terminating null byte ('\0').
+ * @arg sid             Unused parameter.
+ * @arg ctx_str         Buffer containing the security context string.
+ *
+ * @return 0 on success or a negative error code.
+ */
+int xfrmnl_sa_set_sec_ctx (struct xfrmnl_sa* sa, unsigned int doi, unsigned int alg, unsigned int len,
+               unsigned int sid, const char* ctx_str)
 {
        /* Free up the old context string and allocate new one */
        if (sa->sec_ctx)
                free (sa->sec_ctx);
-       if ((sa->sec_ctx = calloc (1, sizeof (struct xfrmnl_sec_ctx) + (sizeof (uint8_t) * len))) == NULL)
+       if ((sa->sec_ctx = calloc(1, sizeof (struct xfrmnl_user_sec_ctx) + len)) == NULL)
                return -1;
 
        /* Save the new info */
-       sa->sec_ctx->ctx_doi    =   doi;
+       sa->sec_ctx->len        =   sizeof(struct xfrmnl_user_sec_ctx) + len;
+       sa->sec_ctx->exttype    =   XFRMA_SEC_CTX;
        sa->sec_ctx->ctx_alg    =   alg;
+       sa->sec_ctx->ctx_doi    =   doi;
        sa->sec_ctx->ctx_len    =   len;
-       sa->sec_ctx->ctx_sid    =   sid;
-       memcpy (sa->sec_ctx->ctx_str, ctx_str, sizeof (uint8_t) * len);
+       memcpy (sa->sec_ctx->ctx, ctx_str, len);
 
        sa->ce_mask |= XFRM_SA_ATTR_SECCTX;