SA Bugzilla – Bug 6664
check_freemail_header() misses many domains
Last modified: 2018-02-23 18:18:26 UTC
Created attachment 4972 [details] Patch to Freemail.pm to catch freemail forgeries FREEMAIL_FORGED_REPLYTO is missing about 50% of potential hits, because the Reply-To address passed to _is_freemail() is usually terminated with a chevron and/or newline. As a result it only matches the regexes ending .*. This is because of a Perl programming error. What is intended is: @@ -419,7 +423,7 @@ } } - my $email = lc($pms->get(index($header,':') ? $header : $header.":addr")); + my $email = lc($pms->get(index($header,':') >= 0 ? $header : $header.":addr")); if ($email eq '') { dbg("header $header not found from mail"); However, there are further issues I'd suggest fixing at the same time. Firstly, a spammer wanting a reply to a freemail address might include it as one of *multiple* addresses in a Reply-To header. Hence, each should be tested for freemail and compared to the From. Secondly, by adding an optional parameter for a header to compare to, FREEMAIL_FORGED_REPLYTO could be made quite versatile and catch more freemail spam in the first instance then FREEMAIL_REPLYTO (excluding lists and annoying anomalies like Linkedin in the rules); also FREEMAIL_REPLYTO_END_DIGIT could lose the FPs where From and Reply-To are equal (eg in a personalised Mailman list); and various other combinations testing (X-)Sender and Errors-To against From become possible. (I've tested the variant rules against a live stream and would like to submit them for mass testing and scoring in a separate bug.)
Created attachment 4973 [details] Test case that intended code will miss
Created attachment 4974 [details] Easier test case caught by either complete or partial fix
(BTW I have signed a CLA as listed at http://people.apache.org/committer-index.html#unlistedclas)
(In reply to comment #3) > (BTW I have signed a CLA as listed at > http://people.apache.org/committer-index.html#unlistedclas) CLA status should be up to date. Thanks.
> As a result it only matches the regexes ending .*. > This is because of a Perl programming error. A bug indeed, index() returns -1 on a failure. Where does the $hdrexclude in the patch come from? Should be declared, defined and documented.
Bug is a duplicated of bz #6871 and fixed in svn r1416457. Enhancement is not documented and is lying for 6 years. Is somebody interested in this enhancement or we should close the bz ?
I think it was just missed because of the duplicated bug. I had it my mind it was fixed. Do you mind looking at this and checking 3.4 and trunk to make a patch that you think is ready to commit? Regards, KAM
The bug[¹] has been fixed in both trunk and 3.4 branch, as for the enhancement, I will finish the patch soon. [¹] - my $email = lc($pms->get(index($header,':') ? $header : $header.":addr")); + my $email = lc($pms->get(index($header,':') >= 0 ? $header : $header.":addr"));
Created attachment 5490 [details] fix for multiple emails in headers As the original post said, if there are multiple emails in a Reply-To header as on test1 example it wont get catched. Adding an option to be able to do more tests is an additional enhancement and a second step.
Fix for enhancement committed in r1825153.