You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe Jr." <wr...@rowe-clan.net> on 2011/12/20 10:26:31 UTC
Re: CVE-2011-3607, int overflow ap_pregsub()
We should come to a conclusion on this.
On 11/15/2011 8:22 AM, "Plüm, Rüdiger, VF-Group" wrote:
> The patch is fine on trunk because the affected code is not within
>
> AP_DECLARE(char *) ap_pregsub(...)
>
> but within
>
> static apr_status_t regsub_core(apr_pool_t *p, char **result,
> struct ap_varbuf *vb, const char *input,
> const char *source, size_t nmatch,
> ap_regmatch_t pmatch[], apr_size_t maxlen)
>
> but there is no regsub_core in 2.2.x. So the patch needs to be adjusted for backport
> to 2.2.x. But returning NULL in the 2.2.x case looks to be the correct thing to do
> as this is how trunk behaves now.
> OTOH there was some discussion on this list whether it is correct to backport this trunk
> behaviour to 2.2.x.
>
> Regards
>
> Rüdiger
>
>> -----Original Message-----
>> From: Roman Drahtmueller [mailto:draht@suse.de]
>> Sent: Dienstag, 15. November 2011 15:13
>> To: dev@httpd.apache.org
>> Subject: CVE-2011-3607, int overflow ap_pregsub()
>>
>> Hi there,
>>
>> Revision 1198940 attempts to fix an integer overflow in
>> ap_pregsub() in
>> server/util.c:394. The patch is:
>>
>> --- httpd/httpd/trunk/server/util.c 2011/11/07 21:09:41 1198939
>> +++ httpd/httpd/trunk/server/util.c 2011/11/07 21:13:40 1198940
>> @@ -411,6 +411,8 @@
>> len++;
>> }
>> else if (no < nmatch && pmatch[no].rm_so <
>> pmatch[no].rm_eo) {
>> + if (APR_SIZE_MAX - len <= pmatch[no].rm_eo -
>> pmatch[no].rm_so)
>> + return APR_ENOMEM;
>> len += pmatch[no].rm_eo - pmatch[no].rm_so;
>> }
>>
>>
>> , and appears wrong, because, ap_pregsub() is
>>
>> AP_DECLARE(char *) ap_pregsub(...)
>>
>> This would require something along the lines of (proposal):
>>
>>
>> }
>> else if (no < nmatch && pmatch[no].rm_so <
>> pmatch[no].rm_eo) {
>> + if (APR_SIZE_MAX - len <= pmatch[no].rm_eo -
>> pmatch[no].rm_so) {
>> + ap_log_error(APLOG_MARK, APLOG_WARNING,
>> APR_ENOMEM, NULL,
>> + "integer overflow or out of memory
>> condition." );
>> + return NULL;
>> + }
>> len += pmatch[no].rm_eo - pmatch[no].rm_so;
>> }
>>
>> }
>>
>> dest = dst = apr_pcalloc(p, len + 1);
>>
>> + if(!dest)
>> + return NULL;
>> +
>> +
>> /* Now actually fill in the string */
>>
>>
>> ...or simply without the error logging.
>>
>> Thoughts?
>> Thanks,
>> Roman.
>>
>
Re: CVE-2011-3607, int overflow ap_pregsub()
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Wed, 21 Dec 2011, Greg Ames wrote:
> On Tue, Dec 20, 2011 at 4:26 AM, William A. Rowe Jr.
> <wr...@rowe-clan.net> wrote:
>> We should come to a conclusion on this.
>
> How about this for 2.2.x ?
>
> --- server/util.c (revision 1179624)
> +++ server/util.c (working copy)
> @@ -82,6 +82,8 @@
> #define IS_SLASH(s) (s == '/')
> #endif
>
> +/* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
> +#define UTIL_SIZE_MAX (~((apr_size_t)0))
>
> /*
> * Examine a field value (such as a media-/content-type) string and return
> @@ -391,6 +393,11 @@
> len++;
> }
> else if (no < nmatch && pmatch[no].rm_so < pmatch[no].rm_eo) {
> + if (UTIL_SIZE_MAX - len <= pmatch[no].rm_eo - pmatch[no].rm_so) {
> + ap_log_error(APLOG_MARK, APLOG_WARNING, APR_ENOMEM, NULL,
> + "integer overflow or out of memory condition." );
> + return NULL;
> + }
> len += pmatch[no].rm_eo - pmatch[no].rm_so;
> }
len is int in 2.2. This should be changed into an apr_size_t, too.
> full discloser: my make using apr 1.2 choked trying to compile
> byterange_filter because apr_array_clear wasn't defined. It is in apr
> 1.3. However httpd's configure.in appears to be happy with any apr
> 1.x release.
Then configure should be fixed, IMHO.
Re: CVE-2011-3607, int overflow ap_pregsub()
Posted by Guenter Knauf <fu...@apache.org>.
Am 21.12.2011 23:28, schrieb Guenter Knauf:
> Am 21.12.2011 20:08, schrieb Greg Ames:
>> On Tue, Dec 20, 2011 at 4:26 AM, William A. Rowe Jr.
>> <wr...@rowe-clan.net> wrote:
>>> We should come to a conclusion on this.
>>
>> How about this for 2.2.x ?
>>
>> --- server/util.c (revision 1179624)
>> +++ server/util.c (working copy)
>> @@ -82,6 +82,8 @@
>> #define IS_SLASH(s) (s == '/')
>> #endif
>>
>> +/* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
>> +#define UTIL_SIZE_MAX (~((apr_size_t)0))
> thats bad - you should protect it since some compilers do not like macro
> redefines (for the case you have APR already defining it):
>
> /* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
> #ifndef UTIL_SIZE_MAX
> #define UTIL_SIZE_MAX (~((apr_size_t)0))
> #endif
sorry, forget - I missed you didnt use the APR macro :-)
Re: CVE-2011-3607, int overflow ap_pregsub()
Posted by Guenter Knauf <fu...@apache.org>.
Am 21.12.2011 20:08, schrieb Greg Ames:
> On Tue, Dec 20, 2011 at 4:26 AM, William A. Rowe Jr.
> <wr...@rowe-clan.net> wrote:
>> We should come to a conclusion on this.
>
> How about this for 2.2.x ?
>
> --- server/util.c (revision 1179624)
> +++ server/util.c (working copy)
> @@ -82,6 +82,8 @@
> #define IS_SLASH(s) (s == '/')
> #endif
>
> +/* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
> +#define UTIL_SIZE_MAX (~((apr_size_t)0))
thats bad - you should protect it since some compilers do not like macro
redefines (for the case you have APR already defining it):
/* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
#ifndef UTIL_SIZE_MAX
#define UTIL_SIZE_MAX (~((apr_size_t)0))
#endif
Gün.
Re: CVE-2011-3607, int overflow ap_pregsub()
Posted by Rüdiger Plüm <ru...@vodafone.com>.
Am 21.12.2011 20:08, schrieb Greg Ames:
> On Tue, Dec 20, 2011 at 4:26 AM, William A. Rowe Jr.
> <wr...@rowe-clan.net> wrote:
>> We should come to a conclusion on this.
>
> How about this for 2.2.x ?
>
> --- server/util.c (revision 1179624)
> +++ server/util.c (working copy)
> @@ -82,6 +82,8 @@
> #define IS_SLASH(s) (s == '/')
> #endif
>
> +/* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
> +#define UTIL_SIZE_MAX (~((apr_size_t)0))
>
> /*
> * Examine a field value (such as a media-/content-type) string and return
> @@ -391,6 +393,11 @@
> len++;
> }
> else if (no< nmatch&& pmatch[no].rm_so< pmatch[no].rm_eo) {
> + if (UTIL_SIZE_MAX - len<= pmatch[no].rm_eo - pmatch[no].rm_so) {
> + ap_log_error(APLOG_MARK, APLOG_WARNING, APR_ENOMEM, NULL,
> + "integer overflow or out of memory condition." );
> + return NULL;
> + }
> len += pmatch[no].rm_eo - pmatch[no].rm_so;
> }
>
> Is apr 1.3 required for current 2.2.x? I know it wasn't for older
IMHO APR 1.3 is mandatory for 2.2.x.
Regards
Rüdiger
Re: CVE-2011-3607, int overflow ap_pregsub()
Posted by Greg Ames <am...@gmail.com>.
On Tue, Dec 20, 2011 at 4:26 AM, William A. Rowe Jr.
<wr...@rowe-clan.net> wrote:
> We should come to a conclusion on this.
How about this for 2.2.x ?
--- server/util.c (revision 1179624)
+++ server/util.c (working copy)
@@ -82,6 +82,8 @@
#define IS_SLASH(s) (s == '/')
#endif
+/* same as APR_SIZE_MAX which doesn't appear until APR 1.3 */
+#define UTIL_SIZE_MAX (~((apr_size_t)0))
/*
* Examine a field value (such as a media-/content-type) string and return
@@ -391,6 +393,11 @@
len++;
}
else if (no < nmatch && pmatch[no].rm_so < pmatch[no].rm_eo) {
+ if (UTIL_SIZE_MAX - len <= pmatch[no].rm_eo - pmatch[no].rm_so) {
+ ap_log_error(APLOG_MARK, APLOG_WARNING, APR_ENOMEM, NULL,
+ "integer overflow or out of memory condition." );
+ return NULL;
+ }
len += pmatch[no].rm_eo - pmatch[no].rm_so;
}
Is apr 1.3 required for current 2.2.x? I know it wasn't for older
2.2.x releases, and I hope we don't change apr levels in the middle of
an httpd version. Therefore I created the local #define
UTIL_SIZE_MAX. Do we document the apr minimum prereqs anywhere?
full discloser: my make using apr 1.2 choked trying to compile
byterange_filter because apr_array_clear wasn't defined. It is in apr
1.3. However httpd's configure.in appears to be happy with any apr
1.x release.
Greg