You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Fritsch <sf...@sfritsch.de> on 2011/11/27 17:33:41 UTC
Re: svn commit: r1205894 - in /httpd/httpd/trunk: include/util_filter.h
modules/cache/mod_cache.c server/util_filter.c
On Thu, 24 Nov 2011, jim@apache.org wrote:
> Author: jim
> Date: Thu Nov 24 15:53:16 2011
> New Revision: 1205894
>
> URL: http://svn.apache.org/viewvc?rev=1205894&view=rev
> Log:
> Use varargs...
>
> Modified:
> httpd/httpd/trunk/include/util_filter.h
> httpd/httpd/trunk/modules/cache/mod_cache.c
> httpd/httpd/trunk/server/util_filter.c
>
> Modified: httpd/httpd/trunk/server/util_filter.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1205894&r1=1205893&r2=1205894&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/util_filter.c (original)
> +++ httpd/httpd/trunk/server/util_filter.c Thu Nov 24 15:53:16 2011
> @@ -544,17 +544,25 @@ AP_DECLARE(apr_status_t) ap_pass_brigade
> */
> AP_DECLARE(apr_status_t) ap_pass_brigade_fchk(request_rec *r,
> apr_bucket_brigade *bb,
> - const char *errmsg)
> + const char *fmt,
> + ...)
> {
> apr_status_t rv;
> - if (!errmsg)
> - errmsg = "ap_pass_brigade returned";
>
> rv = ap_pass_brigade(r->output_filters, bb);
> if (rv != APR_SUCCESS) {
> if (rv != AP_FILTER_ERROR) {
> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r,
> - "%s %d", errmsg, rv);
> + if (!fmt)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r,
> + "ap_pass_brigade returned %d", rv);
> + else {
> + va_list ap;
> + const char *res;
> + va_start(ap, fmt);
> + res = apr_pvsprintf(r->pool, fmt, ap);
> + va_end(ap);
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, res, NULL);
> + }
No, this is not right. If some caller passes arguments to
ap_pass_brigade_fchk that may cause the result of apr_pvsprintf to contain
a "%", you would get a format-string vulnerability. This could easily
happen if some error message included the URL.
You must use
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, "%s", res);
intead.
> return HTTP_INTERNAL_SERVER_ERROR;
> }
> return AP_FILTER_ERROR;
>
>
>
Re: svn commit: r1205894 - in /httpd/httpd/trunk: include/util_filter.h modules/cache/mod_cache.c server/util_filter.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Nov 27, 2011, at 11:33 AM, Stefan Fritsch wrote:
>> + else {
>> + va_list ap;
>> + const char *res;
>> + va_start(ap, fmt);
>> + res = apr_pvsprintf(r->pool, fmt, ap);
>> + va_end(ap);
>> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, res, NULL);
>> + }
>
> No, this is not right. If some caller passes arguments to ap_pass_brigade_fchk that may cause the result of apr_pvsprintf to contain a "%", you would get a format-string vulnerability. This could easily happen if some error message included the URL.
>
> You must use
>
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, "%s", res);
>
> intead.
Thx!