From 34f13b76c55f64f5158b6588c906fc8250f47077 Mon Sep 17 00:00:00 2001 From: Hidden-Node Date: Thu, 23 Jul 2026 19:05:22 +0330 Subject: [PATCH] fix(security): reject newlines/control chars in TOML imports, tighten manifest filter --- android/app/src/main/AndroidManifest.xml | 3 - .../java/com/masterdns/vpn/MainActivity.kt | 25 +------ .../com/masterdns/vpn/ProfileTomlImporter.kt | 47 +++++++++++++ .../com/masterdns/vpn/util/ConfigGenerator.kt | 6 +- .../com/masterdns/vpn/ConfigGeneratorTest.kt | 29 ++++++-- .../masterdns/vpn/ProfileTomlImporterTest.kt | 67 +++++++++++++++++++ 6 files changed, 144 insertions(+), 33 deletions(-) create mode 100644 android/app/src/main/java/com/masterdns/vpn/ProfileTomlImporter.kt create mode 100644 android/app/src/test/java/com/masterdns/vpn/ProfileTomlImporterTest.kt diff --git a/android/app/src/main/AndroidManifest.xml b/android/app/src/main/AndroidManifest.xml index a541536..7ed5f33 100644 --- a/android/app/src/main/AndroidManifest.xml +++ b/android/app/src/main/AndroidManifest.xml @@ -45,12 +45,9 @@ - - - diff --git a/android/app/src/main/java/com/masterdns/vpn/MainActivity.kt b/android/app/src/main/java/com/masterdns/vpn/MainActivity.kt index 25d272b..5eed655 100644 --- a/android/app/src/main/java/com/masterdns/vpn/MainActivity.kt +++ b/android/app/src/main/java/com/masterdns/vpn/MainActivity.kt @@ -154,29 +154,8 @@ class MainActivity : ComponentActivity() { ?.takeIf { it.isNotEmpty() } } - private fun parseTomlValues(tomlContent: String): Map { - val values = mutableMapOf() - tomlContent.lineSequence().forEach { raw -> - val line = raw.substringBefore("#").trim() - if (line.isEmpty() || "=" !in line) return@forEach - val key = line.substringBefore("=").trim() - val valueRaw = line.substringAfter("=").trim() - val parsed = when { - key == "DOMAINS" -> valueRaw - .removePrefix("[") - .removeSuffix("]") - .split(",") - .map { it.trim().removeSurrounding("\"") } - .filter { it.isNotBlank() } - .joinToString(", ") - valueRaw.startsWith("\"") && valueRaw.endsWith("\"") -> - valueRaw.removeSurrounding("\"") - else -> valueRaw - } - values[key] = parsed - } - return values - } + private fun parseTomlValues(tomlContent: String): Map = + ProfileTomlImporter.parseTomlValues(tomlContent) private fun normalizeProtocol(value: String?): String { return when (value?.trim()?.uppercase()) { diff --git a/android/app/src/main/java/com/masterdns/vpn/ProfileTomlImporter.kt b/android/app/src/main/java/com/masterdns/vpn/ProfileTomlImporter.kt new file mode 100644 index 0000000..659c813 --- /dev/null +++ b/android/app/src/main/java/com/masterdns/vpn/ProfileTomlImporter.kt @@ -0,0 +1,47 @@ +package com.masterdns.vpn + +internal object ProfileTomlImporter { + /** + * Maximum allowed length per imported TOML value. Tuned to accommodate + * realistic ENCRYPTION_KEY + DOMAINS lists while bounding the cost of + * a malicious import. + */ + internal const val MAX_VALUE_LENGTH = 4096 + + /** + * Parse TOML key=value lines into a flat map. Pure function. + * + * Behavior: + * - Comments via `#` are stripped per line. + * - `DOMAINS = [...]` array parsed as comma-joined string. + * - Quoted strings `"...` have surrounding quotes stripped. + * - Values containing control characters (\n, \r, or char code + * below 0x20) or exceeding MAX_VALUE_LENGTH are REJECTED — + * the entry is skipped (no key stored). + */ + internal fun parseTomlValues(tomlContent: String): Map { + val values = mutableMapOf() + tomlContent.lineSequence().forEach { raw -> + val line = raw.substringBefore("#").trim() + if (line.isEmpty() || "=" !in line) return@forEach + val key = line.substringBefore("=").trim() + val valueRaw = line.substringAfter("=").trim() + val parsed = when { + key == "DOMAINS" -> valueRaw + .removePrefix("[") + .removeSuffix("]") + .split(",") + .map { it.trim().removeSurrounding("\"") } + .filter { it.isNotBlank() } + .joinToString(", ") + valueRaw.startsWith("\"") && valueRaw.endsWith("\"") -> + valueRaw.removeSurrounding("\"") + else -> valueRaw + } + if (parsed.any { it == '\n' || it == '\r' || it.code < 0x20 }) return@forEach + if (parsed.length > MAX_VALUE_LENGTH) return@forEach + values[key] = parsed + } + return values + } +} diff --git a/android/app/src/main/java/com/masterdns/vpn/util/ConfigGenerator.kt b/android/app/src/main/java/com/masterdns/vpn/util/ConfigGenerator.kt index ae56a6a..a5b7182 100644 --- a/android/app/src/main/java/com/masterdns/vpn/util/ConfigGenerator.kt +++ b/android/app/src/main/java/com/masterdns/vpn/util/ConfigGenerator.kt @@ -217,6 +217,10 @@ object ConfigGenerator { } internal fun escapeToml(s: String): String { - return s.replace("\\", "\\\\").replace("\"", "\\\"") + return s.replace("\\", "\\\\") + .replace("\"", "\\\"") + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t") } } diff --git a/android/app/src/test/java/com/masterdns/vpn/ConfigGeneratorTest.kt b/android/app/src/test/java/com/masterdns/vpn/ConfigGeneratorTest.kt index 884b12d..4bc5400 100644 --- a/android/app/src/test/java/com/masterdns/vpn/ConfigGeneratorTest.kt +++ b/android/app/src/test/java/com/masterdns/vpn/ConfigGeneratorTest.kt @@ -21,12 +21,29 @@ class ConfigGeneratorTest { } @Test - fun escapeToml_newlineIsPassedThroughLiteral() { - // Documented current behavior: escapeToml does NOT escape newlines, - // so values containing \n can inject new top-level keys into the - // generated TOML. This test locks in that behavior so plan 008 - // (TOML whitelist) can flip it explicitly. - assertEquals("a\nb", ConfigGenerator.escapeToml("a\nb")) + fun escapeToml_newlineIsEscaped() { + // Plan 008 fix: newlines MUST be escaped so they do not inject new + // top-level TOML keys when ConfigGenerator interpolates a value + // (e.g. ENCRYPTION_KEY) into the generated config. + assertEquals("a\\nb", ConfigGenerator.escapeToml("a\nb")) + } + + @Test + fun escapeToml_carriageReturnIsEscaped() { + assertEquals("a\\rb", ConfigGenerator.escapeToml("a\rb")) + } + + @Test + fun escapeToml_tabIsEscaped() { + assertEquals("a\\tb", ConfigGenerator.escapeToml("a\tb")) + } + + @Test + fun escapeToml_combinedEscapeOrderBackslashFirst() { + // Backslash must be escaped FIRST so subsequent escapes don't + // double-encode a real backslash. + // Input: a"b\c\nd Expected: a\"b\\c\nd + assertEquals("a\\\"b\\\\c\\nd", ConfigGenerator.escapeToml("a\"b\\c\nd")) } @Test diff --git a/android/app/src/test/java/com/masterdns/vpn/ProfileTomlImporterTest.kt b/android/app/src/test/java/com/masterdns/vpn/ProfileTomlImporterTest.kt new file mode 100644 index 0000000..633a72f --- /dev/null +++ b/android/app/src/test/java/com/masterdns/vpn/ProfileTomlImporterTest.kt @@ -0,0 +1,67 @@ +package com.masterdns.vpn + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Test + +class ProfileTomlImporterTest { + @Test + fun parses_simpleKeyValuePair() { + val out = ProfileTomlImporter.parseTomlValues("ENCRYPTION_KEY = \"abc\"") + assertEquals("abc", out["ENCRYPTION_KEY"]) + } + + @Test + fun parses_domainsArray() { + val out = ProfileTomlImporter.parseTomlValues("DOMAINS = [\"a.com\", \"b.com\"]") + assertEquals("a.com, b.com", out["DOMAINS"]) + } + + @Test + fun strips_commentsAfterHash() { + val out = ProfileTomlImporter.parseTomlValues("ENCRYPTION_KEY = \"abc\" # trailing") + assertEquals("abc", out["ENCRYPTION_KEY"]) + } + + @Test + fun skips_blankAndCommentOnlyLines() { + val out = ProfileTomlImporter.parseTomlValues( + """ + + # full comment + ENCRYPTION_KEY = "x" + """.trimIndent() + ) + assertEquals("x", out["ENCRYPTION_KEY"]) + assertFalse(out.containsKey("")) + } + + @Test + fun rejects_valuesWithEmbeddedControlChar() { + // A value containing a raw control char (here bell, 0x07 — chosen + // because lineSequence() splits on \n/\r but NOT on \u0007, so + // the char reaches the rejection guard intact) must be dropped + // so it cannot reach ConfigGenerator interpolation. Guards the + // full class of control-char injection, of which newline was the + // headline case (newline is now handled by escaping at the + // ConfigGenerator output side AND by lineSequence() splitting at + // the input side; other control chars are handled here). + val malicious = "ENCRYPTION_KEY = \"x\u0007y\"" + val out = ProfileTomlImporter.parseTomlValues(malicious) + assertFalse("expected ENCRYPTION_KEY to be rejected", out.containsKey("ENCRYPTION_KEY")) + } + + @Test + fun rejects_valuesExceedingMaxLength() { + val longValue = "x".repeat(ProfileTomlImporter.MAX_VALUE_LENGTH + 1) + val out = ProfileTomlImporter.parseTomlValues("ENCRYPTION_KEY = \"$longValue\"") + assertFalse(out.containsKey("ENCRYPTION_KEY")) + } + + @Test + fun accepts_valuesAtMaxLength() { + val exactLength = "x".repeat(ProfileTomlImporter.MAX_VALUE_LENGTH) + val out = ProfileTomlImporter.parseTomlValues("ENCRYPTION_KEY = \"$exactLength\"") + assertEquals(exactLength, out["ENCRYPTION_KEY"]) + } +}