Was this a bug in APR from 2008?

[Roman Garcia <ro...@gmail.com>]

Hi, sorry for the click-bait.

Back in 2008 we notified Apache HTTP Server team about this
<https://bz.apache.org/bugzilla/show_bug.cgi?id=45187> issue.

Back then, I didn't realize this was (maybe, I'm assuming now) both an APR
issue as an HTTPD issue.

What we initially found was a HTTPD was returning FORBIDDEN for long URLs.
Looking at the underlying code, we found HTTPD wasn't receiving context for
too-long file paths.

We did a change in APR to handle MAX_PATH cases on *NIX systems (it was
already there in WIN32). Then we were able to add handling for those cases
on HTTPD.

Without a minimal idea of what APR stands for, or even its status regarding
maintainance, I would imagine both *NIX and WIN32 any other OS should
handle the file-path-too-long, and in the case of OSes like Linux you could
go as far as to which FS manages the file to handle different limits.

Would you guys agree the patch for APR is a valid one? Probably incomplete?

Regards
Roman

Re: Was this a bug in APR from 2008?

[William A Rowe Jr <wr...@rowe-clan.net>]

On Fri, Nov 30, 2018 at 10:18 AM Eric Covener <co...@gmail.com> wrote:

> On Fri, Nov 30, 2018 at 11:00 AM William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
> >
> > The comment here makes no sense (unix, not windows). But the patch
> itself seems reasonable. There is a performance hit, but nothing compared
> to the call into stat/lstat. Other's opinions?
>
> Seems risky from regression POV.  Safer to map errno of ENAMETOOLONG
> to the APR_ENAMETOOLONG, in case APR_NAMEMAX is lower than actual
> limit at runtime.
>

Which is the current implementation;

/** @see APR_STATUS_IS_ENAMETOOLONG */
#ifdef ENAMETOOLONG
#define APR_ENAMETOOLONG ENAMETOOLONG
#else
#define APR_ENAMETOOLONG   (APR_OS_START_CANONERR + 3)
#endif

On Win32, in theory we can have longer 32k character file paths, but these
were capped at 8k out of buffer space and other practical considerations.
(This might be something to revisit in the future.) This correlates to your
concern about new (regressive) limitations applied to Unix, in that we
started with a saner limit and would only expand it, later on.

For the time being, I'm happy if we drop this submission, unless it is shown
that [l]stat on specific unix platforms will errors other than ENAMETOOLONG
in response to such calls. My quick review of linux suggests it is fine
as-is,
as you suggested above.

Not sure about httpd patch though, IIUC it only helps a faulty config
> or module (dirwalk happening for long URI that won't actually be
> served off disk)
>

But it is well understood that we can't remove the filesystem module,
because it isn't modularized. I'm also not entirely comfortable with the
httpd patch, but the not-found reaction isn't nonsensical (it isn't a file,
so how would other modules like to behave?)

Re: Was this a bug in APR from 2008?

[Eric Covener <co...@gmail.com>]

On Fri, Nov 30, 2018 at 11:00 AM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> The comment here makes no sense (unix, not windows). But the patch itself seems reasonable. There is a performance hit, but nothing compared to the call into stat/lstat. Other's opinions?

Seems risky from regression POV.  Safer to map errno of ENAMETOOLONG
to the APR_ENAMETOOLONG, in case APR_NAMEMAX is lower than actual
limit at runtime.

Not sure about httpd patch though, IIUC it only helps a faulty config
or module (dirwalk happening for long URI that won't actually be
served off disk)

Re: Was this a bug in APR from 2008?

[William A Rowe Jr <wr...@rowe-clan.net>]

The comment here makes no sense (unix, not windows). But the patch itself
seems reasonable. There is a performance hit, but nothing compared to the
call into stat/lstat. Other's opinions?

--- ./srclib/apr/file_io/unix/filestat.c	2007-10-17 00:36:49.000000000 -0300
+++ ./srclib/apr/file_io/unix/filestat.c	2008-06-11 22:26:50.000000000 -0300
@@ -279,6 +279,14 @@
     struct_stat info;
     int srv;

+    /* Catch fname length == MAX_PATH since GetFileAttributesEx fails
+     * with PATH_NOT_FOUND.  We would rather indicate length error than
+     * 'not found'
+     */
+    if (strlen(fname) >= APR_PATH_MAX) {
+        return APR_ENAMETOOLONG;
+    }
+
     if (wanted & APR_FINFO_LINK)
         srv = lstat(fname, &info);
     else


On Thu, Nov 29, 2018 at 2:34 PM Roman Garcia <ro...@gmail.com> wrote:

> Hi, sorry for the click-bait.
>
> Back in 2008 we notified Apache HTTP Server team about this
> <https://bz.apache.org/bugzilla/show_bug.cgi?id=45187> issue.
>
> Back then, I didn't realize this was (maybe, I'm assuming now) both an APR
> issue as an HTTPD issue.
>
> What we initially found was a HTTPD was returning FORBIDDEN for long URLs.
> Looking at the underlying code, we found HTTPD wasn't receiving context for
> too-long file paths.
>
> We did a change in APR to handle MAX_PATH cases on *NIX systems (it was
> already there in WIN32). Then we were able to add handling for those cases
> on HTTPD.
>
> Without a minimal idea of what APR stands for, or even its status
> regarding maintainance, I would imagine both *NIX and WIN32 any other OS
> should handle the file-path-too-long, and in the case of OSes like Linux
> you could go as far as to which FS manages the file to handle different
> limits.
>
> Would you guys agree the patch for APR is a valid one? Probably incomplete?
>
> Regards
> Roman
>
>