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>
=================================================================== --- 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) }, };