Skip to content

Commit

Permalink
Fixed: Limit max combining characters in TerminalRow to 15 characters…
Browse files Browse the repository at this point in the history 
… to prevent buffer overflows

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
  • Loading branch information
@agnostic-apollo
agnostic-apollo committed Apr 7, 2024
1 parent 7b19cd2 commit 67f4891
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 4 deletions.
41 changes: 37 additions & 4 deletions terminal-emulator/src/main/java/com/termux/terminal/TerminalRow.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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 {
Expand Down
25 changes: 25 additions & 0 deletions terminal-emulator/src/main/java/com/termux/terminal/WcWidth.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

}

0 comments on commit 67f4891

Please sign in to comment.