From f1accae2955c1bfc902eb9b18ca6cbaab4e8d890 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 5 Jun 2026 19:48:50 +0000 Subject: [PATCH] Fix bidi balancing character. --- .../java/org/signal/core/util/BidiUtil.kt | 7 +- .../java/org/signal/core/util/BidiUtilTest.kt | 160 ++++++++++++++++-- 2 files changed, 151 insertions(+), 16 deletions(-) diff --git a/core/util-jvm/src/main/java/org/signal/core/util/BidiUtil.kt b/core/util-jvm/src/main/java/org/signal/core/util/BidiUtil.kt index cdbe6ebb5c..2e4fbc5a7c 100644 --- a/core/util-jvm/src/main/java/org/signal/core/util/BidiUtil.kt +++ b/core/util-jvm/src/main/java/org/signal/core/util/BidiUtil.kt @@ -109,8 +109,7 @@ object BidiUtil { var isolateCloseCount = 0 var i = 0 - val len = text.codePointCount(0, text.length) - while (i < len) { + while (i < text.length) { val codePoint = text.codePointAt(i) if (Bidi.OVERRIDES.contains(codePoint)) { @@ -122,7 +121,7 @@ object BidiUtil { } else if (codePoint == Bidi.PDI) { isolateCloseCount++ } - i++ + i += Character.charCount(codePoint) } val suffix = StringBuilder() @@ -133,7 +132,7 @@ object BidiUtil { } while (isolateCount > isolateCloseCount) { - suffix.appendCodePoint(Bidi.FSI) + suffix.appendCodePoint(Bidi.PDI) isolateCloseCount++ } diff --git a/core/util-jvm/src/test/java/org/signal/core/util/BidiUtilTest.kt b/core/util-jvm/src/test/java/org/signal/core/util/BidiUtilTest.kt index 93fd912cd8..b3f0aab632 100644 --- a/core/util-jvm/src/test/java/org/signal/core/util/BidiUtilTest.kt +++ b/core/util-jvm/src/test/java/org/signal/core/util/BidiUtilTest.kt @@ -1,11 +1,39 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + package org.signal.core.util import org.junit.Assert.assertEquals import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Test class BidiUtilTest { + companion object { + // Isolate initiators + private const val LRI = "⁦" + private const val RLI = "⁧" + private const val FSI = "⁨" + + // Closes any isolate initiator + private const val PDI = "⁩" + + // Override initiators + private const val LRE = "‪" + private const val RLE = "‫" + private const val LRO = "‭" + private const val RLO = "‮" + + // Closes any override initiator + private const val PDF = "‬" + + // A supplementary code point (grinning face emoji), encoded as a surrogate pair. + private const val EMOJI = "😀" + } + private val replacement = "�" @Test @@ -20,19 +48,19 @@ class BidiUtilTest { @Test fun replaceBidiCharacters_overrides_areReplaced() { - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("‪")) // LRE - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("‫")) // RLE - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("‬")) // PDF - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("‭")) // LRO - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("‮")) // RLO + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(LRE)) // LRE + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(RLE)) // RLE + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(PDF)) // PDF + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(LRO)) // LRO + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(RLO)) // RLO } @Test fun replaceBidiCharacters_isolates_areReplaced() { - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("⁦")) // LRI - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("⁧")) // RLI - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("⁨")) // FSI - assertEquals("$replacement", BidiUtil.replaceBidiCharacters("⁩")) // PDI + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(LRI)) // LRI + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(RLI)) // RLI + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(FSI)) // FSI + assertEquals("$replacement", BidiUtil.replaceBidiCharacters(PDI)) // PDI } @Test @@ -43,16 +71,124 @@ class BidiUtilTest { @Test fun replaceBidiCharacters_spoofedExtension_isNeutralized() { // "documenttxt.exe" would render as "documentexe.txt" without sanitization. - val malicious = "document‮txt.exe⁩" + val malicious = "document" + RLO + "txt.exe" + PDI val cleaned = BidiUtil.replaceBidiCharacters(malicious) assertEquals("document${replacement}txt.exe$replacement", cleaned) - assertEquals(0, cleaned!!.count { it == '‮' || it == '⁩' }) + assertEquals(0, cleaned!!.count { it == RLO[0] || it == PDI[0] }) } @Test fun replaceBidiCharacters_multipleControls_allReplaced() { - val input = "a‫b⁦c‮d" + val input = "a" + RLE + "b" + LRI + "c" + RLO + "d" assertEquals("a${replacement}b${replacement}c${replacement}d", BidiUtil.replaceBidiCharacters(input)) } + + @Test + fun isolateBidi_returnsEmptyForNull() { + assertEquals("", BidiUtil.isolateBidi(null)) + } + + @Test + fun isolateBidi_returnsEmptyForEmpty() { + assertEquals("", BidiUtil.isolateBidi("")) + } + + @Test + fun isolateBidi_passesThroughPlainAscii() { + assertEquals("Hello, World!", BidiUtil.isolateBidi("Hello, World!")) + } + + @Test + fun isolateBidi_wrapsNonAsciiInIsolate() { + // No bidi controls, just non-ASCII text. It should simply be wrapped in FSI...PDI. + assertEquals(FSI + "café" + PDI, BidiUtil.isolateBidi("café")) + } + + @Test + fun isolateBidi_balancedIsolateIsLeftAlone() { + val input = RLI + "x" + PDI + assertEquals(FSI + input + PDI, BidiUtil.isolateBidi(input)) + } + + @Test + fun isolateBidi_unmatchedIsolateInitiatorIsClosedWithPdi() { + // The core of the report: an unmatched isolate initiator must be terminated with PDI, + // NOT with another isolate initiator (FSI), which would open yet another scope. + val result = BidiUtil.isolateBidi(RLI + "evil") + assertEquals(FSI + RLI + "evil" + PDI + PDI, result) + assertBalanced(result) + } + + @Test + fun isolateBidi_multipleUnmatchedIsolateInitiatorsEachGetPdi() { + val result = BidiUtil.isolateBidi(LRI + RLI + "evil") + assertEquals(FSI + LRI + RLI + "evil" + PDI + PDI + PDI, result) + assertBalanced(result) + } + + @Test + fun isolateBidi_unmatchedOverrideIsClosedWithPdf() { + val result = BidiUtil.isolateBidi(RLO + "evil") + assertEquals(FSI + RLO + "evil" + PDF + PDI, result) + assertBalanced(result) + } + + @Test + fun isolateBidi_mixedUnmatchedOverridesAndIsolates() { + val result = BidiUtil.isolateBidi(LRE + RLI + "evil") + // Overrides are closed first, then isolates. + assertEquals(FSI + LRE + RLI + "evil" + PDF + PDI + PDI, result) + assertBalanced(result) + } + + @Test + fun isolateBidi_extraClosersAreNotOverBalanced() { + // Already-closed (or over-closed) controls should not produce extra suffix characters. + val input = "x" + PDI + PDF + assertEquals(FSI + input + PDI, BidiUtil.isolateBidi(input)) + } + + @Test + fun isolateBidi_countsControlAfterSupplementaryCodePoint() { + // Regression test for the code-point iteration bug: a surrogate pair before a bidi + // control used to cause the loop to stop short and miss the trailing initiator, + // leaving it unterminated. + val result = BidiUtil.isolateBidi(EMOJI + RLI + "evil") + assertEquals(FSI + EMOJI + RLI + "evil" + PDI + PDI, result) + assertBalanced(result) + } + + @Test + fun isolateBidi_handlesSupplementaryCodePointBetweenControls() { + val result = BidiUtil.isolateBidi(RLI + EMOJI + RLO + "evil") + assertEquals(FSI + RLI + EMOJI + RLO + "evil" + PDF + PDI + PDI, result) + assertBalanced(result) + } + + /** + * Asserts that the isolate and override scopes in [text] are fully balanced: depth never + * goes negative and ends at exactly zero for both isolates and overrides. This is the + * security-relevant property — no attacker-controlled scope may leak past the isolated segment. + */ + private fun assertBalanced(text: String) { + var isolateDepth = 0 + var overrideDepth = 0 + + var i = 0 + while (i < text.length) { + when (text.codePointAt(i)) { + LRI.codePointAt(0), RLI.codePointAt(0), FSI.codePointAt(0) -> isolateDepth++ + PDI.codePointAt(0) -> isolateDepth-- + LRE.codePointAt(0), RLE.codePointAt(0), LRO.codePointAt(0), RLO.codePointAt(0) -> overrideDepth++ + PDF.codePointAt(0) -> overrideDepth-- + } + assertTrue("Isolate depth went negative in: $text", isolateDepth >= 0) + assertTrue("Override depth went negative in: $text", overrideDepth >= 0) + i += Character.charCount(text.codePointAt(i)) + } + + assertEquals("Unterminated isolate(s) in: $text", 0, isolateDepth) + assertEquals("Unterminated override(s) in: $text", 0, overrideDepth) + } }