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.
Yours,
Ingo
