On Thu, 5 Sep 2013, Geoff Beier wrote:
Thanks for the feedback!
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?
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.
I've confirmed both of the above in a debugger.
And what problems do they cause?
I think the way to finish the fix using this approach is probably to use the
attribute count gathered after the loop on line 619 (following the call to
split_str()) to initialize ludp->lud_attrs_dup, and bracket the freeing of
those elements in if(ludp->lud_attrs_dup inside _ldap_free_urldesc(). If
that sounds right to you, I'll try to put something together a later this
week.
Or just break the loop on the first NULL pointer?
I've attached an updated version of the patch that does that and also it keeps
the original struct layout before the new 'lud_attrs_dup' as it struck me
ldap_url_parse() probably needs that - if anyone ever actually builds this
code with HAVE_LDAP_URL_PARSE defined.
--
/ daniel.haxx.se
From d81fd49e56b45ee92fcd4caf325a0fe1c99b2312 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <[email protected]>
Date: Tue, 20 Aug 2013 16:50:43 -0400
Subject: [PATCH] ldap_url_parse2: avoid double-free with malformed LDAP URL
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 | 60 ++++++++++++++++++++++--------------------------------------
1 file changed, 22 insertions(+), 38 deletions(-)
diff --git a/lib/ldap.c b/lib/ldap.c
index 2352715..4e695e9 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);
}
@@ -637,8 +633,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 +648,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 +694,15 @@ 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]);
+ 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);
}
- 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.4.rc3
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
