Address CVE-2018-17141 and fixes a few vulnerabilities in code supporting JPEG

These changes are adapted from Lee's fix for this vulnerability.

Luis Merino, Markus Vervier, and Eric Sesterhenn of X41 D-SEC GmbH
(Security Advisory: X41-2018-008) discovered an uninitialized pointer write
and also an out-of-bounds write in FaxModem::writeECMData() that could lead
to remote code execution with a specially-crafted fax sender.

These changes fix the coding errors and deliberately prevent malicious and
malfunctioning senders from inadvertently or deliberately setting JPEG and
MH/MR/MMR/JBIG formats in the same DCS signal.

diff --git a/faxd/Class2.c++ b/faxd/Class2.c++
index 9bd312d3b778b93f8e3ed5382bdd93e4f5f64a99..6439719094995c4315adc05dd0c56e721d71a314 100644 (file)
--- a/faxd/Class2.c++
+++ b/faxd/Class2.c++
@@ -485,6 +485,15 @@ Class2Modem::parseClass2Capabilities(const char* cap, Class2Params& params, bool
        } else {
            if (jpscan == 0x1) params.jp = JP_GREY;
            else if (jpscan & 0x2) params.jp = JP_COLOR;
+           /*
+            * ITU T.30 does not specify that bits 16 (MR) or 31 (MMR) must be set to zero if color fax is used;
+            * and ITU T.32 Table 21 provides a data field, "JP", for JPEG support separate from "DF" for data
+            * format and does not specify that DF is meaningless in DCS when JP is used; but because T.4/T.6
+            * (MH/MR/MMR), JBIG, and JPEG are distinct formats from each other, we must conclude that any
+            * indication of JPEG in DCS must, therefore, invalidate any indication in DCS of MH/MR/MMR/JBIG.
+            * Otherwise, having both df and jp be non-zero will be confusing and possibly cause problems.
+            */
+           if (params.jp != JP_NONE) params.df = 0;    // Yes, this is DF_1DMH, but there is no "DF_NONE".
        }
        return (true);
     } else {

diff --git a/faxd/CopyQuality.c++ b/faxd/CopyQuality.c++
index 6ebc93658af43dfd5dcbfabd90354c3f137b6a1a..d1f2d0f5143bcf9dff1c58757e94df7bd77ebce4 100644 (file)
--- a/faxd/CopyQuality.c++
+++ b/faxd/CopyQuality.c++
@@ -38,6 +38,7 @@
 #include <ctype.h>
 
 #define        RCVBUFSIZ       (32*1024)               // XXX
+#define        COLORBUFSIZ     (2000*1024)             // 1MB is not big enough
 
 static void setupCompression(TIFF*, u_int, u_int, uint32);
 
@@ -356,7 +357,7 @@ FaxModem::recvPageDLEData(TIFF* tif, bool checkQuality,
                 * rather fax-specific.
                 */
                recvEOLCount = 0;
-               recvRow = (u_char*) malloc(1024*1000);    // 1M should do it?
+               recvRow = (u_char*) malloc(COLORBUFSIZ);
                fxAssert(recvRow != NULL, "page buffering error (JPEG page).");
                recvPageStart = recvRow;
            }
@@ -408,8 +409,12 @@ FaxModem::recvPageDLEData(TIFF* tif, bool checkQuality,
                    if (params.df == DF_JBIG) {
                        flushRawData(tif, 0, (const u_char*) buf, cc);
                    } else {
-                       memcpy(recvRow, (const char*) buf, cc);
-                       recvRow += cc;
+                       /* We don't support reception of a JPEG page bigger than COLORBUFSIZ. */
+                       if (recvRow + cc - recvPageStart > COLORBUFSIZ) cc = recvPageStart + COLORBUFSIZ - recvRow;
+                       if (cc > 0) {
+                           memcpy(recvRow, (const char*) buf, cc);
+                           recvRow += cc;
+                       }
                    }
                } while (!fin);
                if (params.df == DF_JBIG) clearSDNORMCount();
@@ -987,7 +992,7 @@ FaxModem::writeECMData(TIFF* tif, u_char* buf, u_int cc, const Class2Params& par
            case JP_GREY+4:
            case JP_COLOR+4:
                recvEOLCount = 0;
-               recvRow = (u_char*) malloc(1024*1000);    // 1M should do it?
+               recvRow = (u_char*) malloc(COLORBUFSIZ);
                fxAssert(recvRow != NULL, "page buffering error (JPEG page).");
                recvPageStart = recvRow;
                setupStartPage(tif, params);
@@ -1039,14 +1044,20 @@ FaxModem::writeECMData(TIFF* tif, u_char* buf, u_int cc, const Class2Params& par
            }
            break;
     }
-    if (params.jp != JP_GREY && params.jp != JP_COLOR) {
-       flushRawData(tif, 0, (const u_char*) buf, cc);
-    } else {
-       memcpy(recvRow, (const char*) buf, cc);
-       recvRow += cc;
-    }
-    if (seq & 2 && (params.jp == JP_GREY || params.jp == JP_COLOR)) {
-       fixupJPEG(tif);
+    switch (dataform) {
+       case JP_GREY+4:
+       case JP_COLOR+4:
+           /* We don't support reception of a JPEG page bigger than COLORBUFSIZ. */
+           if (recvRow + cc - recvPageStart > COLORBUFSIZ) cc = recvPageStart + COLORBUFSIZ - recvRow;
+           if (cc > 0) {
+               memcpy(recvRow, (const char*) buf, cc);
+               recvRow += cc;
+           }
+           if (seq & 2) fixupJPEG(tif);
+           break;
+       default:
+           flushRawData(tif, 0, (const u_char*) buf, cc);
+           break;
     }
 }
 

diff --git a/libhylafax/Class2Params.c++ b/libhylafax/Class2Params.c++
index 0409cbdf33992c380bf0e44394237bfa3a045279..81b9a22bcd45e4a12b6e263967159b48c71147bf 100644 (file)
--- a/libhylafax/Class2Params.c++
+++ b/libhylafax/Class2Params.c++
@@ -303,6 +303,15 @@ Class2Params::setFromDCS(FaxParams& dcs_caps)
     if (dcs_caps.isBitEnabled(FaxParams::BITNUM_FULLCOLOR)) {
        if (jp == JP_GREY) jp = JP_COLOR;
     }
+    /*
+     * ITU T.30 does not specify that bits 16 (MR) or 31 (MMR) must be set to zero if color fax is used;
+     * and ITU T.32 Table 21 provides a data field, "JP", for JPEG support separate from "DF" for data
+     * format and does not specify that DF is meaningless in DCS when JP is used; but because T.4/T.6
+     * (MH/MR/MMR), JBIG, and JPEG are distinct formats from each other, we must conclude that any
+     * indication of JPEG in DCS must, therefore, invalidate any indication in DCS of MH/MR/MMR/JBIG.
+     * Otherwise, having both df and jp be non-zero will be confusing and possibly cause problems.
+     */
+    if (jp != JP_NONE) df = 0; // Yes, this is DF_1DMH, but there is no "DF_NONE".
     if (ec == EC_DISABLE &&
        (df == DF_2DMMR || df == DF_JBIG || jp == JP_GREY || jp == JP_COLOR)) {
        // MMR, JBIG, and JPEG require ECM... we've seen cases where fax