On Tue, Apr 29, 2014 at 6:06 PM <minf...@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.
Index: modules/aaa/mod_authnz_ldap.c
===================================================================
--- modules/aaa/mod_authnz_ldap.c (revision 1913813)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -206,7 +206,7 @@ static const char* authn_ldap_xlate_password(reque
* search filter will be (&(posixid=*)(uid=userj)).
*/
#define FILTER_LENGTH MAX_STRING_LEN
-static apr_status_t authn_ldap_build_filter(char *filtbuf,
+static apr_status_t authn_ldap_build_filter(char filtbuf[FILTER_LENGTH],
request_rec *r,
const char *user,
const char *filter,
@@ -244,7 +244,7 @@ static const char* authn_ldap_xlate_password(reque
* config-supplied portions.
*/
- if ((nofilter = (filter && !strcasecmp(filter, "none")))) {
+ if ((nofilter = (!filter || !*filter || !strcasecmp(filter, "none")))) {
len = apr_snprintf(filtbuf, FILTER_LENGTH, "(%s=", sec->attribute);
}
else {
@@ -256,12 +256,12 @@ static const char* authn_ldap_xlate_password(reque
* LDAP filter metachars are escaped.
*/
filtbuf_end = filtbuf + FILTER_LENGTH - 1;
+ for (p = user, q = filtbuf + len; *p; ) {
+ if (strchr("*()\\", *p) != NULL) {
#if APR_HAS_MICROSOFT_LDAPSDK
- for (p = user, q=filtbuf + len;
- *p && q < filtbuf_end; ) {
- if (strchr("*()\\", *p) != NULL) {
- if ( q + 3 >= filtbuf_end)
- break; /* Don't write part of escape sequence if we can't write all of it */
+ if (q + 3 >= filtbuf_end) { /* accounts for final \0 */
+ return APR_EGENERAL;
+ }
*q++ = '\\';
switch ( *p++ )
{
@@ -281,23 +281,22 @@ static const char* authn_ldap_xlate_password(reque
*q++ = '5';
*q++ = 'c';
break;
- }
- }
- else
- *q++ = *p++;
- }
+ }
#else
- for (p = user, q=filtbuf + len;
- *p && q < filtbuf_end; *q++ = *p++) {
- if (strchr("*()\\", *p) != NULL) {
+ if (q + 2 >= filtbuf_end) { /* accounts for final \0 */
+ return APR_EGENERAL;
+ }
*q++ = '\\';
- if (q >= filtbuf_end) {
- break;
+ *q++ = *p++;
+#endif
+ }
+ else {
+ if (q + 1 >= filtbuf_end) { /* accounts for final \0 */
+ return APR_EGENERAL;
}
+ *q++ = *p++;
}
}
-#endif
- *q = '\0';
/*
* Append the closing parens of the filter, unless doing so would
@@ -305,21 +304,19 @@ static const char* authn_ldap_xlate_password(reque
*/
if (nofilter) {
- if (q + 1 <= filtbuf_end) {
- strcat(filtbuf, ")");
- }
- else {
+ if (q + 1 >= filtbuf_end) { /* accounts for final \0 */
return APR_EGENERAL;
}
+ *q++ = ')';
}
else {
- if (q + 2 <= filtbuf_end) {
- strcat(filtbuf, "))");
- }
- else {
+ if (q + 2 >= filtbuf_end) { /* accounts for final \0 */
return APR_EGENERAL;
}
+ *q++ = ')';
+ *q++ = ')';
}
+ *q = '\0';
return APR_SUCCESS;
}
