CVE-2021-45930: Out-of-bounds Write (CWE-787)
value: Qt SVG in Qt 5.0.0 through 5.15.2 and 6.0.0 through 6.2.1 has an out-of-bounds write in QtPrivate::QCommonArrayOps<QPainterPath::Element>::growAppend (called from QPainterPath::addPath and QPathClipper::intersect).
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 5.5
Published: 2022-01-01T01:15Z
Repository: https://github.com/qt/qtsvg
Language: C++
Files changed: src/svg/qsvghandler.cpp
Commit date: 2021-10-27 10:57:16 +0200
Commit message: Do stricter error checking when parsing path nodes

The SVG spec mandates that path parsing should terminate on the first
error encountered, and an error be reported. To improve the handling
of corrupt files, implement such error handling, and also limit the
number of QPainterPath elements to a reasonable range.

Fixes: QTBUG-96044
Pick-to: 6.2 5.15 5.12
Change-Id: Ic5e65d6b658516d6f1317c72de365c8c7ad81891
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
Reviewed-by: Robert Löhning <robert.loehning@qt.io>

Diff: diff --git a/src/svg/qsvghandler.cpp b/src/svg/qsvghandler.cpp
index db29211..dd869ff 100644
--- a/src/svg/qsvghandler.cpp
+++ b/src/svg/qsvghandler.cpp
@@ -1593,18 +1593,20 @@ static void pathArc(QPainterPath &path,
     }
 }
 
 static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
 {
+    const int maxElementCount = 0x7fff; // Assume file corruption if more path elements than this
     qreal x0 = 0, y0 = 0;              // starting point
     qreal x = 0, y = 0;                // current point
     char lastMode = 0;
     QPointF ctrlPt;
     const QChar *str = dataStr.constData();
     const QChar *end = str + dataStr.size();
 
-    while (str != end) {
+    bool ok = true;
+    while (ok && str != end) {
         while (str->isSpace() && (str + 1) != end)
             ++str;
         QChar pathElem = *str;
         ++str;
         QChar endc = *end;
@@ -1617,18 +1619,17 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
         *const_cast<QChar *>(end) = endc;
         if (pathElem == QLatin1Char('z') || pathElem == QLatin1Char('Z'))
             arg.append(0);//dummy
         const qreal *num = arg.constData();
         int count = arg.count();
-        while (count > 0) {
+        while (ok && count > 0) {
             qreal offsetX = x;        // correction offsets
             qreal offsetY = y;        // for relative commands
             switch (pathElem.unicode()) {
             case 'm': {
                 if (count < 2) {
-                    num++;
-                    count--;
+                    ok = false;
                     break;
                 }
                 x = x0 = num[0] + offsetX;
                 y = y0 = num[1] + offsetY;
                 num += 2;
@@ -1641,12 +1642,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                  pathElem = QLatin1Char('l');
             }
                 break;
             case 'M': {
                 if (count < 2) {
-                    num++;
-                    count--;
+                    ok = false;
                     break;
                 }
                 x = x0 = num[0];
                 y = y0 = num[1];
                 num += 2;
@@ -1668,12 +1668,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 path.closeSubpath();
             }
                 break;
             case 'l': {
                 if (count < 2) {
-                    num++;
-                    count--;
+                    ok = false;
                     break;
                 }
                 x = num[0] + offsetX;
                 y = num[1] + offsetY;
                 num += 2;
@@ -1682,12 +1681,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
 
             }
                 break;
             case 'L': {
                 if (count < 2) {
-                    num++;
-                    count--;
+                    ok = false;
                     break;
                 }
                 x = num[0];
                 y = num[1];
                 num += 2;
@@ -1723,12 +1721,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 path.lineTo(x, y);
             }
                 break;
             case 'c': {
                 if (count < 6) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF c1(num[0] + offsetX, num[1] + offsetY);
                 QPointF c2(num[2] + offsetX, num[3] + offsetY);
                 QPointF e(num[4] + offsetX, num[5] + offsetY);
@@ -1740,12 +1737,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 'C': {
                 if (count < 6) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF c1(num[0], num[1]);
                 QPointF c2(num[2], num[3]);
                 QPointF e(num[4], num[5]);
@@ -1757,12 +1753,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 's': {
                 if (count < 4) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF c1;
                 if (lastMode == 'c' || lastMode == 'C' ||
                     lastMode == 's' || lastMode == 'S')
@@ -1779,12 +1774,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 'S': {
                 if (count < 4) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF c1;
                 if (lastMode == 'c' || lastMode == 'C' ||
                     lastMode == 's' || lastMode == 'S')
@@ -1801,12 +1795,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 'q': {
                 if (count < 4) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF c(num[0] + offsetX, num[1] + offsetY);
                 QPointF e(num[2] + offsetX, num[3] + offsetY);
                 num += 4;
@@ -1817,12 +1810,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 'Q': {
                 if (count < 4) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF c(num[0], num[1]);
                 QPointF e(num[2], num[3]);
                 num += 4;
@@ -1833,12 +1825,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 't': {
                 if (count < 2) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF e(num[0] + offsetX, num[1] + offsetY);
                 num += 2;
                 count -= 2;
@@ -1854,12 +1845,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 'T': {
                 if (count < 2) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 QPointF e(num[0], num[1]);
                 num += 2;
                 count -= 2;
@@ -1875,12 +1865,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = e.y();
                 break;
             }
             case 'a': {
                 if (count < 7) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 qreal rx = (*num++);
                 qreal ry = (*num++);
                 qreal xAxisRotation = (*num++);
@@ -1898,12 +1887,11 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 y = ey;
             }
                 break;
             case 'A': {
                 if (count < 7) {
-                    num += count;
-                    count = 0;
+                    ok = false;
                     break;
                 }
                 qreal rx = (*num++);
                 qreal ry = (*num++);
                 qreal xAxisRotation = (*num++);
@@ -1920,16 +1908,19 @@ static bool parsePathDataFast(QStringView dataStr, QPainterPath &path)
                 x = ex;
                 y = ey;
             }
                 break;
             default:
-                return false;
+                ok = false;
+                break;
             }
             lastMode = pathElem.toLatin1();
+            if (path.elementCount() > maxElementCount)
+                ok = false;
         }
     }
-    return true;
+    return ok;
 }
 
 static bool parseStyle(QSvgNode *node,
                        const QXmlStreamAttributes &attributes,
                        QSvgHandler *);
@@ -2983,12 +2974,12 @@ static QSvgNode *createPathNode(QSvgNode *parent,
 {
     QStringView data = attributes.value(QLatin1String("d"));
 
     QPainterPath qpath;
     qpath.setFillRule(Qt::WindingFill);
-    //XXX do error handling
-    parsePathDataFast(data, qpath);
+    if (!parsePathDataFast(data, qpath))
+        qCWarning(lcSvgHandler, "Invalid path data; path truncated.");
 
     QSvgNode *path = new QSvgPath(parent, qpath);
     return path;
 }
 

Vulnerable code:     while (str != end) {
        while (count > 0) {
                    num++;
                    count--;
                    num++;
                    count--;
                    num++;
                    count--;
                    num++;
                    count--;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                    num += count;
                    count = 0;
                return false;
    return true;
    //XXX do error handling
    parsePathDataFast(data, qpath);
Fixed code:     const int maxElementCount = 0x7fff; // Assume file corruption if more path elements than this
    bool ok = true;
    while (ok && str != end) {
        while (ok && count > 0) {
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                    ok = false;
                ok = false;
                break;
            if (path.elementCount() > maxElementCount)
                ok = false;
    return ok;
    if (!parsePathDataFast(data, qpath))
        qCWarning(lcSvgHandler, "Invalid path data; path truncated.");