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 <[email protected]> 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 <[email protected]> wrote:
>
>>
>> On Sat, Oct 29, 2022 at 1:16 PM Joe Schaefer <[email protected]> wrote:
>>
>>>
>>>
>>> On Sat, Oct 29, 2022 at 1:00 PM Yann Ylavic <[email protected]>
>>> wrote:
>>>
>>>> Hi Joe,
>>>>
>>>> here comes the "goofer".
>>>>
>>>> On Fri, Oct 28, 2022 at 9:05 PM <[email protected]> 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.
>>> <[email protected]>
>>> 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.
>> <[email protected]>
>> 954.253.3732 <//954.253.3732>
>>
>>
>>
>
> --
> Joe Schaefer, Ph.D.
> We only build what you need built.
> <[email protected]>
> 954.253.3732 <//954.253.3732>
>
>
>
--
Joe Schaefer, Ph.D.
We only build what you need built.
<[email protected]>
954.253.3732 <//954.253.3732>
===================================================================
--- 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,
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) },
};
