Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Ruediger Pluem]

On 1/17/22 5:10 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Jan 17 16:10:51 2022
> New Revision: 1897156
> 
> URL: http://svn.apache.org/viewvc?rev=1897156=rev
> Log:
> core: Allow an optional expression to be specified for an effective
> path in the DirectoryMatch and LocationMatch directives. This allows
> modules like mod_dav to map URLs to URL spaces or to directories on
> the filesystem.
> 
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/core.xml
> httpd/httpd/trunk/modules/dav/main/mod_dav.c
> httpd/httpd/trunk/server/core.c
> 
avior in the filesystem
> 
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1897156=1897155=1897156=diff
> ==
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Mon Jan 17 16:10:51 2022

>  
> @@ -723,6 +737,57 @@ static int dav_get_overwrite(request_rec
>  return -1;
>  }
>  
> +static int uripath_is_canonical(const char *uripath)

Isn't this a filesystem path we are talking about in the  case?
How does this function work on Windows when people might use '\' instead of '/'?

> +{
> +const char *dot_pos, *ptr = uripath;
> +apr_size_t i, len;
> +unsigned pattern = 0;
> +
> +/* URIPATH is canonical if it has:
> + *  - no '.' segments
> + *  - no closing '/'
> + *  - no '//'
> + */
> +
> +if (ptr[0] == '.'
> +&& (ptr[1] == '/' || ptr[1] == '\0'
> +|| (ptr[1] == '.' && (ptr[2] == '/' || ptr[2] == 
> '\0' {
> +return 0;
> +}
> +
> +/* valid special cases */
> +len = strlen(ptr);
> +if (len < 2) {

Empty pathes ("") are ok?

> +return 1;
> +}
> +
> +/* invalid endings */
> +if (ptr[len - 1] == '/' || (ptr[len - 1] == '.' && ptr[len - 2] == '/')) 
> {
> +return 0;
> +}
> +
> +/* '.' are rare. So, search for them globally. There will often be no
> + * more than one hit.  Also note that we already checked for invalid
> + * starts and endings, i.e. we only need to check for "/./"

Why don't we need to look for /../ segments?

> + */
> +for (dot_pos = memchr(ptr, '.', len); dot_pos;
> +dot_pos = strchr(dot_pos + 1, '.')) {
> +if (dot_pos > ptr && dot_pos[-1] == '/' && dot_pos[1] == '/') {
> +return 0;
> +}
> +}
> +
> +/* Now validate the rest of the path. */
> +for (i = 0; i < len - 1; ++i) {
> +pattern = ((pattern & 0xff) << 8) + (unsigned char) ptr[i];
> +if (pattern == 0x101 * (unsigned char) ('/')) {
> +return 0;
> +}
> +}
> +
> +return 1;
> +}
> +
>  /* resolve a request URI to a resource descriptor.
>   *
>   * If label_allowed != 0, then allow the request target to be altered by

Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Yann Ylavic]

On Mon, Jan 17, 2022 at 5:10 PM  wrote:
>
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1897156=1897155=1897156=diff
> ==
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Mon Jan 17 16:10:51 2022
> @@ -723,6 +737,57 @@ static int dav_get_overwrite(request_rec
>  return -1;
>  }
>
> +static int uripath_is_canonical(const char *uripath)

Maybe reuse ap_normalize_path() for the implementation, like:

static int uripath_is_canonical(apr_pool_t *p, const char *uripath)
{
char *path = apr_pstrdup(uripath);
return (ap_normalize_path(path, (AP_NORMALIZE_MERGE_SLASHES |
 AP_NORMALIZE_NOT_ABOVE_ROOT))
&& strcmp(uri_path, path) == 0);
}

?


Regards;
Yann.

Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Tue, Jan 18, 2022 at 2:20 PM Ruediger Pluem  wrote:
>
> On 1/18/22 8:54 PM, Yann Ylavic wrote:
> > On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr  
> > wrote:
> >>
> >> Hi Joe, Yann and company,
> >>
> >> please consider this, we will not build against PCRE2 without pcre2-config
> >> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
> >> to it, no hassle.
> >
> > My concern is about the defaut, what happens if no --with-pcre or
> > --with-pcre=yes is specified?
> >
> >>
> >> If someone went to the trouble of installing pcre2, wouldn't we want
> >> to pick that
> >> up, even across a patch release?
> >
> > I think that a lot of systems have both installed (including the -dev
> > packages for building) since pcre2 is the only one supported by some
> > projects now and at least httpd requires pcre1 (until the next
> > release) so it's likely there too already.
> > I haven't looked at the configure part of the patch yet, it's possible
> > that your proposed backport already picks up pcre1 when both are
> > available (which looks the most reasonable to me for 2.4.x), I'm just
> > lazily asking..
>
> I think the default is pcre2:
>
> https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a#file-httpd-2-4-x-pcre2-10-x-patch-L97-L99
>
> Regards
>
> Rüdiger

You are right - pcre2-10.x is the new default of the proposed patch
and the behavior of trunk.

I do understand that RHEL, Ubuntu, all the other packagers are
applying patches on patches even of this abandoned branch.

However the author advises us, it's a bad idea to play with pcre
arrays on stack. The author advises us that there are known
vulnerabilities (known to him) that cannot be fixed on the 8.x branch,
owing to the underlying design. Whether we care, since the flaws are
mostly in the expressions under the control of the admin, and not the
untrusted input against the expressions, I don't know.

Re: httpd test framework svn repo borked?

[Johan Corveleyn]

On Sat, Jan 15, 2022 at 12:56 AM William A Rowe Jr  wrote:
>
> On Fri, Jan 14, 2022 at 4:08 AM Stefan Sperling  wrote:
> >
> > On Thu, Jan 13, 2022 at 07:17:27PM -0600, William A Rowe Jr wrote:
> > > Thanks Stefan, it's attempting [locally] to replace a file, which was
> > > just created during the
> > > checkout (which might even be open).
> >
> > Hmm. I assume SVN would close such files based on APR pool lifetime.
> >
> > Handling of the pristine installation step has been changed in SVN trunk:
> > https://svn.apache.org/r1886490
> > In particular:
> > """
> > On Windows, it uses the existing mechanism of installing the files
> > by handle, without having to reopen them.
> > """
>
> As indicated, this is in the ubuntu 18.04 SFU on windows, the msys2-64
> current distribution, as well as Windows "native". Very consistent.

Was browsing some mails and just saw this thread.

Not sure I'm doing the right thing, but I don't see this issue on my
Windows 10 machine (with standard NTFS filesystem, just your regular
Windows laptop), with a standard 1.14.0 SVN client (from TortoiseSVN).

[[[
C:\research>svn --version -q
1.14.0

C:\research>svn co  -q -r 1896892
https://svn.apache.org/repos/asf/httpd/test/framework/trunk

C:\research>cd trunk

C:\research\trunk>dir /b
Apache-Test
build
c-modules
LICENSE
Makefile.PL
Misc.pm
NOTICE
README
scripts
STATUS
t
]]]

-- 
Johan

Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS

[Ruediger Pluem]

On 1/18/22 8:54 PM, Yann Ylavic wrote:
> On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr  wrote:
>>
>> Hi Joe, Yann and company,
>>
>> please consider this, we will not build against PCRE2 without pcre2-config
>> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
>> to it, no hassle.
> 
> My concern is about the defaut, what happens if no --with-pcre or
> --with-pcre=yes is specified?
> 
>>
>> If someone went to the trouble of installing pcre2, wouldn't we want
>> to pick that
>> up, even across a patch release?
> 
> I think that a lot of systems have both installed (including the -dev
> packages for building) since pcre2 is the only one supported by some
> projects now and at least httpd requires pcre1 (until the next
> release) so it's likely there too already.
> I haven't looked at the configure part of the patch yet, it's possible
> that your proposed backport already picks up pcre1 when both are
> available (which looks the most reasonable to me for 2.4.x), I'm just
> lazily asking..

I think the default is pcre2:

https://gist.github.com/wrowe/73f655d13bbe0f12030aa4557e804d8a#file-httpd-2-4-x-pcre2-10-x-patch-L97-L99

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()

[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: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS

[Yann Ylavic]

On Tue, Jan 18, 2022 at 7:48 PM William A Rowe Jr  wrote:
>
> Hi Joe, Yann and company,
>
> please consider this, we will not build against PCRE2 without pcre2-config
> installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
> to it, no hassle.

My concern is about the defaut, what happens if no --with-pcre or
--with-pcre=yes is specified?

>
> If someone went to the trouble of installing pcre2, wouldn't we want
> to pick that
> up, even across a patch release?

I think that a lot of systems have both installed (including the -dev
packages for building) since pcre2 is the only one supported by some
projects now and at least httpd requires pcre1 (until the next
release) so it's likely there too already.
I haven't looked at the configure part of the patch yet, it's possible
that your proposed backport already picks up pcre1 when both are
available (which looks the most reasonable to me for 2.4.x), I'm just
lazily asking..

Regards;
Yann.

Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Graham Leggett]

On 18 Jan 2022, at 12:31, Ruediger Pluem  wrote:

> Not sure I get the intention correct, but this looks like a change to core 
> for a mod_dav specific short coming.

It's not specific to mod_dav, no.

I created a patch ages ago for mod_dav_svn that depended on this patch and this 
got buried under other things, and I have other modules that need the same 
thing: http://svn.apache.org/viewvc/subversion/branches/mod-dav-svn-expressions/

At the core of the problem is for many modules to work, they need to know the 
“root” of the URL space, so that a PATH_INFO can be accurately calculated. As 
soon as you’re in a LocationMatch or DirectoryMatch the root of the URL space 
is replaced with the regex, and you’re lost.

> e.g. in mod_proxy similar problems are solved for ProxyPass that can be used 
> in and outside Directory / Location elements.

The ProxyPass was originally outside of Directory / Location only, I added the 
option of the one parameter version of ProxyPass that inherited from the 
Directory / Location, but this (as I recall) doesn't work for regexes.

It would be great if the two parameter ProxyPass can go away in future to 
simplify.

> I would be in favor for a similar approach here, by either allowing a second 
> optional parameter to the 'DAV' directive that sets
> this or for a separate directive that allows to set this. In case nothing is 
> set the cmd->path could be used as currently.
> This also allows to use this feature in 'if' sections as well.
> Another option would be to provide this information via an environment 
> variable that can set via SetEnvIfExpr or a RewriteRule.

H…

Adding an extra parameter to DAV, and then to something generic like SetHandler 
may do the trick. This gives most modules access to a sane path when used 
inside a Match.

Let me think about this for a bit.

Regards,
Graham
—

Re: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS

[William A Rowe Jr]

On Mon, Jan 17, 2022 at 6:38 PM Yann Ylavic  wrote:
>
> On Mon, Jan 17, 2022 at 4:05 PM Joe Orton  wrote:
> >
> > For 2.4.x I would argue it's better to keep a preference for 8.x over
> > 10.x so that users aren't switched from one to the other across an
> > upgrade - with some new performance trade-off we know about - without
> > changing the environment/configure line?
>
> +1, we should try to find PCRE1 first in 2.4.x and fall back to PCRE2
> only if PCRE1 is not there.

Hi Joe, Yann and company,

please consider this, we will not build against PCRE2 without pcre2-config
installed from a pcre2-dev package. We find PCRE1 with pcre-config and link
to it, no hassle.

If someone went to the trouble of installing pcre2, wouldn't we want
to pick that
up, even across a patch release?

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: svn commit: r1896976 - /httpd/httpd/branches/2.4.x/STATUS

[Ruediger Pluem]

On 1/17/22 8:35 PM, William A Rowe Jr wrote:
> 
> 
> On Mon, Jan 17, 2022, 09:37 Ruediger Pluem  > wrote:
> 
> 
> 
> On 1/17/22 4:05 PM, Joe Orton wrote:
> > On Sun, Jan 16, 2022 at 03:35:15PM -0600, William A Rowe Jr wrote:
> >> 4 day ago, you all saw this. 6 years ago, you all started using this 
> on trunk.
> >>
> >> Don't know what I can to do help this along and honor the library
> >> author's wishes for all of us to walk away from the dead fork, and use
> >> the maintained fork. It's whatever it is, I'm out of here and removing
> >> the backport application branch, whoever 3rd upvotes this be prepared
> >> to apply this for us all, thanks.
> >
> > I'm fine with PCRE 10.x as a trunk/2.5 feature.  PCRE upstream have
> > maintained 8.x better than e.g. zlib upstream have done in recent years
> > (last zlib release in 2017).  So I don't find the fact it's considered
> > EOL upstream presents any particular urgency, it's still supported
> > downstream on platforms people deploy to.
> >
> > For 2.4.x I would argue it's better to keep a preference for 8.x over
> > 10.x so that users aren't switched from one to the other across an
> > upgrade - with some new performance trade-off we know about - without
> > changing the environment/configure line?
> 
> Sounds sensible for Linux to keep the default to 8.x if found where people
> can expect their distribution to maintain stuff provided that the 
> distribution is still maintained.
> I am not so sure for other platforms especially Windows where I guess 
> that people get stuff
> more often directly from upstream.
> 
> 
> Sensible? Did you read the memo at pcre.org ? There will be 
> no more evaluations of security risks on the
> abandoned fork and we were told this back in May 2021.
> 
> Do you still have the same posture? Some of us spent the last 5 years arguing 
> for httpd.next, and I resigned over it. Your call,

Unless I missed something on the link you provided this just looks like an EOL 
of a version by the upstream project. Many Linux
distributions, especially the LTS ones contain such versions of various 
products and promise to maintain them for the distribution
until the support for the distribution version stops. e.g. RedHat 7 still ships 
openssl 1.0.2 which is not supported upstream any
longer, but still receives updates in RedHat 7 until its regular support ends 
in 2024.
So my posture is still the same. But I want to thank you for making this 
backport proposal for which I already voted as it brought
up to our attention that 2.4.x users which cannot / do not want to use the old 
PCRE version for various reasons (e.g. no one
providing support for it, for the particular platform they use, not wanting to 
use such LTS distribution provided versions, etc.)
had no choice to use the upstream supported version. Once the backport comes in 
they have that possibility which is great.

Regards

Rüdiger

Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Ruediger Pluem]

On 1/17/22 5:10 PM, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Jan 17 16:10:51 2022
> New Revision: 1897156
> 
> URL: http://svn.apache.org/viewvc?rev=1897156=rev
> Log:
> core: Allow an optional expression to be specified for an effective
> path in the DirectoryMatch and LocationMatch directives. This allows
> modules like mod_dav to map URLs to URL spaces or to directories on
> the filesystem.
> 
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/docs/manual/mod/core.xml
> httpd/httpd/trunk/modules/dav/main/mod_dav.c
> httpd/httpd/trunk/server/core.c
> 

Not sure I get the intention correct, but this looks like a change to core for 
a mod_dav specific short coming.
e.g. in mod_proxy similar problems are solved for ProxyPass that can be used in 
and outside Directory / Location elements.
I would be in favor for a similar approach here, by either allowing a second 
optional parameter to the 'DAV' directive that sets
this or for a separate directive that allows to set this. In case nothing is 
set the cmd->path could be used as currently.
This also allows to use this feature in 'if' sections as well.
Another option would be to provide this information via an environment variable 
that can set via SetEnvIfExpr or a RewriteRule.

Regards

Rüdiger

Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Joe Orton]

On Mon, Jan 17, 2022 at 04:10:51PM -, minf...@apache.org wrote:
> Author: minfrin
> Date: Mon Jan 17 16:10:51 2022
> New Revision: 1897156
> 
> URL: http://svn.apache.org/viewvc?rev=1897156=rev
> Log:
> core: Allow an optional expression to be specified for an effective
> path in the DirectoryMatch and LocationMatch directives. This allows
> modules like mod_dav to map URLs to URL spaces or to directories on
> the filesystem.

https://app.travis-ci.com/github/apache/httpd/jobs/555883817#L2039

This has introduced new warnings:

In file included from mod_dav.c:51:
mod_dav.c: In function ‘uripath_is_canonical’:
mod_dav.c:774:38: error: passing argument 1 of ‘ap_strchr’ discards ‘const’ 
qualifier from pointer target type [-Werror=discarded-qualifiers]
  774 | dot_pos = strchr(dot_pos + 1, '.')) {
  |  ^~~
/home/travis/build/apache/httpd/include/httpd.h:2469:34: note: in definition of 
macro ‘strchr’
 2469 | # define strchr(s, c)  ap_strchr(s,c)
  |  ^
/home/travis/build/apache/httpd/include/httpd.h:2457:36: note: expected ‘char 
*’ but argument is of type ‘const char *’
 2457 | AP_DECLARE(char *) ap_strchr(char *s, int c);
  |  ~~^