diff --git a/app/src/main/java/com/ngoline/easygpg/PGPKeyManager.kt b/app/src/main/java/com/ngoline/easygpg/PGPKeyManager.kt index 21f10c9..6b8b1cd 100644 --- a/app/src/main/java/com/ngoline/easygpg/PGPKeyManager.kt +++ b/app/src/main/java/com/ngoline/easygpg/PGPKeyManager.kt @@ -770,17 +770,29 @@ class PGPKeyManager(private val context: Context) { return prefs.getBoolean(context.getString(R.string.obfuscate_pgp_markers), false) } - private fun obfuscateMarkers(input: String): String { - // Replace all PGP marker lines and version/comment lines with 00023CD1 - return input.lines().joinToString("") { line -> - if (line.startsWith("-----BEGIN ") || - line.startsWith("-----END ") || - line.isBlank()) PGPConstants.OBFUSCATED_MARKER - else if (line.startsWith("Version:") || - line.startsWith("Comment:")) "" - else line + /** + * Replaces the armor header and footer with [PGPConstants.OBFUSCATED_MARKER]. + * + * Blank lines are dropped rather than replaced. Armored output carries one after the version + * header and another at the end, so replacing them emitted the marker twice at each end — and + * a doubled sixteen-character hex run is a more distinctive signature than a single one, which + * works against the point of obfuscating at all. + * + * Internal rather than private so the round trip can be tested directly. + */ + @VisibleForTesting + internal fun obfuscateMarkers(input: String): String = + input.lines().joinToString("") { line -> + when { + line.startsWith("-----BEGIN ") || line.startsWith("-----END ") -> + PGPConstants.OBFUSCATED_MARKER + // Version and Comment identify the implementation; blank lines carry nothing. + // Deobfuscation re-chunks the base64 regardless, so neither is needed. + line.isBlank() -> "" + line.startsWith("Version:") || line.startsWith("Comment:") -> "" + else -> line + } } - } /** Internal rather than private only so the obfuscation round trip can be tested directly. */ @VisibleForTesting diff --git a/app/src/test/java/com/ngoline/easygpg/ObfuscationTest.kt b/app/src/test/java/com/ngoline/easygpg/ObfuscationTest.kt new file mode 100644 index 0000000..3814bc5 --- /dev/null +++ b/app/src/test/java/com/ngoline/easygpg/ObfuscationTest.kt @@ -0,0 +1,116 @@ +package com.ngoline.easygpg + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Marker obfuscation exists to stop a message looking like PGP, so what it emits matters as much + * as whether it round trips. Both are pinned here. + */ +@RunWith(RobolectricTestRunner::class) +class ObfuscationTest { + + private lateinit var manager: PGPKeyManager + private val marker = PGPConstants.OBFUSCATED_MARKER + + /** What Bouncy Castle actually produces: a version header, a blank line, then the body. */ + private val armored = buildString { + append("-----BEGIN PGP MESSAGE-----\n") + append("Version: BCPG v1.81\n") + append("\n") + append("wV4DTmkMQQGcaxcSAQdANo1t8BWzhOFza9jwZQIc7uS59MO5E4gmVyK5laDT0w\n") + append("7TgjnZ6sV1Qccg1SHgYRLREAHraEeRmWVwelavItS5HvTJ2LRU6NFrZV6wlka\n") + append("=x9Yz\n") + append("-----END PGP MESSAGE-----\n") + } + + @Before + fun setUp() { + installBouncyCastleProvider() + manager = PGPKeyManager(ApplicationProvider.getApplicationContext()) + } + + @Test + fun `the marker is not repeated back to back`() { + // The bug this guards: blank lines were replaced with the marker too, and armored output + // has one after the version header and one at the end, so the marker appeared twice at + // each end. A doubled hex run is a more distinctive signature than a single one. + val obfuscated = manager.obfuscateMarkers(armored) + + assertFalse( + "marker appears twice in a row: $obfuscated", + obfuscated.contains(marker + marker), + ) + } + + @Test + fun `there is exactly one marker at each end`() { + val obfuscated = manager.obfuscateMarkers(armored) + + assertEquals("expected two markers in total, in $obfuscated", 2, + Regex(Regex.escape(marker)).findAll(obfuscated).count()) + assertTrue(obfuscated, obfuscated.startsWith(marker)) + assertTrue(obfuscated, obfuscated.endsWith(marker)) + } + + @Test + fun `no PGP armor survives obfuscation`() { + val obfuscated = manager.obfuscateMarkers(armored) + + assertFalse(obfuscated, obfuscated.contains("-----BEGIN")) + assertFalse(obfuscated, obfuscated.contains("-----END")) + assertFalse("the version header identifies the implementation", obfuscated.contains("BCPG")) + } + + @Test + fun `the payload itself is untouched`() { + val obfuscated = manager.obfuscateMarkers(armored) + + assertTrue(obfuscated, obfuscated.contains("wV4DTmkMQQGcaxcSAQdANo1t8BWzhOFza9jwZQIc7uS59MO5E4gmVyK5laDT0w")) + assertTrue("the checksum must survive", obfuscated.contains("=x9Yz")) + } + + @Test + fun `obfuscating and deobfuscating returns usable armor`() { + val restored = manager.deobfuscateMarkers(manager.obfuscateMarkers(armored)) + + assertTrue(restored, restored.trimStart().startsWith("-----BEGIN PGP MESSAGE-----")) + assertTrue(restored, restored.trimEnd().endsWith("-----END PGP MESSAGE-----")) + // The body has to come back byte for byte, or the message will not decrypt. + val body = { s: String -> s.lines().filter { it.isNotBlank() } + .filterNot { it.startsWith("-----") || it.startsWith("Version:") } + .joinToString("") } + assertEquals(body(armored), body(restored)) + } + + @Test + fun `messages obfuscated by the previous version still deobfuscate`() { + // Anything already sent carries the doubled markers. Deobfuscation strips every + // occurrence, so those must keep working. + val oldFormat = marker + marker + + "wV4DTmkMQQGcaxcSAQdANo1t8BWzhOFza9jwZQIc7uS59MO5E4gmVyK5laDT0w" + + "7TgjnZ6sV1Qccg1SHgYRLREAHraEeRmWVwelavItS5HvTJ2LRU6NFrZV6wlka" + + "=x9Yz" + marker + marker + + val restored = manager.deobfuscateMarkers(oldFormat) + + assertTrue(restored, restored.trimStart().startsWith("-----BEGIN PGP MESSAGE-----")) + assertFalse("a marker leaked into the output", restored.contains(marker)) + assertTrue("the checksum was lost", restored.contains("=x9Yz")) + } + + @Test + fun `an obfuscated message is still recognised as one`() { + // decryptMessage and the notification listener both detect by leading marker. + val obfuscated = manager.obfuscateMarkers(armored) + + assertTrue(obfuscated.trimStart().startsWith(marker)) + } +}