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>
