Re: [Regression in httpd 2.4.52] mod_dav: Potentially unbounded memory usage in PROPFIND with dav_get_props() and dav_propfind_walker()

[Ruediger Pluem]

On 1/18/22 2:58 PM, Evgeny Kotkov wrote:
> Ruediger Pluem  writes:
> 
>> Can you please check if the below patch fixes your issue?
> 
> I have to say that the reason and the original intention of using
> resource->pool's userdata here are still somewhat unclear to me.

An application seems to be here:

https://github.com/minfrin/mod_dav_calendar/blob/main/mod_dav_calendar.c#L2170

As far as I understand the code needs the data stored in the userdata and has 
no other means to retrieve it.

> 
> But it does look like the patch performs the allocation only once per
> resource->pool, and so it should fix the unbounded memory usage issue.

r1897182

Regards

Rüdiger

Re: [Regression in httpd 2.4.52] mod_dav: Potentially unbounded memory usage in PROPFIND with dav_get_props() and dav_propfind_walker()

[Evgeny Kotkov]

Ruediger Pluem  writes:

> Can you please check if the below patch fixes your issue?

I have to say that the reason and the original intention of using
resource->pool's userdata here are still somewhat unclear to me.

But it does look like the patch performs the allocation only once per
resource->pool, and so it should fix the unbounded memory usage issue.

Thanks!


Regards,
Evgeny Kotkov

Re: [Regression in httpd 2.4.52] mod_dav: Potentially unbounded memory usage in PROPFIND with dav_get_props() and dav_propfind_walker()

[Ruediger Pluem]

On 1/14/22 1:57 PM, Evgeny Kotkov wrote:
> Hi,
> 
> I might have stumbled across a regression in httpd 2.4.52 where mod_dav was
> changed in a way where dav_get_props() now allocates data in resource->pool.
> 
> I think that r1879889 [1] is the change that is causing the new behavior.
> This change has been backported to 2.4.x in r1895893 [2].
> 
> Consider the new part of the dav_get_props() function:
> 
> DAV_DECLARE(dav_get_props_result) dav_get_props()
> {
>…
>element = apr_pcalloc(propdb->resource->pool, sizeof(dav_liveprop_elem));
>element->doc = doc;
>…
> 
> This code unconditionally allocates data in resource->pool (this is the only
> such place, other allocations performed by this function happen in propdb->p).
> 
> The dav_get_props() is called by dav_propfind_walker().  The walker function
> is driven by a specific provider.  Both of the two implementations known to
> me, mod_dav_fs and mod_dav_svn, happen to use a long-living pool as the
> resource->pool during the walk.
> 
> I think that with this change, the PROPFIND walks are going to make O(N)
> allocations for O(N) walked items — which is an unbounded memory usage
> and a regression, compared to 2.4.51.
> 
> [1] https://svn.apache.org/r1879889
> [2] https://svn.apache.org/r1895893

Can you please check if the below patch fixes your issue?

Index: modules/dav/main/props.c
===
--- modules/dav/main/props.c(revision 1896810)
+++ modules/dav/main/props.c(working copy)
@@ -805,9 +805,15 @@
 /* we lose both the document and the element when calling (insert_prop),
  * make these available in the pool.
  */
-element = apr_pcalloc(propdb->resource->pool, sizeof(dav_liveprop_elem));
+element = dav_get_liveprop_element(propdb->resource);
+if (!element) {
+element = apr_pcalloc(propdb->resource->pool, 
sizeof(dav_liveprop_elem));
+apr_pool_userdata_setn(element, DAV_PROP_ELEMENT, NULL, 
propdb->resource->pool);
+}
+else {
+memset(element, 0, sizeof(dav_liveprop_elem));
+}
 element->doc = doc;
-apr_pool_userdata_setn(element, DAV_PROP_ELEMENT, NULL, 
propdb->resource->pool);

 /* ### NOTE: we should pass in TWO buffers -- one for keys, one for
the marks */


Regards

Rüdiger

[Regression in httpd 2.4.52] mod_dav: Potentially unbounded memory usage in PROPFIND with dav_get_props() and dav_propfind_walker()

[Evgeny Kotkov]

Hi,

I might have stumbled across a regression in httpd 2.4.52 where mod_dav was
changed in a way where dav_get_props() now allocates data in resource->pool.

I think that r1879889 [1] is the change that is causing the new behavior.
This change has been backported to 2.4.x in r1895893 [2].

Consider the new part of the dav_get_props() function:

DAV_DECLARE(dav_get_props_result) dav_get_props()
{
   …
   element = apr_pcalloc(propdb->resource->pool, sizeof(dav_liveprop_elem));
   element->doc = doc;
   …

This code unconditionally allocates data in resource->pool (this is the only
such place, other allocations performed by this function happen in propdb->p).

The dav_get_props() is called by dav_propfind_walker().  The walker function
is driven by a specific provider.  Both of the two implementations known to
me, mod_dav_fs and mod_dav_svn, happen to use a long-living pool as the
resource->pool during the walk.

I think that with this change, the PROPFIND walks are going to make O(N)
allocations for O(N) walked items — which is an unbounded memory usage
and a regression, compared to 2.4.51.

[1] https://svn.apache.org/r1879889
[2] https://svn.apache.org/r1895893


Thanks,
Evgeny Kotkov