Daniel Stenberg wrote:
First, in the case of a well-formed URL, ludp->lud_attrs_dup is not
initialized prior calling
ludp->lud_attrs_dup[i] = curl_easy_unescape(data, ludp->lud_attrs[i],
0, NULL);
inside unescape_elements().
The struct is allocated with calloc() in _ldap_url_parse(), is it not?
Yes. When the struct is allocated using calloc(), that allocates space
for a pointer and sets the pointer to 0.
Nothing allocates memory for the array, however, so the pointer
lud_attrs_dup is still 0x00 when the code above uses the [] operator.
That causes an attempt to dereference 0. I confirmed that this is
happening and causes a crash.
Second, in the case of a malformed URL, this loop:
+ for(i = 0; ludp->lud_attrs_dup[i]; i++)
+ free(ludp->lud_attrs_dup[i]);
runs in _ldap_free_urldesc() regardless of whether ludp->lud_attrs_dup
has been initialized.
... thus they should be NULL if not inited and free() works on NULL.
Even if we in general avoid free on NULL in libcurl to better catch
mistakes so I'd be fine with adding a check.
It's not freeing NULL that causes a crash. It's dereferencing NULL using
the [] operator that causes a crash.
I think we just need to allocate the array after we count the elements
in ludp->lud_attrs, check to make sure the array's been allocated before
using the [] operator to free its elements, and make sure inside
_ldap_free_urldesc() to free ludp->lud_attrs_dup if it's been allocated.
I've attached a modified version of your patch to show what I mean.
That's been tested on win32, and confirmed expected behavior for both
malformed and well-formed ldap urls.
(I just noticed that my editor on Windows messed up the indentation, and
I flubbed the subject line when using git format-patch, but I don't have
time to address that right at this moment. If this otherwise looks good,
I can fix that and resend if you want.)
Thanks,
Geoff
>From fa5ba9fb5ae46200288e32de6f2b723a34277248 Mon Sep 17 00:00:00 2001
From: Geoff Beier <[email protected]>
Date: Thu, 5 Sep 2013 17:51:53 -0400
Subject: [PATCH] When an error occurs parsing an LDAP URL, The
ludp->lud_attrs[i] entries could be freed even though they sometimes point to
data within an allocated area.
This change introduces a lud_attrs_dup[] array for the duplicated string
pointers, and it removes the unused lud_exts array.
---
lib/ldap.c | 65 ++++++++++++++++++++++++++------------------------------------
1 file changed, 27 insertions(+), 38 deletions(-)
diff --git a/lib/ldap.c b/lib/ldap.c
index 2352715..7ee9817 100644
--- a/lib/ldap.c
+++ b/lib/ldap.c
@@ -77,13 +77,14 @@
/* Use our own implementation. */
typedef struct {
- char *lud_host;
- int lud_port;
- char *lud_dn;
- char **lud_attrs;
- int lud_scope;
- char *lud_filter;
- char **lud_exts;
+ char *lud_host;
+ int lud_port;
+ char *lud_dn;
+ char **lud_attrs;
+ int lud_scope;
+ char *lud_filter;
+ char **lud_exts;
+ char **lud_attrs_dup; /* gets each entry malloc'ed */
} CURL_LDAPURLDesc;
#undef LDAPURLDesc
@@ -364,7 +365,7 @@ static CURLcode Curl_ldap(struct connectdata *conn, bool
*done)
}
rc = ldap_search_s(server, ludp->lud_dn, ludp->lud_scope,
- ludp->lud_filter, ludp->lud_attrs, 0, &result);
+ ludp->lud_filter, ludp->lud_attrs_dup, 0, &result);
if(rc != 0 && rc != LDAP_SIZELIMIT_EXCEEDED) {
failf(data, "LDAP remote: %s", ldap_err2string(rc));
@@ -543,14 +544,9 @@ static bool unescape_elements (void *data, LDAPURLDesc
*ludp)
}
for(i = 0; ludp->lud_attrs && ludp->lud_attrs[i]; i++) {
- ludp->lud_attrs[i] = curl_easy_unescape(data, ludp->lud_attrs[i], 0, NULL);
- if(!ludp->lud_attrs[i])
- return (FALSE);
- }
-
- for(i = 0; ludp->lud_exts && ludp->lud_exts[i]; i++) {
- ludp->lud_exts[i] = curl_easy_unescape(data, ludp->lud_exts[i], 0, NULL);
- if(!ludp->lud_exts[i])
+ ludp->lud_attrs_dup[i] = curl_easy_unescape(data, ludp->lud_attrs[i],
+ 0, NULL);
+ if(!ludp->lud_attrs_dup[i])
return (FALSE);
}
@@ -623,6 +619,8 @@ static int _ldap_url_parse2 (const struct connectdata
*conn, LDAPURLDesc *ludp)
for(i = 0; ludp->lud_attrs[i]; i++)
LDAP_TRACE (("attr[%d] '%s'\n", i, ludp->lud_attrs[i]));
+ /* allocate the array to receive the unescaped attributes */
+ ludp->lud_attrs_dup = calloc(i+1, sizeof(char*));
}
p = q;
@@ -637,8 +635,9 @@ static int _ldap_url_parse2 (const struct connectdata
*conn, LDAPURLDesc *ludp)
if(*p && *p != '?') {
ludp->lud_scope = str2scope(p);
- if(ludp->lud_scope == -1)
+ if(ludp->lud_scope == -1) {
return LDAP_INVALID_SYNTAX;
+ }
LDAP_TRACE (("scope %d\n", ludp->lud_scope));
}
@@ -651,25 +650,13 @@ static int _ldap_url_parse2 (const struct connectdata
*conn, LDAPURLDesc *ludp)
q = strchr(p, '?');
if(q)
*q++ = '\0';
- if(!*p)
+ if(!*p) {
return LDAP_INVALID_SYNTAX;
+ }
ludp->lud_filter = p;
LDAP_TRACE (("filter '%s'\n", ludp->lud_filter));
- p = q;
- if(!p)
- goto success;
-
- /* parse extensions
- */
- ludp->lud_exts = split_str(p);
- if(!ludp->lud_exts)
- return LDAP_NO_MEMORY;
-
- for(i = 0; ludp->lud_exts[i]; i++)
- LDAP_TRACE (("exts[%d] '%s'\n", i, ludp->lud_exts[i]));
-
success:
if(!unescape_elements(conn->data, ludp))
return LDAP_NO_MEMORY;
@@ -709,16 +696,18 @@ static void _ldap_free_urldesc (LDAPURLDesc *ludp)
free(ludp->lud_filter);
if(ludp->lud_attrs) {
- for(i = 0; ludp->lud_attrs[i]; i++)
- free(ludp->lud_attrs[i]);
+ if(ludp->lud_attrs_dup) {
+ for(i = 0; ludp->lud_attrs_dup[i]; i++) {
+ if(!ludp->lud_attrs_dup[i])
+ /* abort loop on first NULL */
+ break;
+ free(ludp->lud_attrs_dup[i]);
+ }
+ free(ludp->lud_attrs_dup);
+ }
free(ludp->lud_attrs);
}
- if(ludp->lud_exts) {
- for(i = 0; ludp->lud_exts[i]; i++)
- free(ludp->lud_exts[i]);
- free(ludp->lud_exts);
- }
free (ludp);
}
#endif /* !HAVE_LDAP_URL_PARSE */
--
1.8.3.msysgit.0
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
