On Tue, Apr 28, 2020 at 01:46:17AM +0200, Ingo Schwarze wrote: > Hi Alexei, > > Alexei Malinin wrote on Tue, Apr 28, 2020 at 01:05:23AM +0300: > > > I ported akpop3d to NetBSD and found the Subject. > > GCC warning was the following: > > authenticate.c: In function 'is_user_allowed': > > authenticate.c:110:11: warning: switch condition has boolean value > > [-Wswitch-bool] > > This is a textbook example of how you must *not* react to a compiler > warning. Do not apply random patches just to shut up the compiler, > without understanding what the code does or what your patches change. > Such reckless behaviour is exactly how Debian produced the spectacular > security vulnerability in their port of OpenSSH several years ago. > > > From pure code inspection, i conclude that what you report is very > likely a security vulnerability. > > > What the code currently does is this: > > * If the file /etc/pop3.deny does not exist or an error occurs > reading from it, all users are denied access. > * If the file /etc/pop3.deny exists and can be read, > users listed in the file are denied access, but > *all* users *not* listed in the file are granted access. > * In either case, the file /etc/pop3.allow is totally ignored. > It may or may not exist, and if it does, the contents are > read, but whatever is in there makes no difference whatsoever. > * Note that the confusing condition > if ((allow == 0 && deny == 0) || (allow == 1 && deny == 0)) { > a few lines below is equivalent to just > if (deny == 0) { > and consequently, the assignments to the variable "allow" are > effectively dead stores. > > From the akpop3d(8) manual page, it remains totally unclear what > the desired behaviour is supposed to be, but the above cannot > possibly be right. It looks as if it was never tested at all. > > > The only effect of your change is as follows. With your change, > we get this in addition to the above: > > * If the file /etc/pop3.allow does not exist or an error occurs > reading from it, all users are denied access. > * But if it can be read, its contents are still totally > ignored. > > Quite obviously, that cannot possibly be correct behaviour either, > so if there is a vulnerability (hard to say given that desired > behaviour is unspecified), it seems unlikely that your change fully > fixes it. > > > Judging from the website, this program has been unmaintained for > more than 15 years. > > I think we should delete the port completely, giving something like > "sloppily coded, sloppily documented, severely buggy and likely > vulnerable abandonware" as the reason for deletion.
It has my vote. -- Antoine