INN commit: trunk/nnrpd (line.c misc.c nnrpd.c)

[INN Commit]

    Date: Friday, December 4, 2015 @ 12:52:27
  Author: iulius
Revision: 9960

Improve the robustness of SASL handling

After sasl_decode(), add a check to prevent a call to memcpy() for 0
byte or for more bytes than the destination buffer.

Check that sasl_errdetail() does not return NULL.

Improve a few comments in the source code.

Modified:
  trunk/nnrpd/line.c
  trunk/nnrpd/misc.c
  trunk/nnrpd/nnrpd.c

---------+
 line.c  |   46 +++++++++++++++++++++++++++++-----------------
 misc.c  |    6 +++---
 nnrpd.c |   33 ++++++++++++++++++++-------------
 3 files changed, 52 insertions(+), 33 deletions(-)

Modified: line.c
===================================================================
--- line.c	2015-11-03 19:55:06 UTC (rev 9959)
+++ line.c	2015-12-04 20:52:27 UTC (rev 9960)
@@ -119,24 +119,36 @@
 	if (n <= 0)
             break; /* EOF or error. */
 
-#ifdef HAVE_SASL
-	if (sasl_conn && sasl_ssf) {
-	    /* Security layer in place, decode the data. */
-	    const char *out;
-	    unsigned outlen;
-	    int r;
+#if defined(HAVE_SASL)
+        if (sasl_conn != NULL && sasl_ssf > 0) {
+            /* Security layer in place, decode the data.
+             * The incoming data is always encoded in chunks of length
+             * inferior or equal to NNTP_MAXLEN_COMMAND (the maxbufsize value
+             * of SASL_SEC_PROPS passed as part of the SASL exchange).
+             * So there's enough data to read in the p buffer. */
+            const char *out;
+            unsigned outlen;
+            int r;
+ 
+            if ((r = sasl_decode(sasl_conn, p, n, &out, &outlen)) == SASL_OK) {
+                if (outlen > len) {
+                    sysnotice("sasl_decode() returned too much output");
+                    n = -1;
+                } else {
+                    if (outlen > 0) {
+                        memcpy(p, out, outlen);
+                    }
+                    n = outlen;
+                }
+            } else {
+                const char *ed = sasl_errdetail(sasl_conn);
 
-	    if ((r = sasl_decode(sasl_conn, p, n, &out, &outlen)) == SASL_OK) {
-		if (outlen)
-                    memcpy(p, out, outlen);
-		n = outlen;
-	    } else {
-		sysnotice("sasl_decode() failed: %s; %s",
-			  sasl_errstring(r, NULL, NULL),
-			  sasl_errdetail(sasl_conn));
-		n = -1;
-	    }
-	}
+                sysnotice("sasl_decode() failed: %s; %s",
+                          sasl_errstring(r, NULL, NULL),
+                          ed != NULL ? ed : "no detail");
+                n = -1;
+            }
+        }
 #endif /* HAVE_SASL */
     } while (n == 0); /* Split SASL blob, need to read more data. */
 

Modified: misc.c
===================================================================
--- misc.c	2015-11-03 19:55:06 UTC (rev 9959)
+++ misc.c	2015-12-04 20:52:27 UTC (rev 9960)
@@ -460,7 +460,7 @@
     bool boolval;
 
     if (encryption_layer_on) {
-        Reply("%d Already using an active TLS layer\r\n", NNTP_ERR_ACCESS);
+        Reply("%d Already using a TLS layer\r\n", NNTP_ERR_ACCESS);
         return;
     }
 
@@ -519,7 +519,7 @@
         ExitWithStats(1, false);
     }
 
-#ifdef HAVE_SASL
+# if defined(HAVE_SASL)
     /* Tell SASL about the negotiated layer. */
     result = sasl_setprop(sasl_conn, SASL_SSF_EXTERNAL,
                           (sasl_ssf_t *) &tls_cipher_usebits);
@@ -531,7 +531,7 @@
     if (result != SASL_OK) {
         syslog(L_NOTICE, "sasl_setprop() failed: CMDstarttls()");
     }
-#endif /* HAVE_SASL */
+# endif /* HAVE_SASL */
 
     /* Reset our read buffer so as to prevent plaintext command injection. */
     line_reset(&NNTPline);

Modified: nnrpd.c
===================================================================
--- nnrpd.c	2015-11-03 19:55:06 UTC (rev 9959)
+++ nnrpd.c	2015-12-04 20:52:27 UTC (rev 9960)
@@ -36,6 +36,7 @@
 #if defined(HAVE_OPENSSL)
 extern SSL *tls_conn;
 #endif
+
 #if defined(HAVE_OPENSSL) || defined(HAVE_SASL)
 bool encryption_layer_on = false;
 #endif 
@@ -439,12 +440,15 @@
 
     Printf("READER\r\n");
 
-#ifdef HAVE_SASL
-    /* Check whether at least one SASL mechanism is available. */
+#if defined(HAVE_SASL)
+    /* Check whether at least one SASL mechanism is available.
+     * The SASL capability has to be advertised, even after authentication,
+     * so that the client can detect a possible active down-negotiation
+     * attack. */
     if (mechlist != NULL && strlen(mechlist) > 2) {
         Printf("SASL%s\r\n", mechlist);
     }
-#endif
+#endif /* HAVE_SASL */
 
 #ifdef HAVE_OPENSSL
     /* A TLS layer is not active and the client is not already authenticated. */
@@ -640,18 +644,21 @@
 	const char *out;
 	unsigned outlen;
 
-#ifdef HAVE_SASL
-	if (sasl_conn && sasl_ssf) {
+#if defined(HAVE_SASL)
+        if (sasl_conn != NULL && sasl_ssf > 0) {
             int r;
 
-	    /* Can only encode as much as the client can handle at one time. */
-	    n = (len > sasl_maxout) ? sasl_maxout : len;
-	    if ((r = sasl_encode(sasl_conn, p, n, &out, &outlen)) != SASL_OK) {
-		sysnotice("sasl_encode() failed: %s",
-			  sasl_errstring(r, NULL, NULL));
-		return;
-	    }
-	} else
+            /* Can only encode as much as the client can handle at one time. */
+            n = (len > sasl_maxout) ? sasl_maxout : len;
+            if ((r = sasl_encode(sasl_conn, p, n, &out, &outlen)) != SASL_OK) {
+                const char *ed = sasl_errdetail(sasl_conn);
+
+                sysnotice("sasl_encode() failed: %s; %s",
+                          sasl_errstring(r, NULL, NULL),
+                          ed != NULL ? ed : "no detail");
+                return;
+            }
+        } else
 #endif /* HAVE_SASL */
 	{
 	    /* Output the entire unencoded string. */