Missed one. The patch that introduced these changes was revision=1895107.
On Sat, Oct 29, 2022 at 9:15 PM Joe Schaefer <j...@sunstarsys.com> wrote:
> Of course, I don’t know how to advise you regarding the security aspects,
> since you’re doing what you thought was the right thing to do and put the
> mfd parser into an error state instead of leaving well enough alone. But
> basically libapreq2 users get annoyed when the parser breaks on valid
> input, and may get antsy when their server goes bonkers because they aren’t
> in the habit of doing error handling on this condition.
>
> On Sat, Oct 29, 2022 at 8:36 PM Joe Schaefer <j...@sunstarsys.com> wrote:
>
>> Found the problem: it's just a misunderstanding about what is admissible
>> in a successful file upload widget.
>> If someone doesn't add a file to the upload widget, it is still a
>> successful control and should be processed as such on the server.
>> In this case, just like with opera, the filename attribute will be
>> present, but set to an empty double-quoted string.
>>
>> Here's my patchset, enjoy.
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> On Sat, Oct 29, 2022 at 2:47 PM Joe Schaefer <j...@sunstarsys.com> wrote:
>>
>>> Curiously, this doesn't seem to present any problems for
>>> apreq_header_attribute in trunk/HEAD. A good thing.
>>>
>>> That means we may need to look more closely at r1903484 in glue/perl.
>>>
>>> On Sat, Oct 29, 2022 at 2:12 PM Joe Schaefer <j...@sunstarsys.com> wrote:
>>>
>>>>
>>>> On Sat, Oct 29, 2022 at 1:16 PM Joe Schaefer <j...@sunstarsys.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Sat, Oct 29, 2022 at 1:00 PM Yann Ylavic <ylavic....@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Joe,
>>>>>>
>>>>>> here comes the "goofer".
>>>>>>
>>>>>> On Fri, Oct 28, 2022 at 9:05 PM <j...@sunstarsys.com> wrote:
>>>>>> >
>>>>>> > Long time fan, not a first time caller.
>>>>>>
>>>>>> Yet what a crappy thread (and comments on [1]).
>>>>>> All top posting, unreplyable.
>>>>>>
>>>>>> >
>>>>>> > Libapreq2 was intended to be a safe,fast, standards compliant
>>>>>> library- primarily *safe* before all other priorities. Some of the work
>>>>>> going on lately in util.c is starting to undermine that prime directive,
>>>>>> so
>>>>>> I’d like to better understand why these changes are happening, and why
>>>>>> they
>>>>>> are snowballing into a less functional, less secure software product that
>>>>>> is driving up my support costs on CPAN.
>>>>>>
>>>>>> Yeah sure, rewriting history. That marvelous previous 2.16 just
>>>>>> exploded when faced with google's oss-fuzzers (and not just a little,
>>>>>> quite some reports) which now fuzz httpd trunk (thus apreq).
>>>>>> CVE-2022-22728 is about libapreq2 v2.16 *and earlier" right? So
>>>>>> something pre-dated my changes.
>>>>>
>>>>>
>>>>> Fair enough.
>>>>>
>>>>>
>>>>>>
>>>>> >
>>>>>> > For instance, this revision 1867789 is a pure pessimization: it
>>>>>> trades userland RAM for filesystem cache RAM, that’s it, but it’s not a
>>>>>> big
>>>>>> deal. Just churn.
>>>>>>
>>>>>> I call it a fix for an UAF (Use After Free). This is my only change in
>>>>>> 2.16 btw, while you seem to suggest that security issues started with
>>>>>> 2.16.
>>>>>>
>>>>>> >
>>>>>> > Everything in the crufty, old apreq_header_attribute code I wrote
>>>>>> was completely tossed and reimplemented. Why?
>>>>>>
>>>>>> Someone had to address the security reports, and someone (me) dared
>>>>>> touching your code because it was not safe (i.e.
>>>>>> broken/crashing/vulnerable/..), not for the lulz nor breaking users.
>>>>>> I'm very sorry if that happened, only those who do nothing do not
>>>>>> break anything though.
>>>>>> Existing tests were still passing, but shit happens.
>>>>>>
>>>>>
>>>>> Then lets deal with it by adding more tests.
>>>>>
>>>>>
>>>>>>
>>>>>> > We’re just racking up CVE’s, people are disabling the mfd parser
>>>>>> altogether, and it no longer support common use cases that people now
>>>>>> complain about because it supported cases in the wild that the new work
>>>>>> does not.
>>>>>>
>>>>>> Are there multiple issues? I know of the one reported in [1] about
>>>>>> "file upload does not work if any file fields are blank".
>>>>>> That's not actionable sorry (I don't understand what it means), no
>>>>>> more than your rant here and elusive "hints" on where/how to fix it.
>>>>>> I asked in the other thread for a reproducer in the form of a HTTP
>>>>>> payload, not a mod_perl handler which I don't know how to debug (let
>>>>>> alone without the right thing to send on the client side).
>>>>>>
>>>>>>
>>>>> I translated the bug report for you. It involves browsers like Opera
>>>>> that send filename=""
>>>>> attributes in the Content-Disposition header. It's generating an
>>>>> accidental DoS, depending
>>>>> on how people use the upload API. Toss in some tests into util.t and
>>>>> I'll add this one for you.
>>>>>
>>>>>
>>>>>> >
>>>>>> > With the latest code coming out of p5p for Perl, there’s a whole
>>>>>> new reason for excitement in httpd-land: the mod_perl2 + mpm_event
>>>>>> combination is rock solid and screaming fast with HTTP/2. The only
>>>>>> reason
>>>>>> I resubbed here is in the hopes of some synergy retaking these
>>>>>> perl-related
>>>>>> projects, since mod_perl2 is the only game in town for embedded
>>>>>> interpreters in httpd2 (and no, lua is not the answer, it’s not thread
>>>>>> safe
>>>>>> either).
>>>>>>
>>>>>> Synergy! What a great intro..
>>>>>>
>>>>>>
>>>>>> Regards;
>>>>>> Yann.
>>>>>>
>>>>>> [1] https://rt.cpan.org/Public/Bug/Display.html?id=144470
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Joe Schaefer, Ph.D.
>>>>> We only build what you need built.
>>>>> <j...@sunstarsys.com>
>>>>> 954.253.3732 <//954.253.3732>
>>>>>
>>>>>
>>>>>
>>>>
>>>> Let's start with this (untested) patch...
>>>>
>>>>
>>>> Index: library/t/util.c
>>>> ===================================================================
>>>> --- library/t/util.c (revision 1904922)
>>>> +++ library/t/util.c (working copy)
>>>> @@ -271,6 +271,7 @@
>>>> static void test_header_attribute(dAT, void *ctx)
>>>> {
>>>> const char hdr[] = "name=\"filename=foo\"; filename=\"quux.txt\"";
>>>> + const char opera[] = "name=\"foo\"; filename=\"\"";
>>>> const char *val;
>>>> apr_size_t vlen;
>>>>
>>>> @@ -284,6 +285,10 @@
>>>> AT_int_eq(vlen, 8);
>>>> AT_mem_eq("quux.txt", val, 8);
>>>>
>>>> + AT_int_eq(apreq_header_attribute(opera, "filename" 8, &val, &vlen),
>>>> + APR_SUCCESS);
>>>> + AT_int_eq(vlen,0);
>>>> +
>>>> }
>>>>
>>>> static void test_brigade_concat(dAT, void *ctx)
>>>> @@ -315,7 +320,7 @@
>>>> { dT(test_join, 0) },
>>>> { dT(test_brigade_fwrite, 0) },
>>>> { dT(test_file_mktemp, 0) },
>>>> - { dT(test_header_attribute, 6) },
>>>> + { dT(test_header_attribute, 8) },
>>>> { dT(test_brigade_concat, 0) },
>>>> };
>>>> --
>>>> Joe Schaefer, Ph.D.
>>>> We only build what you need built.
>>>> <j...@sunstarsys.com>
>>>> 954.253.3732 <//954.253.3732>
>>>>
>>>>
>>>>
>>>
>>> --
>>> Joe Schaefer, Ph.D.
>>> We only build what you need built.
>>> <j...@sunstarsys.com>
>>> 954.253.3732 <//954.253.3732>
>>>
>>>
>>>
>>
>> --
>> Joe Schaefer, Ph.D.
>> We only build what you need built.
>> <j...@sunstarsys.com>
>> 954.253.3732 <//954.253.3732>
>>
>>
>> --
> Joe Schaefer, Ph.D.
> We only build what you need built.
> <j...@sunstarsys.com>
> 954.253.3732 <//954.253.3732>
>
>
>
--
Joe Schaefer, Ph.D.
We only build what you need built.
<j...@sunstarsys.com>
954.253.3732 <//954.253.3732>
Index: library/parser_multipart.c
===================================================================
--- library/parser_multipart.c (revision 1904922)
+++ library/parser_multipart.c (working copy)
@@ -425,7 +425,7 @@
if (cd != NULL) {
s = apreq_header_attribute(cd, "name", 4,
&name, &nlen);
- if (s == APR_SUCCESS && nlen) {
+ if (s == APR_SUCCESS) {
next_ctx->param_name = apr_pstrmemdup(pool, name,
nlen);
}
@@ -467,7 +467,7 @@
s = apreq_header_attribute(cd, "filename",
8, &filename, &flen);
- if (s == APR_SUCCESS && flen) {
+ if (s == APR_SUCCESS) {
apreq_param_t *param;
param = apreq_param_make(pool, name, nlen,
@@ -497,7 +497,7 @@
s = apreq_header_attribute(cd, "filename",
8, &filename, &flen);
- if (s != APR_SUCCESS || !flen || !ctx->param_name) {
+ if (s != APR_SUCCESS || !ctx->param_name) {
ctx->status = MFD_ERROR;
goto mfd_parse_brigade;
}
Index: library/t/util.c
===================================================================
--- library/t/util.c (revision 1904922)
+++ library/t/util.c (working copy)
@@ -270,7 +270,8 @@
static void test_header_attribute(dAT, void *ctx)
{
- const char hdr[] = "name=\"filename=foo\"; filename=\"quux.txt\"";
+ const char hdr[] = "form-data; name=\"filename=foo\";
filename=\"quux.txt\"";
+ const char opera[] = "form-data; name=\"foo\"; filename=\"\"";
const char *val;
apr_size_t vlen;
@@ -284,6 +285,14 @@
AT_int_eq(vlen, 8);
AT_mem_eq("quux.txt", val, 8);
+ AT_int_eq(apreq_header_attribute(opera, "filename", 8, &val, &vlen),
+ APR_SUCCESS);
+ AT_int_eq(vlen,0);
+
+ AT_int_eq(apreq_header_attribute(opera, "name", 4, &val, &vlen),
+ APR_SUCCESS);
+ AT_int_eq(vlen,3);
+
}
static void test_brigade_concat(dAT, void *ctx)
@@ -315,7 +324,7 @@
{ dT(test_join, 0) },
{ dT(test_brigade_fwrite, 0) },
{ dT(test_file_mktemp, 0) },
- { dT(test_header_attribute, 6) },
+ { dT(test_header_attribute, 10) },
{ dT(test_brigade_concat, 0) },
};
