You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2014/04/29 18:05:56 UTC
svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c
Author: minfrin
Date: Tue Apr 29 16:05:56 2014
New Revision: 1591012
URL: http://svn.apache.org/r1591012
Log:
mod_authnz_ldap: Fail explicitly when the filter is too long. Remove
unnecessary apr_pstrdup() and strlen().
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/docs/log-message-tags/next-number
httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1591012&r1=1591011&r2=1591012&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Tue Apr 29 16:05:56 2014
@@ -1,6 +1,9 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.0
+ *) mod_authnz_ldap: Fail explicitly when the filter is too long. Remove
+ unnecessary apr_pstrdup() and strlen(). [Graham Leggett]
+
*) mod_proxy_fcgi: Don't segfault when failing to connect to the backend.
[Jeff Trawick]
Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1591012&r1=1591011&r2=1591012&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
+++ httpd/httpd/trunk/docs/log-message-tags/next-number Tue Apr 29 16:05:56 2014
@@ -1 +1 @@
-2622
+2634
Modified: httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c?rev=1591012&r1=1591011&r2=1591012&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
+++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Tue Apr 29 16:05:56 2014
@@ -188,11 +188,8 @@ static const char* authn_ldap_xlate_pass
/*
* Build the search filter, or at least as much of the search filter that
- * will fit in the buffer. We don't worry about the buffer not being able
- * to hold the entire filter. If the buffer wasn't big enough to hold the
- * filter, ldap_search_s will complain, but the only situation where this
- * is likely to happen is if the client sent a really, really long
- * username, most likely as part of an attack.
+ * will fit in the buffer, and return APR_EGENERAL if it won't fit, otherwise
+ * APR_SUCCESS.
*
* The search filter consists of the filter provided with the URL,
* combined with a filter made up of the attribute provided with the URL,
@@ -205,31 +202,23 @@ static const char* authn_ldap_xlate_pass
* search filter will be (&(posixid=*)(uid=userj)).
*/
#define FILTER_LENGTH MAX_STRING_LEN
-static void authn_ldap_build_filter(char *filtbuf,
+static apr_status_t authn_ldap_build_filter(char *filtbuf,
request_rec *r,
- const char* sent_user,
- const char* sent_filter,
+ const char *user,
+ const char *filter,
authn_ldap_config_t *sec)
{
- char *p, *q, *filtbuf_end;
- char *user, *filter;
+ char *q;
+ const char *p, *filtbuf_end;
apr_xlate_t *convset = NULL;
apr_size_t inbytes;
apr_size_t outbytes;
char *outbuf;
- int nofilter = 0;
+ int nofilter = 0, len;
- if (sent_user != NULL) {
- user = apr_pstrdup (r->pool, sent_user);
- }
- else
- return;
-
- if (sent_filter != NULL) {
- filter = apr_pstrdup (r->pool, sent_filter);
- }
- else
+ if (!filter) {
filter = sec->filter;
+ }
if (charset_conversions) {
convset = get_conv_set(r);
@@ -242,7 +231,7 @@ static void authn_ldap_build_filter(char
/* Convert the user name to UTF-8. This is only valid for LDAP v3 */
if (apr_xlate_conv_buffer(convset, user, &inbytes, outbuf, &outbytes) == APR_SUCCESS) {
- user = apr_pstrdup(r->pool, outbuf);
+ user = outbuf;
}
}
@@ -252,10 +241,10 @@ static void authn_ldap_build_filter(char
*/
if ((nofilter = (filter && !strcasecmp(filter, "none")))) {
- apr_snprintf(filtbuf, FILTER_LENGTH, "(%s=", sec->attribute);
+ len = apr_snprintf(filtbuf, FILTER_LENGTH, "(%s=", sec->attribute);
}
else {
- apr_snprintf(filtbuf, FILTER_LENGTH, "(&(%s)(%s=", filter, sec->attribute);
+ len = apr_snprintf(filtbuf, FILTER_LENGTH, "(&(%s)(%s=", filter, sec->attribute);
}
/*
@@ -264,7 +253,7 @@ static void authn_ldap_build_filter(char
*/
filtbuf_end = filtbuf + FILTER_LENGTH - 1;
#if APR_HAS_MICROSOFT_LDAPSDK
- for (p = user, q=filtbuf + strlen(filtbuf);
+ for (p = user, q=filtbuf + len;
*p && q < filtbuf_end; ) {
if (strchr("*()\\", *p) != NULL) {
if ( q + 3 >= filtbuf_end)
@@ -294,7 +283,7 @@ static void authn_ldap_build_filter(char
*q++ = *p++;
}
#else
- for (p = user, q=filtbuf + strlen(filtbuf);
+ for (p = user, q=filtbuf + len;
*p && q < filtbuf_end; *q++ = *p++) {
if (strchr("*()\\", *p) != NULL) {
*q++ = '\\';
@@ -312,14 +301,23 @@ static void authn_ldap_build_filter(char
*/
if (nofilter) {
- if (q + 1 <= filtbuf_end)
+ if (q + 1 <= filtbuf_end) {
strcat(filtbuf, ")");
+ }
+ else {
+ return APR_EGENERAL;
+ }
}
else {
- if (q + 2 <= filtbuf_end)
+ if (q + 2 <= filtbuf_end) {
strcat(filtbuf, "))");
+ }
+ else {
+ return APR_EGENERAL;
+ }
}
+ return APR_SUCCESS;
}
static void *create_authnz_ldap_dir_config(apr_pool_t *p, char *d)
@@ -531,7 +529,13 @@ static authn_status authn_ldap_check_pas
}
/* build the username filter */
- authn_ldap_build_filter(filtbuf, r, user, NULL, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, user, NULL, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02622)
+ "auth_ldap authenticate: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ util_ldap_connection_close(ldc);
+ return AUTH_GENERAL_ERROR;
+ }
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r,
"auth_ldap authenticate: final authn filter is %s", filtbuf);
@@ -678,7 +682,12 @@ static authz_status ldapuser_check_autho
sizeof(authn_ldap_request_t));
/* Build the username filter */
- authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02623)
+ "auth_ldap authorize: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ return AUTHZ_DENIED;
+ }
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
@@ -862,7 +871,12 @@ static authz_status ldapgroup_check_auth
req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
sizeof(authn_ldap_request_t));
/* Build the username filter */
- authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02624)
+ "auth_ldap authorize: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ return AUTHZ_DENIED;
+ }
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
@@ -1051,7 +1065,12 @@ static authz_status ldapdn_check_authori
req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
sizeof(authn_ldap_request_t));
/* Build the username filter */
- authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02625)
+ "auth_ldap authorize: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ return AUTHZ_DENIED;
+ }
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
@@ -1173,7 +1192,12 @@ static authz_status ldapattribute_check_
req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
sizeof(authn_ldap_request_t));
/* Build the username filter */
- authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02626)
+ "auth_ldap authorize: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ return AUTHZ_DENIED;
+ }
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
@@ -1301,7 +1325,12 @@ static authz_status ldapfilter_check_aut
req = (authn_ldap_request_t *)apr_pcalloc(r->pool,
sizeof(authn_ldap_request_t));
/* Build the username filter */
- authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, r->user, NULL, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02627)
+ "auth_ldap authorize: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ return AUTHZ_DENIED;
+ }
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
@@ -1341,7 +1370,12 @@ static authz_status ldapfilter_check_aut
"auth_ldap authorize: checking filter %s", t);
/* Build the username filter */
- authn_ldap_build_filter(filtbuf, r, req->user, t, sec);
+ if (APR_SUCCESS != authn_ldap_build_filter(filtbuf, r, req->user, t, sec)) {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02628)
+ "auth_ldap authorize: ldap filter too long (>%d): %s",
+ FILTER_LENGTH, filtbuf);
+ return AUTHZ_DENIED;
+ }
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
@@ -1427,7 +1461,7 @@ static authz_status ldapsearch_check_aut
require = ap_expr_str_exec(r, expr, &err);
if (err) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02629)
"auth_ldap authorize: require ldap-search: Can't "
"evaluate require expression: %s", err);
return AUTHZ_DENIED;
@@ -1438,7 +1472,7 @@ static authz_status ldapsearch_check_aut
if (t[0]) {
const char **vals;
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02630)
"auth_ldap authorize: checking filter %s", t);
/* Search for the user DN */
@@ -1447,20 +1481,20 @@ static authz_status ldapsearch_check_aut
/* Make sure that the filtered search returned a single dn */
if (result == LDAP_SUCCESS && dn) {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
"auth_ldap authorize: require ldap-search: "
"authorization successful");
return AUTHZ_GRANTED;
}
else {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02632)
"auth_ldap authorize: require ldap-search: "
"%s authorization failed [%s][%s]",
t, ldc->reason, ldap_err2string(result));
}
}
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02633)
"auth_ldap authorize filter: authorization denied for "
"to %s", r->uri);
Re: svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c
Posted by Graham Leggett via dev <de...@httpd.apache.org>.
On 18 Nov 2023, at 17:14, Yann Ylavic <yl...@gmail.com> wrote:
> Oh it seems that the callers want the "filtbuf" to be \0 terminated
> (even in case of error), so this v2 probably..
Looking at this now.
Seems to be some differences between trunk and v2.4, seeing how they can be aligned to make this easier.
Regards,
Graham
—
Re: svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Nov 18, 2023 at 5:59 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> All in all, I'd rewrite this function like in the attached patch (not
> even compile tested, just to show what I'm talking about..).
Oh it seems that the callers want the "filtbuf" to be \0 terminated
(even in case of error), so this v2 probably..
>
> Regards;
> Yann.
Re: svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Apr 29, 2014 at 6:06 PM <mi...@apache.org> wrote:
>
> Author: minfrin
> Date: Tue Apr 29 16:05:56 2014
> New Revision: 1591012
>
> URL: http://svn.apache.org/r1591012
> Log:
> mod_authnz_ldap: Fail explicitly when the filter is too long. Remove
> unnecessary apr_pstrdup() and strlen().
It seems that the escaping cases don't error out if filtbuf is not
large enough? (see below)
>
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Tue Apr 29 16:05:56 2014
[]
> @@ -205,31 +202,23 @@ static const char* authn_ldap_xlate_pass
> * search filter will be (&(posixid=*)(uid=userj)).
> */
> #define FILTER_LENGTH MAX_STRING_LEN
> -static void authn_ldap_build_filter(char *filtbuf,
> +static apr_status_t authn_ldap_build_filter(char *filtbuf,
> request_rec *r,
> - const char* sent_user,
> - const char* sent_filter,
> + const char *user,
> + const char *filter,
> authn_ldap_config_t *sec)
> {
[]
> @@ -264,7 +253,7 @@ static void authn_ldap_build_filter(char
> */
> filtbuf_end = filtbuf + FILTER_LENGTH - 1;
> #if APR_HAS_MICROSOFT_LDAPSDK
> - for (p = user, q=filtbuf + strlen(filtbuf);
> + for (p = user, q=filtbuf + len;
> *p && q < filtbuf_end; ) {
> if (strchr("*()\\", *p) != NULL) {
> if ( q + 3 >= filtbuf_end)
Here we break the loop and fall through with no error.
> @@ -294,7 +283,7 @@ static void authn_ldap_build_filter(char
> *q++ = *p++;
> }
> #else
> - for (p = user, q=filtbuf + strlen(filtbuf);
> + for (p = user, q=filtbuf + len;
> *p && q < filtbuf_end; *q++ = *p++) {
> if (strchr("*()\\", *p) != NULL) {
> *q++ = '\\';
Next it's:
if (q >= filtbuf_end) {
break;
}
so same here, plus it's not consistent because for
APR_HAS_MICROSOFT_LDAPSDK in the previous hunk we break out before
'\\' while here it's after.
> @@ -312,14 +301,23 @@ static void authn_ldap_build_filter(char
> */
>
> if (nofilter) {
> - if (q + 1 <= filtbuf_end)
> + if (q + 1 <= filtbuf_end) {
> strcat(filtbuf, ")");
> + }
> + else {
> + return APR_EGENERAL;
> + }
> }
> else {
> - if (q + 2 <= filtbuf_end)
> + if (q + 2 <= filtbuf_end) {
> strcat(filtbuf, "))");
> + }
> + else {
> + return APR_EGENERAL;
> + }
> }
These final checks do not catch the escaping truncations either
because we might have left some room. Also the strcat()s look
inefficient since "q" points at the end of "filtbuf" already.
>
> + return APR_SUCCESS;
> }
All in all, I'd rewrite this function like in the attached patch (not
even compile tested, just to show what I'm talking about..).
Regards;
Yann.