Re: mail/akpop3d authenticate.c:user_in_file bug
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
Re: mail/akpop3d authenticate.c:user_in_file bug
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
mail/akpop3d authenticate.c:user_in_file bug
Hello!
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]
switch (user_in_file(user,POP3ALLOW_FILE)>0) {
^
Please look at the patches below.
--
Alexei
Index: patch-authenticate_c
===
RCS file: /cvs/ports/mail/akpop3d/patches/patch-authenticate_c,v
retrieving revision 1.2
diff -u -p -r1.2 patch-authenticate_c
--- patch-authenticate_c 22 May 2017 20:03:43 - 1.2
+++ patch-authenticate_c 27 Apr 2020 21:42:16 -
@@ -17,7 +17,7 @@ Index: authenticate.c
int allow, deny;
- switch (user_in_file(user,"/etc/pop3.allow")>0) {
-+ switch (user_in_file(user,POP3ALLOW_FILE)>0) {
++ switch (user_in_file(user,POP3ALLOW_FILE)) {
case 0:
allow = 0;
break;
Index: Makefile
===
RCS file: /cvs/ports/mail/akpop3d/Makefile,v
retrieving revision 1.13
diff -u -p -r1.13 Makefile
--- Makefile 12 Jul 2019 20:47:24 - 1.13
+++ Makefile 27 Apr 2020 21:48:43 -
@@ -3,7 +3,7 @@
COMMENT= small and secure POP3 daemon
DISTNAME= akpop3d-0.7.7
-REVISION = 3
+REVISION = 4
CATEGORIES= mail
HOMEPAGE= http://www.synflood.at/akpop3d.html
.
