Move attribute names to allocated strings

Unfortunately as our interfaces allow for importing and exporting the
gssntlmssp context via serialization we need to use allocated strings or
fixed size buffers.

In this case, given the attribute names are arbitrary strings I prefer
to allocate the names for ease of handling.
The bulk of the data for sids is in the values anyway so it is a small
price to pay to have a strdup() for the attribute name.

Signed-off-by: Simo Sorce <[email protected]>

Author: simo5

src/gss_names.c

@@ -306,8 +306,11 @@ int gssntlm_copy_attrs(const struct gssntlm_name_attribute *src,
     }
 
     for (size_t i = 0; i < attrs_count; i++) {
-        /* read-only persistent data ptr can be just copied */
-        copied_attrs[i].attr_name = src[i].attr_name;
+        copied_attrs[i].attr_name = strdup(src[i].attr_name);
+        if (copied_attrs[i].attr_name == NULL) {
+            gssntlm_release_attrs(&copied_attrs);
+            return ENOMEM;
+        }
         copied_attrs[i].attr_value.length = src[i].attr_value.length;
         copied_attrs[i].attr_value.value = malloc(src[i].attr_value.length);
         if (copied_attrs[i].attr_value.value == NULL) {
@@ -323,69 +326,13 @@ int gssntlm_copy_attrs(const struct gssntlm_name_attribute *src,
     return 0;
 }
 
-/* Low-level func with buffer ownership transfer
- * attr_name - read-only static string, should not be released
- * attr_value - ownership of that buffer is MOVED to dst
- *              (MUST NOT BE RELEASED from caller side) */
-int gssntlm_append_attr(const char *attr_name, gss_buffer_t attr_value,
-                        struct gssntlm_name *dst)
-{
-    size_t prev_attrs_count = gssntlm_get_attrs_count(dst->attrs);
-    /* 1 for new attribute +1 for terminator entry */
-    size_t new_attrs_count = prev_attrs_count + 2;
-    struct gssntlm_name_attribute *attrs;
-
-    if (!attr_name || !attr_value || !dst) {
-        return ERR_NOARG;
-    }
-
-    /* Increase buffer - if there was no any attributes before,
-     * realloc is identical to malloc */
-    attrs = realloc(dst->attrs,
-                    new_attrs_count * sizeof(struct gssntlm_name_attribute));
-    if (attrs == NULL) {
-        return ENOMEM;
-    }
-    dst->attrs = attrs;
-
-    attrs[prev_attrs_count].attr_name = attr_name;
-    attrs[prev_attrs_count].attr_value.value = attr_value->value;
-    attrs[prev_attrs_count].attr_value.length = attr_value->length;
-
-    /* Fill the last terminator entry with zeroes
-       beacuse realloc does not init added memory */
-    memset(&attrs[prev_attrs_count + 1], 0, sizeof(struct gssntlm_name_attribute));
-
-    return 0;
-}
-
-int gssntlm_append_attr_str(const char *attr_name, const char *attr_value,
-                            struct gssntlm_name *dst)
-{
-    int ret;
-    gss_buffer_desc buf;
-    if (!attr_value) {
-        return ERR_NOARG;
-    }
-    buf.value = strdup(attr_value);
-    if (!buf.value) {
-        return ENOMEM;
-    }
-    buf.length = strlen(attr_value) + 1; /* +1 for EOL */
-    ret = gssntlm_append_attr(attr_name, &buf, dst);
-    if (ret) {
-        free(buf.value);
-    }
-    return ret;
-}
-
 struct gssntlm_name_attribute *gssntlm_find_attr(
                                         struct gssntlm_name_attribute *attrs,
                                         const char *attr_name,
                                         size_t attr_name_len)
 {
     for (size_t i = 0; attrs && (attrs[i].attr_name != NULL); i++) {
-        /* We store attr_name as const static zero-terminated string, so
+        /* We store attr_name as a zero-terminated string, so
          * it is always zero-terminated */
         if (attr_name_len == strlen(attrs[i].attr_name) &&
             strncasecmp(attrs[i].attr_name, attr_name, attr_name_len) == 0) {
@@ -398,6 +345,7 @@ struct gssntlm_name_attribute *gssntlm_find_attr(
 void gssntlm_release_attrs(struct gssntlm_name_attribute **attrs)
 {
     for (size_t i = 0; *attrs && (*attrs)[i].attr_name != NULL; i++) {
+        free((*attrs)[i].attr_name);
         free((*attrs)[i].attr_value.value);
     }
     safefree(*attrs);

src/gss_ntlmssp.h

@@ -41,9 +41,7 @@
 #define NTLMSSP_CTX_FLAG_AUTH_WITH_MIC  0x04 /* Auth MIC was created */
 
 struct gssntlm_name_attribute {
-    const char *attr_name; /* Read-only static string, should not be released */
-                           /* NULL is used to indicate         */
-                           /* the terminating element of array */
+    char *attr_name; /* NULL indicates array termination */
     gss_buffer_desc attr_value;
 };
 

src/winbind.c

@@ -216,24 +216,33 @@ static uint32_t format_sids_as_name_attribute(
                                     const struct wbcAuthUserInfo *wbc_info,
                                     struct gssntlm_name_attribute **auth_attrs)
 {
-    char *sids_buf, *sids_buf_realloced;
-    size_t offset = 0;
-    struct gssntlm_name_attribute *attrs;
     size_t worst_sids_list_len = WBC_SID_STRING_BUFLEN * wbc_info->num_sids;
+    struct gssntlm_name_attribute *attrs = NULL;
+    char *sids_buf_realloced;
+    char *sids_buf = NULL;
+    char *name = NULL;
+    size_t offset = 0;
+    int ret = EFAULT;
 
     /* Allocate buffers */
+    name = strdup(gssntlmssp_sids_urn);
+    if (name == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
 
     /* 1 for returned attribute +1 for termiator entry */
     attrs = calloc(2, sizeof(struct gssntlm_name_attribute));
     if (attrs == NULL) {
-        return ENOMEM;
+        ret = ENOMEM;
+        goto done;
     }
 
     /* sids buffer is allocated with the worst-case size */
     sids_buf = malloc(worst_sids_list_len);
     if (sids_buf == NULL) {
-        free(attrs);
-        return ENOMEM;
+        ret = ENOMEM;
+        goto done;
     }
 
     /* Construct name attributes string */
@@ -259,10 +268,20 @@ static uint32_t format_sids_as_name_attribute(
         sids_buf = sids_buf_realloced;
     }
 
-    attrs[0].attr_name = gssntlmssp_sids_urn;
+    ret = 0;
+
+done:
+    if (ret) {
+        free(name);
+        free(attrs);
+        free(sids_buf);
+        return ret;
+    }
+
+    attrs[0].attr_name = name;
     attrs[0].attr_value.length = offset;
     attrs[0].attr_value.value = sids_buf;
-    /* attrs[1] will be filled by zeros automatically by calloc */
+    /* attrs[1] was zeroed by calloc */
 
     *auth_attrs = attrs;
 

tests/ntlmssptest.c

@@ -2283,18 +2283,18 @@ static void generate_random_binary_blob(char *dst, size_t bytes_count)
 }
 
 /* Low-level func with buffer ownership transfer
- * attr_name - read-only static string, should not be released
- * attr_value - ownership of that buffer is MOVED to dst
- *              (MUST NOT BE RELEASED from caller side) */
-static int gssntlm_append_attr(const char *attr_name, gss_buffer_t attr_value,
-                               struct gssntlm_name *dst)
+ * name - allocated string with the name of the attribute
+ * value - allocated buffer with a value of size 'length'
+ */
+static int gssntlm_append_attr(const char *name, void *value,
+                               size_t length, struct gssntlm_name *dst)
 {
     size_t prev_attrs_count = gssntlm_get_attrs_count(dst->attrs);
     /* 1 for new attribute +1 for terminator entry */
     size_t new_attrs_count = prev_attrs_count + 2;
     struct gssntlm_name_attribute *attrs;
 
-    if (!attr_name || !attr_value || !dst) {
+    if (!name || !value || !length || !dst) {
         return ERR_NOARG;
     }
 
@@ -2307,36 +2307,22 @@ static int gssntlm_append_attr(const char *attr_name, gss_buffer_t attr_value,
     }
     dst->attrs = attrs;
 
-    attrs[prev_attrs_count].attr_name = attr_name;
-    attrs[prev_attrs_count].attr_value.value = attr_value->value;
-    attrs[prev_attrs_count].attr_value.length = attr_value->length;
-
-    /* Fill the last terminator entry with zeroes
-       beacuse realloc does not init added memory */
-    memset(&attrs[prev_attrs_count + 1], 0, sizeof(struct gssntlm_name_attribute));
-
-    return 0;
-}
-
-static int gssntlm_append_attr_str(const char *attr_name,
-                                   const char *attr_value,
-                                   struct gssntlm_name *dst)
-{
-    int ret;
-    gss_buffer_desc buf;
-    if (!attr_value) {
-        return ERR_NOARG;
-    }
-    buf.value = strdup(attr_value);
-    if (!buf.value) {
+    attrs[prev_attrs_count].attr_name = strdup(name);
+    if (attrs[prev_attrs_count].attr_name == NULL) {
         return ENOMEM;
     }
-    buf.length = strlen(attr_value) + 1; /* +1 for EOL */
-    ret = gssntlm_append_attr(attr_name, &buf, dst);
-    if (ret) {
-        free(buf.value);
+    attrs[prev_attrs_count].attr_value.value = malloc(length);
+    if (attrs[prev_attrs_count].attr_value.value == NULL) {
+        safefree(attrs[prev_attrs_count].attr_name);
+        return ENOMEM;
     }
-    return ret;
+    memcpy(attrs[prev_attrs_count].attr_value.value, value, length);
+    attrs[prev_attrs_count].attr_value.length = length;
+
+    /* terminate array */
+    attrs[prev_attrs_count + 1].attr_name = NULL;
+
+    return 0;
 }
 
 static int append_sids_list_attr(const char *urn, size_t min_count,
@@ -2352,26 +2338,20 @@ static int append_sids_list_attr(const char *urn, size_t min_count,
         return ENOMEM;
     }
     *length = generate_random_sids_list(str, min_count, max_count);
-    ret = gssntlm_append_attr_str(urn, str, name);
+    /* append zero terminated string for ease of string handling */
+    ret = gssntlm_append_attr(urn, str, strlen(str) + 1, name);
     free(str);
     return ret;
 }
 
-static int append_binary_attr(const char *urn, size_t bytes_count,
+static int append_binary_attr(const char *urn, size_t length,
                               struct gssntlm_name *name)
 {
+    char rndbuf[length];
     int ret;
-    gss_buffer_desc buf = { bytes_count, malloc(bytes_count) };
-    if (!buf.value) {
-        return ENOMEM;
-    }
-    generate_random_binary_blob(buf.value, bytes_count);
-    ret = gssntlm_append_attr(urn, &buf, name);
-    /* If append was ok, memory ownership is moved to name;
-       if failed, we need to release it here */
-    if (ret) {
-        free(buf.value);
-    }
+    generate_random_binary_blob(rndbuf, length);
+    ret = gssntlm_append_attr(urn, rndbuf, length, name);
+    free(rndbuf);
     return ret;
 }