Forgive me for summarizing, but I didn’t come here expecting help, much less collaboration on a solution. I came here expecting to be scolded for having the temerity to critique the quality of the patch sets you’ve been shipping of late In libapreq2. None of my opinions have changed on that subject, and won’t for some time.
The point is I’m part of your extended support channels for libapreq2, because it’s easy and fun to help people who use the code. I’m not here to complain, I’m here expecting more empathy for people like me who give their time graciously without expecting anything in return other than some measure of respect for the effort involved. On Sat, Oct 29, 2022 at 9:59 PM Joe Schaefer <j...@sunstarsys.com> wrote: > 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> > > > -- Joe Schaefer, Ph.D. We only build what you need built. <j...@sunstarsys.com> 954.253.3732 <//954.253.3732>
