[MaraDNS list] Multiple issues in JsStrOS.c

[Rich Felker]

On Sat, Jun 02, 2012 at 06:28:23PM -0400, Sam Trenholme wrote:
> Since this is attempting to come off as a security report, I will
> briefly humor Rich and reply to him even though it's not after the
> 20th.

I'm not clear what I did that warranted this kind of hostile response.
It's certainly not the sort of communication we had back in 2005..

> The bottom line, Rich, is that it is better to light a candle than
> cure the darkness.  If you have an issue with MaraDNS' library, you
> would be a far more productive person if you submitted a patch to
> address the issue instead of just complaining on the mailing list.

The homepage says "MaraDNS support (including bug reports) is
available on the MaraDNS mailing list." so I figured it was
appropriate to send a bug report. Perhaps the fact that the subject
line sounded a bit like a security advisory was problematic..?

> You are aware that MaraDNS 1 is no longer supported

Yes.

> and that MaraDNS 2
> only uses the really old code you're complaining about in the
> authoritative code, which means that nothing about MaraDNS 2's memory
> state can be changed by a remote attacker.

I was not aware that the affected code is not used in deadwood.
Naturally this seems to make the issue non-security-relevant for
MaraDNS 2, but my impression was that you intended JsStr to be a
secure string library. At present, these issues would certainly affect
other code using the library, even if they don't affect MaraDNS.

> "continues to functional normally when malloc() fails" has never been
> a design criteria for either MaraDNS or Deadwood.  The only OSes I

I would consider it a requirement for a secure daemon not to crash or
behave in a way that compromises security under resource-exhaustion
conditions. Naturally it may have to fail temporarily under such
conditions. But this is getting rather OT, so I'll drop it.

> support are Windows and RHEL6-derived versions of Linux.  Both OSes
> are either rebooting or randomly killing processes long before
> malloc() starts failing.

A properly configured Linux system does not randomly kill processes.
The OOM killer is quite smart about which processes it kills, but more
importantly a Linux system configured for deployment as a server will
have overcommit disabled (it's a simple echo "2" >
/proc/sys/vm/overcommit_memory).

> Again, Rich, don't waste my time with random whining.  I have finished
> MaraDNS;

I was not aware that you're not interested in further development of
MaraDNS. If that's the case, sorry for bothering you.

> if you don't like the way it's written, submit a patch.  This

Patch included inline below.

Rich



--- libs/JsStrOS.c.orig
+++ libs/JsStrOS.c
@@ -61,7 +61,7 @@
     lt_hash_spot *new,*point;
 #endif /* DEBUG */
     /* Sanity check: Never allow this; makes C act buggy */
-    if(unit_size == 0 || unit_count == 0)
+    if(unit_size == 0 || unit_count == 0 || unit_count >= INT_MAX/unit_size)
         return 0;
     data = (void *)malloc(unit_count * unit_size);
 #ifdef DEBUG
@@ -115,14 +115,6 @@
     pthread_mutex_unlock(&alloc_lock);
 #endif /* THREADS */
 #endif /* DEBUG */
-    if(data == NULL) {
-        /* Securty: In a situtation where we can not allocate memory,
-           the subsequent behavior of the program is undefined.  Hence,
-           the best thing to do is exit then and there */
-        printf("Aieeeeee, can not allocate memory!");
-        exit(64);
-        return (void *)0;
-        }
     return data;
     }