Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 2 May 2014 07:50:57 -0700
From: Jim Hull <imaginos@...nethull.com>
To: Solar Designer <solar@...nwall.com>
Cc: oss-security@...ts.openwall.com, Andreas Krennmair <ak@...flood.at>
Subject: Re: akpop3d review

This is really quite the blast from the past, I had almost forgotten I
submitted patches for akpop3d. As I recall, one patch was included to fix
file descriptor exhaustion, the second patch was for the MySQL support that
you cover in your review. This was back in 2002, and I ended up not using
akpop3d after all as I needed to store email in MySQL at that time and not
just authentication. Additionally, I recollect that the patch gnu-pop3d was
not included, and the patch for gnu-mailutils went under significant review
and rewrite and was extended beyond my original submission.

Thanks for the comments on the usage of asprintf(), obviously missing
checks on the return was a bad idea. At the time I presumed that the
pointer passed in would be NULL on error and I have since been re-educated
:).

With regards to the username SQL injection fix, I don't believe that was
included in my original patch, subsequently this means the original code
was probably not doing a check at all which is worse. At a
minimum mysql_real_escape_string() should have been used, and at the time I
was thinking the username was probably already taint checked and characters
already whitelisted per conventional username requirements
"[a-z_][a-z0-9_-]*[$]". This was obviously poor judgement on my part.

I have not been following akpop3d since the submission of that last patch,
so I am unaware if the MySQL patch actually has any users. My guess is that
would be slim to none as people probably would run into similar limitations
as I did. I would recommend to Andreas to simply remove MySQL support to
keep his product more simple, and subsequently more secure.

Thanks for the comments!

- Jim


On Thu, May 1, 2014 at 6:09 PM, Solar Designer <solar@...nwall.com> wrote:

> Hi,
>
> As some of you might have guessed my review of akpop3d is prompted by
> OpenBSD 5.5 dropping popa3d from base (a reasonable thing to do given
> the mass surveillance, and given that it was high time I added builtin
> SSL support, which I still have not), and not (yet) offering it in
> packages.  akpop3d is suggested as the primary alternative:
>
> http://www.openbsd.org/faq/upgrade55.html#popa3d
>
> "popa3d removed
> The POP3 daemon has been removed. Alternatives are available in
> packages, such as: akpop3d, courier-pop3 and others."
>
> http://www.synflood.at/akpop3d.html
>
> Is akpop3d a good choice, security-wise?  In terms of offering builtin
> SSL and in terms of code size, yes.  In other ways, I think not so much,
> although I didn't spot anything easily exploitable beyond DoS (so
> there's no reason for end-users to panic, and my posting isn't aimed at
> end-users).
>
> Here are some quick observations, based on akpop3d 0.7.7 released in
> 2004 (yes, 10 years ago), which I think is still the latest (that's
> possibly fine for a small program, it could show code maturity).
> Obviously, I might be biased, but I had considered many of the same
> issues and tradeoffs while writing popa3d, so I think I am a right
> person to comment on akpop3d's design and code.
>
> Design:
>
> akpop3d tries to be secure by means of being simple, to the extent of
> lacking privsep.  This is a fine choice with its pros and cons, although
> I think adequate simplicity can be achieved while also including privsep
> (like I did in popa3d).  (I would especially not rely on the SSL library
> code being somehow "safe" to run outside of a privsep child.  I was
> planning to create a separate privsep child for the SSL in popa3d. but I
> never got around to implementing that.)
>
> akpop3d chooses to support dot-locking and copying of mbox contents (to
> temporary per-message files, which I guess makes akpop3d rather slow,
> perhaps even slower than the old qpopper).  Because of this, along with
> lacking privsep, it retains group "mail" in its child process.  While
> this is not as bad as continuing to run as root, it is pretty bad in
> that group "mail" commonly allows to read/write/delete any other users'
> mail, as well as to introduce trapdoors into the mail spool (thereby
> potentially exploiting other mail programs' peculiar behavior and
> vulnerabilities).  This is what's directly at stake on every line of
> code of akpop3d.  (In contrast, in popa3d many lines of code run as the
> target user only, with no group privileges.  Security aside, I admit
> that there are pros and cons to either approach in terms of backwards
> compatibility and reliability over system crashes vs. performance.)
>
> A couple of other things akpop3d's design.txt mentions are "Secure
> Creation Of Files" (true, but with popa3d's approach no files need to be
> created in the first place) and "Parsing Emails" with the "Don't parse"
> advice/approach (great, but the way UIDLs are calculated I guess they
> are too often non-unique and unstable over other MUA runs - that's two
> non-security problems, which I'd be happy to discuss separately).
>
> Code, integer overflows:
>
> It appears that integer overflows were not considered at all.  Luckily,
> it is possible to audit the code and introduce the due checks (or
> otherwise make the overflows and C unspecified behavior impossible to
> trigger), but as it is it looks pretty bad.  Most notably:
>
> static int number_mails;
> [...]
>       number_mails++;
>       mails = realloc(mails,number_mails*sizeof(struct mail));
>
> That's two potential integer overflows, which might be triggerable by
> untrusted input (depending on mbox size limits with the MTA/LDA and
> filesystem) on two adjacent lines of code.  sizeof(struct mail) is at
> least 268 on a 32-bit system (and usually slightly more on 64-bit):
>
> struct mail {
>   unsigned long size;
>   unsigned long adjsize;
>   int deleted;
>   char filename[256];
> };
>
> Luckily, sizeof(struct mail) is unsigned, and is of a type no smaller
> than int.  Yet if the expression remains 32-bit, the multiplication
> wraps around at "only" 16 million messages in the mbox.  When this
> happens, "mails" is allocated with a smaller size than it needs to be,
> and we have out of bounds writes somewhere on the heap.
>
> In practice, this might be mitigated by a previous realloc() failing to
> allocate the nearly 4 GiB of memory on a 32-bit platform, in which case
> the function would have returned before the overflow could happen:
>
>       if (mails==NULL) {
>         return 0;
>       }
>
> Are there platforms where size_t is 32-bit, yet an almost 4 GiB
> allocation succeeds (meaning the process has more than 4 GiB of address
> space, since more than 268 bytes would surely be already in use by the
> program itself, its stack, etc.)?
>
> Maybe we got lucky here, but we shouldn't continue to depend on subtle
> things like that for security.
>
> Code, async-signal-unsafe functions are used with signals:
>
> akpop3d calls async-signal-unsafe functions from a signal handler:
>
> static void sig_handler(int signo) {
>   remove_lock(mdl);
>   syslog(LOG_INFO,"%s: %u", "caught signal",signo);
>   exit(EXIT_FAILURE);
> }
>
> It also has the signal handler setup while the main program uses
> async-signal-unsafe functions, meaning that the signal handler can be
> entered with e.g. syslog or stdio already in inconsistent state, and
> would proceed to use that state as if it were consistent.  IIRC,
> syslog() in particular is made async-signal-safe on OpenBSD, but not on
> other systems.
>
> syslog() from a signal handler in portable code is not considered
> acceptable (see e.g. CVE-2008-4109).  exit() is not acceptable either
> (flushes stdio buffers, which may already be in inconsistent state, and
> invokes atexit() handlers, which may contain async-signal-unsafe
> functions).  remove_lock() looks OK, though:
>
> void remove_lock(char * maildrop) {
>   size_t lf_len = (size_t)strlen(maildrop)+strlen(".lock")+1;
>   char * lf = alloca(lf_len);
>   if (lf!=NULL) {
>     snprintf(lf,lf_len,"%s.lock",maildrop);
>     unlink(lf);
>   }
> }
>
> Code, risky use of MySQL:
>
>   { /* prevent SQL injections */
>     int i, max = strlen(username);
>     for (i=0;i<max;i++) {
>     if (username[i]=='\'') {
>       username[i] = '_';
>     }
>   }
>
>         // send the query
>         asprintf( &pszQuery, "SELECT username, uid, gid, password,
> homedir, shell, comment FROM %s where username = '%s'", TABLE, username );
>
> If questionable functionality like this is included at all, it'd be
> better to use prepared statements or/and whitelisting.  The code above
> uses a blacklist (of only one char), and fails to blacklist '\', which
> is a special character too.  I don't see how this can be exploited
> (hopefully, it can't be) into anything more than incorrect syntax
> (which can be achieved by making '\' the very last character in
> attempted username, so the closing single quote is not treated as such).
>
> Code, unsafe uses of asprintf():
>
> The code snippet above also serves as an example of this issue. :-)
> glibc's asprintf() leaves the pointer unchanged on error (unlike *BSD's),
> indicating the error only via the return value.  Ulrich rejected a patch
> to introduce the safer *BSD's behavior:
>
> http://www.sourceware.org/ml/libc-alpha/2001-12/msg00021.html
>
> The patch has since been included in a few Linux distros, but most
> distros retain upstream's unsafe behavior.
>
> The code then proceeds to do:
>
>         free( pszQuery );
>
> where pszQuery might have been never initialized (it is not explicitly
> initialized in the code above), so we get a free() call on some stack
> contents in place of the pointer.  With some luck for and/or effort by
> the attacker, this might be exploitable.
>
> Luckily, this issue only occurs twice, and only in mysql.c (which per
> README.MySQL is contributed code, but the author(s) should have reviewed
> it more thoroughly before integration, I think).
>
> README.MySQL also says:
>
> "I originally added a patch like this to gnu-pop3d and then to gnu
> mailutilities."
>
> So these two projects might be affected as well.
>
> Code, lacks child count and frequency limits:
>
> It appears that akpop3d's main daemon loop will happily overload the
> system as long as connections keep coming.  It can also be used to flood
> the logs (use up disk space or/and exhaust syslog bandwidth, which is
> often pretty low, thereby affecting other services that do logging).
> (popa3d has total and per-source connection count limits, and an extra
> parameter and extra logic to mitigate log flooding while nevertheless
> logging anything important about connections that were accepted.)
>
> Code, MD5 may be miscompiled with wrong endianness:
>
> There's no security impact from this, but the included GNU MD5 routines
> are notorious of easy miscompiles, and akpop3d appears to suffer from
> this problem.  md5.c contains:
>
> #ifdef _LIBC
> # include <endian.h>
> # if __BYTE_ORDER == __BIG_ENDIAN
> #  define WORDS_BIGENDIAN 1
> # endif
> #endif
>
> and _LIBC is not defined anywhere in akpop3d.  Thus, I expect that on
> big-endian systems akpop3d uses a non-MD5 where it wanted to use MD5.
>
> (This is a reason why I wrote my own MD5 implementation for popa3d,
> which doesn't depend on compile-time configuration.  Other suitably
> licensed C implementations of MD5 that I could find suffered from this
> potential problem.  Of course, writing one's own crypto code is a risk,
> too, and is generally rightly frowned upon, so it was a tradeoff.)
>
> Andreas, I hope you don't mind me posting this in public, and the
> comparisons against popa3d.  I briefly considered notifying you in
> private first (and I definitely would if I found anything requiring an
> urgent patch), but I felt it's more optimal to make the info public
> right away.  I actually wanted to avoid comparisons against popa3d at
> first, but while writing this message I felt that the comparisons I did
> include anyway are important to show which issues are inherent and which
> could be avoided.  (I think it's obvious that integer overflows and
> unsafe signal handling can be avoided, so I felt no need to include
> explicit comparisons there.)
>
> Finally, I need to add a disclaimer: this is an overall review of
> akpop3d, not a security audit of akpop3d.  I had no goal to check every
> line of code for potential vulnerabilities (e.g., verifying the length
> calculations for possible off-by-ones, which is something I didn't do),
> but rather I wanted to get the overall picture.
>
> I hope someone finds this helpful.
>
> Alexander
>

Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ