Fixed: Limit max combining characters in TerminalRow to 15 characters to prevent buffer overflows

agnostic-apollo · agnostic-apollo · commit 0813e4633012 · 2024-06-18T03:20:11.000+05:00
The exception below causing app crash happens because of malicious input where combining characters keep getting added to same column of the row and this increases the size of `mSpaceUsed` and `mText`, eventually causing a buffer overflow of `mSpaceUsed`, which is limited to max `32767` value as per java `short` limit, but the limit itself isn't the issue, but an endless number of combining characters being added. Check `MAX_COMBINING_CHARACTERS_PER_COLUMN` field javadocs for why the limit `15` was chosen. ``` curl -o matroska.js https://kimapr.net/lappy/matroska.js cat matroska.js ``` The `charCount` below refers to value of `Character.charCount(codePoint)`, like before `oldCharactersUsedForColumn` is appended to `newCharactersUsedForColumn`. ``` TerminalRow: codePoint=112, mColumns=98, mText=637, columnToSet=18, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=510, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=511, newNextColumnIndex=511, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=40, mColumns=98, mText=637, columnToSet=19, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=511, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=512, newNextColumnIndex=512, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=40, mColumns=98, mText=637, columnToSet=20, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=512, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=513, newNextColumnIndex=513, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=101, mColumns=98, mText=637, columnToSet=21, mSpaceUsed=590, javaCharDifference=0, oldStartOfColumnIndex=513, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=1, oldNextColumnIndex=514, newNextColumnIndex=514, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=1 TerminalRow: codePoint=917772, mColumns=98, mText=147, columnToSet=18, mSpaceUsed=98, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=3, oldNextColumnIndex=19, newNextColumnIndex=21, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 I TerminalRow: codePoint=65024, mColumns=98, mText=147, columnToSet=18, mSpaceUsed=100, javaCharDifference=1, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=3, newCharactersUsedForColumn=4, oldNextColumnIndex=21, newNextColumnIndex=22, charCount=1, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917772, mColumns=98, mText=147, columnToSet=18, mSpaceUsed=101, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=4, newCharactersUsedForColumn=6, oldNextColumnIndex=22, newNextColumnIndex=24, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 ... TerminalRow: codePoint=917959, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=32763, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=32666, newCharactersUsedForColumn=32668, oldNextColumnIndex=32684, newNextColumnIndex=32686, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917939, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=32765, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=32668, newCharactersUsedForColumn=32670, oldNextColumnIndex=32686, newNextColumnIndex=32688, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917961, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=32767, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=32670, newCharactersUsedForColumn=32672, oldNextColumnIndex=32688, newNextColumnIndex=32690, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 TerminalRow: codePoint=917804, mColumns=98, mText=32781, columnToSet=18, mSpaceUsed=-32767, javaCharDifference=2, oldStartOfColumnIndex=18, oldCharactersUsedForColumn=1, newCharactersUsedForColumn=3, oldNextColumnIndex=19, newNextColumnIndex=21, charCount=2, oldCodePointDisplayWidth=1, newCodePointDisplayWidth=0 ``` ``` java.lang.ArrayIndexOutOfBoundsException: src.length=32781 srcPos=19 dst.length=32781 dstPos=21 length=-32786 at java.lang.System.arraycopy(System.java:469) at com.termux.terminal.TerminalRow.setChar(TerminalRow.java:196) at com.termux.terminal.TerminalBuffer.setChar(TerminalBuffer.java:455) at com.termux.terminal.TerminalEmulator.emitCodePoint(TerminalEmulator.java:2380) at com.termux.terminal.TerminalEmulator.processCodePoint(TerminalEmulator.java:624) at com.termux.terminal.TerminalEmulator.processByte(TerminalEmulator.java:520) at com.termux.terminal.TerminalEmulator.append(TerminalEmulator.java:487) at com.termux.terminal.TerminalSession$MainThreadHandler.handleMessage(TerminalSession.java:358) at android.os.Handler.dispatchMessage(Handler.java:106) at android.os.Looper.loop(Looper.java:223) at android.app.ActivityThread.main(ActivityThread.java:7664) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947) ``` See also following links for history of related changes to `TerminalRow` for combining characters. Note that jackpal terminal does not crash for above, which termux-app is based on, but changes were done by fornwall in initial commit of termux-app to change the behaviour, hence the crash, but he added the `FIXME: Put a limit of combining characters` comment as a note to solve the current issue in future, which is now. - jackpal/Android-Terminal-Emulator@9a47042 - jackpal/Android-Terminal-Emulator#338 - a18ee58#diff-f84d215b18106c037e01986a3968fa54b74691174a78fcc99493f745d3805be5 Closes #3839
diff --git a/terminal-emulator/src/main/java/com/termux/terminal/TerminalRow.java b/terminal-emulator/src/main/java/com/termux/terminal/TerminalRow.java
@@ -11,11 +11,37 @@ public final class TerminalRow {
 
     private static final float SPARE_CAPACITY_FACTOR = 1.5f;
 
+    /**
+     * Max combining characters that can exist in a column, that are separate from the base character
+     * itself. Any additional combining characters will be ignored and not added to the column.
+     *
+     * There does not seem to be limit in unicode standard for max number of combination characters
+     * that can be combined but such characters are primarily under 10.
+     *
+     * "Section 3.6 Combination" of unicode standard contains combining characters info.
+     * - https://www.unicode.org/versions/Unicode15.0.0/ch03.pdf
+     * - https://en.wikipedia.org/wiki/Combining_character#Unicode_ranges
+     * - https://stackoverflow.com/questions/71237212/what-is-the-maximum-number-of-unicode-combined-characters-that-may-be-needed-to
+     *
+     * UAX15-D3 Stream-Safe Text Format limits to max 30 combining characters.
+     * > The value of 30 is chosen to be significantly beyond what is required for any linguistic or technical usage.
+     * > While it would have been feasible to chose a smaller number, this value provides a very wide margin,
+     * > yet is well within the buffer size limits of practical implementations.
+     * - https://unicode.org/reports/tr15/#Stream_Safe_Text_Format
+     * - https://stackoverflow.com/a/11983435/14686958
+     *
+     * We choose the value 15 because it should be enough for terminal based applications and keep
+     * the memory usage low for a terminal row, won't affect performance or cause terminal to
+     * lag or hang, and will keep malicious applications from causing harm. The value can be
+     * increased if ever needed for legitimate applications.
+     */
+    private static final int MAX_COMBINING_CHARACTERS_PER_COLUMN = 15;
+
     /** The number of columns in this terminal row. */
     private final int mColumns;
     /** The text filling this terminal row. */
     public char[] mText;
-    /** The number of java char:s used in {@link #mText}. */
+    /** The number of java chars used in {@link #mText}. */
     private short mSpaceUsed;
     /** If this row has been line wrapped due to text output at the end of line. */
     boolean mLineWrap;
@@ -163,18 +189,25 @@ public void setChar(int columnToSet, int codePoint, long style) {
         // Get the number of elements in the mText array this column uses now
         int oldCharactersUsedForColumn;
         if (columnToSet + oldCodePointDisplayWidth < mColumns) {
-            oldCharactersUsedForColumn = findStartOfColumn(columnToSet + oldCodePointDisplayWidth) - oldStartOfColumnIndex;
+            int oldEndOfColumnIndex = findStartOfColumn(columnToSet + oldCodePointDisplayWidth);
+            oldCharactersUsedForColumn = oldEndOfColumnIndex - oldStartOfColumnIndex;
         } else {
             // Last character.
             oldCharactersUsedForColumn = mSpaceUsed - oldStartOfColumnIndex;
         }
 
+        // If MAX_COMBINING_CHARACTERS_PER_COLUMN already exist in column, then ignore adding additional combining characters.
+        if (newIsCombining) {
+            int combiningCharsCount = WcWidth.zeroWidthCharsCount(mText, oldStartOfColumnIndex, oldStartOfColumnIndex + oldCharactersUsedForColumn);
+            if (combiningCharsCount >= MAX_COMBINING_CHARACTERS_PER_COLUMN)
+                return;
+        }
+
         // Find how many chars this column will need
         int newCharactersUsedForColumn = Character.charCount(codePoint);
         if (newIsCombining) {
             // Combining characters are added to the contents of the column instead of overwriting them, so that they
             // modify the existing contents.
-            // FIXME: Put a limit of combining characters.
             // FIXME: Unassigned characters also get width=0.
             newCharactersUsedForColumn += oldCharactersUsedForColumn;
         }
@@ -189,7 +222,7 @@ public void setChar(int columnToSet, int codePoint, long style) {
             if (mSpaceUsed + javaCharDifference > text.length) {
                 // We need to grow the array
                 char[] newText = new char[text.length + mColumns];
-                System.arraycopy(text, 0, newText, 0, oldStartOfColumnIndex + oldCharactersUsedForColumn);
+                System.arraycopy(text, 0, newText, 0, oldNextColumnIndex);
                 System.arraycopy(text, oldNextColumnIndex, newText, newNextColumnIndex, oldCharactersAfterColumn);
                 mText = text = newText;
             } else {
diff --git a/terminal-emulator/src/main/java/com/termux/terminal/WcWidth.java b/terminal-emulator/src/main/java/com/termux/terminal/WcWidth.java
@@ -538,4 +538,29 @@ public static int width(char[] chars, int index) {
         return Character.isHighSurrogate(c) ? width(Character.toCodePoint(c, chars[index + 1])) : width(c);
     }
 
+    /**
+     * The zero width characters count like combining characters in the `chars` array from start
+     * index to end index (exclusive).
+     */
+    public static int zeroWidthCharsCount(char[] chars, int start, int end) {
+        if (start < 0 || start >= chars.length)
+            return 0;
+
+        int count = 0;
+        for (int i = start; i < end && i < chars.length;) {
+            if (Character.isHighSurrogate(chars[i])) {
+                if (width(Character.toCodePoint(chars[i], chars[i + 1])) <= 0) {
+                    count++;
+                }
+                i += 2;
+            } else {
+                if (width(chars[i]) <= 0) {
+                    count++;
+                }
+                i++;
+            }
+        }
+        return count;
+    }
+
 }
