Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c

2021-09-06 Thread Ruediger Pluem



On 9/6/21 12:28 PM, Yann Ylavic wrote:
> On Mon, Sep 6, 2021 at 12:16 PM Ruediger Pluem  wrote:
>>
  !ap_cstr_casecmpn(uds_url, "unix:", 5) &&
>>
>> Why doing case insensitive checks here? Shouldn't we insist on case here and 
>> use strncmp ?
> 
> Yes, correct, spurious change.
> 

>>> +{
>>> +apr_table_setn(r->notes, "uds_path", uds_path);
>>> +
>>> +/* Remove the UDS path from *url and r->filename */
>>> +url_len = strlen(origin_url);
>>> +*url = apr_pstrmemdup(r->pool, origin_url, url_len);
>>> +memcpy(uds_url, *url, url_len + 1);
>>
>> With a short uds path and a long origin_url couldn't this be overlapping?
>> I think memcpy is unsafe with overlapping memory and we should stay with 
>> memmove.
> 
> *url is newly allocated here, so there is no possible overlap with the
> initial r->filename.

Good point. I missed this.

Regards

Rüdiger



Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c

2021-09-06 Thread Yann Ylavic
On Mon, Sep 6, 2021 at 12:16 PM Ruediger Pluem  wrote:
>
> On 9/6/21 11:38 AM, Yann Ylavic wrote:
> > Index: modules/proxy/proxy_util.c
> > ===
> > --- modules/proxy/proxy_util.c(revision 1892971)
> > +++ modules/proxy/proxy_util.c(working copy)
> > @@ -2268,33 +2268,45 @@ static int ap_proxy_retry_worker(const char *proxy
> >   * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
> >   * as required.
> >   */
> > -static void fix_uds_filename(request_rec *r, char **url)
> > +static int fix_uds_filename(request_rec *r, char **url)
> >  {
> > -char *ptr, *ptr2;
> > -if (!r || !r->filename) return;
> > +char *uds_url = r->filename + 6, *origin_url;
> >
> > -if (!strncmp(r->filename, "proxy:", 6) &&
> > -!ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> > -(ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
> > +if (!ap_cstr_casecmpn(r->filename, "proxy:", 6) &&
> > +!ap_cstr_casecmpn(uds_url, "unix:", 5) &&
>
> Why doing case insensitive checks here? Shouldn't we insist on case here and 
> use strncmp ?

Yes, correct, spurious change.

>
> > +(origin_url = ap_strchr(uds_url + 5, '|'))) {
> > +char *uds_path = NULL;
> > +apr_size_t url_len;
> >  apr_uri_t urisock;
> >  apr_status_t rv;
> > -*ptr = '\0';
> > -rv = apr_uri_parse(r->pool, ptr2, );
> > -if (rv == APR_SUCCESS) {
> > -char *rurl = ptr+1;
> > -char *sockpath = ap_runtime_dir_relative(r->pool, 
> > urisock.path);
> > -apr_table_setn(r->notes, "uds_path", sockpath);
> > -*url = apr_pstrdup(r->pool, rurl); /* so we get the scheme for 
> > the uds */
> > -/* r->filename starts w/ "proxy:", so add after that */
> > -memmove(r->filename+6, rurl, strlen(rurl)+1);
> > +
> > +*origin_url = '\0';
> > +rv = apr_uri_parse(r->pool, uds_url, );
> > +*origin_url++ = '|';
> > +
> > +if (rv == APR_SUCCESS && urisock.path && !urisock.hostname) {
> > +uds_path = ap_runtime_dir_relative(r->pool, urisock.path);
> > +}
> > +if (!uds_path) {
> > +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> > +"Invalid proxy UDS filename (%s)",
> > +r->filename);
> > +return 0;
> > +}
> > +{
> > +apr_table_setn(r->notes, "uds_path", uds_path);
> > +
> > +/* Remove the UDS path from *url and r->filename */
> > +url_len = strlen(origin_url);
> > +*url = apr_pstrmemdup(r->pool, origin_url, url_len);
> > +memcpy(uds_url, *url, url_len + 1);
>
> With a short uds path and a long origin_url couldn't this be overlapping?
> I think memcpy is unsafe with overlapping memory and we should stay with 
> memmove.

*url is newly allocated here, so there is no possible overlap with the
initial r->filename.


Regards;
Yann.


Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c

2021-09-06 Thread Ruediger Pluem



On 9/6/21 11:38 AM, Yann Ylavic wrote:
> Index: modules/proxy/proxy_util.c
> ===
> --- modules/proxy/proxy_util.c(revision 1892971)
> +++ modules/proxy/proxy_util.c(working copy)
> @@ -2268,33 +2268,45 @@ static int ap_proxy_retry_worker(const char *proxy
>   * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
>   * as required.  
>   */
> -static void fix_uds_filename(request_rec *r, char **url) 
> +static int fix_uds_filename(request_rec *r, char **url) 
>  {
> -char *ptr, *ptr2;
> -if (!r || !r->filename) return;
> +char *uds_url = r->filename + 6, *origin_url;
>  
> -if (!strncmp(r->filename, "proxy:", 6) &&
> -!ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> -(ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
> +if (!ap_cstr_casecmpn(r->filename, "proxy:", 6) &&
> +!ap_cstr_casecmpn(uds_url, "unix:", 5) &&

Why doing case insensitive checks here? Shouldn't we insist on case here and 
use strncmp ?

> +(origin_url = ap_strchr(uds_url + 5, '|'))) {
> +char *uds_path = NULL;
> +apr_size_t url_len;
>  apr_uri_t urisock;
>  apr_status_t rv;
> -*ptr = '\0';
> -rv = apr_uri_parse(r->pool, ptr2, );
> -if (rv == APR_SUCCESS) {
> -char *rurl = ptr+1;
> -char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);
> -apr_table_setn(r->notes, "uds_path", sockpath);
> -*url = apr_pstrdup(r->pool, rurl); /* so we get the scheme for 
> the uds */
> -/* r->filename starts w/ "proxy:", so add after that */
> -memmove(r->filename+6, rurl, strlen(rurl)+1);
> +
> +*origin_url = '\0';
> +rv = apr_uri_parse(r->pool, uds_url, );
> +*origin_url++ = '|';
> +
> +if (rv == APR_SUCCESS && urisock.path && !urisock.hostname) {
> +uds_path = ap_runtime_dir_relative(r->pool, urisock.path);
> +}
> +if (!uds_path) {
> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> +"Invalid proxy UDS filename (%s)",
> +r->filename);
> +return 0;
> +}
> +{
> +apr_table_setn(r->notes, "uds_path", uds_path);
> +
> +/* Remove the UDS path from *url and r->filename */
> +url_len = strlen(origin_url);
> +*url = apr_pstrmemdup(r->pool, origin_url, url_len);
> +memcpy(uds_url, *url, url_len + 1);

With a short uds path and a long origin_url couldn't this be overlapping?
I think memcpy is unsafe with overlapping memory and we should stay with 
memmove.

> +
>  ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
>  "*: rewrite of url due to UDS(%s): %s (%s)",
> -sockpath, *url, r->filename);
> +uds_path, *url, r->filename);
>  }
> -else {
> -*ptr = '|';
> -}
>  }
> +return 1;
>  }
>  

Regards

Rüdiger



Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c

2021-09-06 Thread Yann Ylavic
On Fri, Sep 3, 2021 at 10:11 PM Ruediger Pluem  wrote:
>
>
>
> On 9/3/21 6:52 PM, yla...@apache.org wrote:
> >
> >  if (!strncmp(r->filename, "proxy:", 6) &&
> > -(ptr2 = ap_strcasestr(r->filename, "unix:")) &&
> > -(ptr = ap_strchr(ptr2, '|'))) {
> > +!ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> > +(ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
>
> I know that I voted for it to be backported, but after rethinking this 
> shouldn't this be
>
> (ptr2 = r->filename + 6, ptr = ap_strchr(ptr2 + 5, '|')))
>
> as ptr2 is later used in
>
> rv = apr_uri_parse(r->pool, ptr2, );
>
> With the old code and with my proposal above ptr2 points to the 'u' of 
> 'unix:' with the current code it points to the char after
> 'unix:' likely //.

An UDS url in r->filename should be of the form
"proxy:unix:/path/to/socket|https://www.example.net/...;, configured
either from a ProxyPass, a SetHandler or a RewriteRule [P]. So ptr2
should point to a path (absolute or relative to ServerRoot), not an
authority starting with "//".

But if we want a better validation at fix_uds_filename() level, maybe
something like the attached patch could do?

Regards;
Yann.
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c	(revision 1892971)
+++ modules/proxy/proxy_util.c	(working copy)
@@ -2268,33 +2268,45 @@ static int ap_proxy_retry_worker(const char *proxy
  * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
  * as required.  
  */
-static void fix_uds_filename(request_rec *r, char **url) 
+static int fix_uds_filename(request_rec *r, char **url) 
 {
-char *ptr, *ptr2;
-if (!r || !r->filename) return;
+char *uds_url = r->filename + 6, *origin_url;
 
-if (!strncmp(r->filename, "proxy:", 6) &&
-!ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
-(ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
+if (!ap_cstr_casecmpn(r->filename, "proxy:", 6) &&
+!ap_cstr_casecmpn(uds_url, "unix:", 5) &&
+(origin_url = ap_strchr(uds_url + 5, '|'))) {
+char *uds_path = NULL;
+apr_size_t url_len;
 apr_uri_t urisock;
 apr_status_t rv;
-*ptr = '\0';
-rv = apr_uri_parse(r->pool, ptr2, );
-if (rv == APR_SUCCESS) {
-char *rurl = ptr+1;
-char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);
-apr_table_setn(r->notes, "uds_path", sockpath);
-*url = apr_pstrdup(r->pool, rurl); /* so we get the scheme for the uds */
-/* r->filename starts w/ "proxy:", so add after that */
-memmove(r->filename+6, rurl, strlen(rurl)+1);
+
+*origin_url = '\0';
+rv = apr_uri_parse(r->pool, uds_url, );
+*origin_url++ = '|';
+
+if (rv == APR_SUCCESS && urisock.path && !urisock.hostname) {
+uds_path = ap_runtime_dir_relative(r->pool, urisock.path);
+}
+if (!uds_path) {
+ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+"Invalid proxy UDS filename (%s)",
+r->filename);
+return 0;
+}
+{
+apr_table_setn(r->notes, "uds_path", uds_path);
+
+/* Remove the UDS path from *url and r->filename */
+url_len = strlen(origin_url);
+*url = apr_pstrmemdup(r->pool, origin_url, url_len);
+memcpy(uds_url, *url, url_len + 1);
+
 ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
 "*: rewrite of url due to UDS(%s): %s (%s)",
-sockpath, *url, r->filename);
+uds_path, *url, r->filename);
 }
-else {
-*ptr = '|';
-}
 }
+return 1;
 }
 
 PROXY_DECLARE(int) ap_proxy_pre_request(proxy_worker **worker,
@@ -2312,7 +2324,9 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
   "%s: found worker %s for %s",
   (*worker)->s->scheme, (*worker)->s->name, *url);
 *balancer = NULL;
-fix_uds_filename(r, url);
+if (!fix_uds_filename(r, url)) {
+return HTTP_INTERNAL_SERVER_ERROR;
+}
 access_status = OK;
 }
 else if (r->proxyreq == PROXYREQ_PROXY) {
@@ -2343,7 +2357,9 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
  * regarding the Connection header in the request.
  */
 apr_table_setn(r->subprocess_env, "proxy-nokeepalive", "1");
-fix_uds_filename(r, url);
+if (!fix_uds_filename(r, url)) {
+return HTTP_INTERNAL_SERVER_ERROR;
+}
 }
 }
 }


Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c

2021-09-03 Thread Ruediger Pluem



On 9/3/21 6:52 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Fri Sep  3 16:52:38 2021
> New Revision: 1892874
> 
> URL: http://svn.apache.org/viewvc?rev=1892874=rev
> Log:
> Merge r1892814, r1892853 from trunk:
> 
> mod_proxy: Faster unix socket path parsing in the "proxy:" URL.
> 
> The actual r->filename format is "[proxy:]unix:path|url" for UDS, no need to
> strstr(,"unix:") since it's at the start of the string.
> 
> 
> mod_proxy: Follow up to r1892814.
> 
> Save some few cycles in ap_proxy_de_socketfy() too.
> 
> 
> Submitted by: ylavic
> Reviewed by: ylavic, covener, rpluem
> 
> Added:
> httpd/httpd/branches/2.4.x/changes-entries/fix_uds_filename.txt
>   - copied unchanged from r1892814, 
> httpd/httpd/trunk/changes-entries/fix_uds_filename.txt
> Modified:
> httpd/httpd/branches/2.4.x/   (props changed)
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> 
> Propchange: httpd/httpd/branches/2.4.x/
> --
>   Merged /httpd/httpd/trunk:r1892814,1892853
> 
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c?rev=1892874=1892873=1892874=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c Fri Sep  3 16:52:38 
> 2021
> @@ -1975,7 +1975,7 @@ PROXY_DECLARE(const char *) ap_proxy_de_
>   * the UDS path... ignore it
>   */
>  if (!ap_cstr_casecmpn(url, "unix:", 5) &&
> -((ptr = ap_strchr_c(url, '|')) != NULL)) {
> +((ptr = ap_strchr_c(url + 5, '|')) != NULL)) {
>  /* move past the 'unix:...|' UDS path info */
>  const char *ret, *c;
>  
> 
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1892874=1892873=1892874=diff
> ==
> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Sep  3 16:52:38 
> 2021
> @@ -2281,8 +2281,8 @@ static void fix_uds_filename(request_rec
>  if (!r || !r->filename) return;
>  
>  if (!strncmp(r->filename, "proxy:", 6) &&
> -(ptr2 = ap_strcasestr(r->filename, "unix:")) &&
> -(ptr = ap_strchr(ptr2, '|'))) {
> +!ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> +(ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {

I know that I voted for it to be backported, but after rethinking this 
shouldn't this be

(ptr2 = r->filename + 6, ptr = ap_strchr(ptr2 + 5, '|')))

as ptr2 is later used in

rv = apr_uri_parse(r->pool, ptr2, );

With the old code and with my proposal above ptr2 points to the 'u' of 'unix:' 
with the current code it points to the char after
'unix:' likely //.

Regards

Rüdiger