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
