From e8fd432f14783fddca728e79c1ef48e0eabdde2b Mon Sep 17 00:00:00 2001 From: Graham Date: Wed, 10 Feb 2021 18:17:26 +0000 Subject: [PATCH] Combine key validation with uncompression As key validation has to uncompress the data anyway to confirm the key is valid, it seems silly to uncompress twice given everywhere we performed key validation immediately uncompressed the container afterwards. Signed-off-by: Graham --- .../openrs2/archive/cache/CacheImporter.kt | 8 +- .../archive/cache/Js5ChannelHandler.kt | 8 +- .../org/openrs2/archive/key/KeyBruteForcer.kt | 49 ++-- .../org/openrs2/cache/Js5Compression.kt | 70 ++++-- .../org/openrs2/cache/Js5CompressionTest.kt | 224 +++++++++++++----- 5 files changed, 238 insertions(+), 121 deletions(-) diff --git a/archive/src/main/kotlin/org/openrs2/archive/cache/CacheImporter.kt b/archive/src/main/kotlin/org/openrs2/archive/cache/CacheImporter.kt index 59a314fc59..7d3e03c513 100644 --- a/archive/src/main/kotlin/org/openrs2/archive/cache/CacheImporter.kt +++ b/archive/src/main/kotlin/org/openrs2/archive/cache/CacheImporter.kt @@ -537,13 +537,7 @@ public class CacheImporter @Inject constructor( } } - // TODO(gpe): avoid uncompressing twice (we do it in isEncrypted and uncompress) - val uncompressed = if (Js5Compression.isEncrypted(buf.slice())) { - null - } else { - Js5Compression.uncompress(buf.slice()) - } - + val uncompressed = Js5Compression.uncompressUnlessEncrypted(buf.slice()) return Group(archive, group, buf.retain(), uncompressed, version, versionTruncated) } } catch (ex: IOException) { diff --git a/archive/src/main/kotlin/org/openrs2/archive/cache/Js5ChannelHandler.kt b/archive/src/main/kotlin/org/openrs2/archive/cache/Js5ChannelHandler.kt index 94a215c30e..d0b547d990 100644 --- a/archive/src/main/kotlin/org/openrs2/archive/cache/Js5ChannelHandler.kt +++ b/archive/src/main/kotlin/org/openrs2/archive/cache/Js5ChannelHandler.kt @@ -133,13 +133,7 @@ public class Js5ChannelHandler( throw Exception("Group checksum invalid") } - // TODO(gpe): avoid uncompressing twice (we do it in isEncrypted and uncompress) - val uncompressed = if (Js5Compression.isEncrypted(response.data.slice())) { - null - } else { - Js5Compression.uncompress(response.data.slice()) - } - + val uncompressed = Js5Compression.uncompressUnlessEncrypted(response.data.slice()) groups += CacheImporter.Group( response.archive, response.group, diff --git a/archive/src/main/kotlin/org/openrs2/archive/key/KeyBruteForcer.kt b/archive/src/main/kotlin/org/openrs2/archive/key/KeyBruteForcer.kt index e2475843a3..47e27dfd21 100644 --- a/archive/src/main/kotlin/org/openrs2/archive/key/KeyBruteForcer.kt +++ b/archive/src/main/kotlin/org/openrs2/archive/key/KeyBruteForcer.kt @@ -259,40 +259,29 @@ public class KeyBruteForcer @Inject constructor( keyId: Long, containerId: Long ): Boolean { - val valid = Unpooled.wrappedBuffer(data).use { buf -> - Js5Compression.isKeyValid(buf, key) - } - - if (!valid) { - return false - } + Unpooled.wrappedBuffer(data).use { buf -> + Js5Compression.uncompressIfKeyValid(buf, key).use { uncompressed -> + if (uncompressed == null) { + return false + } - // TODO(gpe): avoid uncompressing twice (we do it here and in isKeyValid) - var len = 0 - var crc32 = 0 + connection.prepareStatement( + """ + UPDATE containers + SET key_id = ?, uncompressed_length = ?, uncompressed_crc32 = ? + WHERE id = ? + """.trimIndent() + ).use { stmt -> + stmt.setLong(1, keyId) + stmt.setInt(2, uncompressed.readableBytes()) + stmt.setInt(3, uncompressed.crc32()) + stmt.setLong(4, containerId) + stmt.execute() + } - Unpooled.wrappedBuffer(data).use { buf -> - Js5Compression.uncompress(buf, key).use { uncompressed -> - len = uncompressed.readableBytes() - crc32 = uncompressed.crc32() + return true } } - - connection.prepareStatement( - """ - UPDATE containers - SET key_id = ?, uncompressed_length = ?, uncompressed_crc32 = ? - WHERE id = ? - """.trimIndent() - ).use { stmt -> - stmt.setLong(1, keyId) - stmt.setInt(2, len) - stmt.setInt(3, crc32) - stmt.setLong(4, containerId) - stmt.execute() - } - - return true } private companion object { diff --git a/cache/src/main/kotlin/org/openrs2/cache/Js5Compression.kt b/cache/src/main/kotlin/org/openrs2/cache/Js5Compression.kt index ff09f51d39..3331840ee7 100644 --- a/cache/src/main/kotlin/org/openrs2/cache/Js5Compression.kt +++ b/cache/src/main/kotlin/org/openrs2/cache/Js5Compression.kt @@ -8,7 +8,6 @@ import org.openrs2.crypto.XteaKey import org.openrs2.crypto.xteaDecrypt import org.openrs2.crypto.xteaEncrypt import java.io.IOException -import java.io.OutputStream public object Js5Compression { private val BZIP2_MAGIC = byteArrayOf(0x31, 0x41, 0x59, 0x26, 0x53, 0x59) @@ -163,11 +162,11 @@ public object Js5Compression { } } - public fun isEncrypted(input: ByteBuf): Boolean { - return !isKeyValid(input, XteaKey.ZERO) + public fun uncompressUnlessEncrypted(input: ByteBuf): ByteBuf? { + return uncompressIfKeyValid(input, XteaKey.ZERO) } - public fun isKeyValid(input: ByteBuf, key: XteaKey): Boolean { + public fun uncompressIfKeyValid(input: ByteBuf, key: XteaKey): ByteBuf? { val typeId = input.readUnsignedByte().toInt() val type = Js5CompressionType.fromOrdinal(typeId) ?: throw IOException("Invalid compression type: $typeId") @@ -178,6 +177,10 @@ public object Js5Compression { } if (type == Js5CompressionType.UNCOMPRESSED) { + if (input.readableBytes() < len) { + throw IOException("Data truncated") + } + /* * There is no easy way for us to be sure whether an uncompressed * group's key is valid or not, as we'd need specific validation @@ -191,7 +194,11 @@ public object Js5Compression { * * We therefore assume all uncompressed groups are unencrypted. */ - return key.isZero + if (!key.isZero) { + return null + } + + return input.readBytes(len) } val lenWithUncompressedLen = len + 4 @@ -223,7 +230,7 @@ public object Js5Compression { decrypt(input.slice(), 16, key).use { plaintext -> val uncompressedLen = plaintext.readInt() if (uncompressedLen < 0) { - return false + return null } when (type) { @@ -232,19 +239,19 @@ public object Js5Compression { val magic = ByteArray(BZIP2_MAGIC.size) plaintext.readBytes(magic) if (!magic.contentEquals(BZIP2_MAGIC)) { - return false + return null } } Js5CompressionType.GZIP -> { val magic = plaintext.readUnsignedShort() if (magic != GZIP_MAGIC) { - return false + return null } // Jagex's implementation only supports DEFLATE. val compressionMethod = plaintext.readUnsignedByte().toInt() if (compressionMethod != GZIP_COMPRESSION_METHOD_DEFLATE) { - return false + return null } } Js5CompressionType.LZMA -> { @@ -257,7 +264,7 @@ public object Js5Compression { val pb = properties / 45 if (pb > LZMA_PB_MAX) { - return false + return null } /* @@ -279,9 +286,9 @@ public object Js5Compression { */ val dictSize = plaintext.readIntLE() if (dictSize < 0) { - return false + return null } else if (dictSize > LZMA_PRESET_DICT_SIZE_MAX) { - return false + return null } } } @@ -292,20 +299,43 @@ public object Js5Compression { val uncompressedLen = plaintext.readInt() check(uncompressedLen >= 0) - try { - OutputStream.nullOutputStream().use { output -> + /** + * We don't pass uncompressedLen to the buffer here: in some cases, + * an incorrect key can produce a valid header (particularly for + * LZMA, which has no magic number). If we're unlucky, + * uncompressedLen will be a huge number (e.g. 1 or 2 gigabytes), + * which might OOM some environments if allocated up front. + * + * However, if the key is incorrect it's likely that actually + * attempting to uncompress the data will quickly produce an error, + * long before we need to actually read 1 or 2 gigabytes of data. + * We therefore allow the buffer to grow dynamically. + */ + plaintext.alloc().buffer().use { output -> + try { type.createInputStream(ByteBufInputStream(plaintext, len), uncompressedLen).use { inputStream -> - if (inputStream.transferTo(output) != uncompressedLen.toLong()) { - return false + var remaining = uncompressedLen + while (remaining > 0) { + val n = output.writeBytes(inputStream, remaining) + if (n == -1) { + // uncompressed data truncated + return null + } + remaining -= n + } + + if (inputStream.read() != -1) { + // uncompressed data overflow + return null } } + } catch (ex: IOException) { + return null } - } catch (ex: IOException) { - return false + + return output.retain() } } - - return true } private fun decrypt(buf: ByteBuf, len: Int, key: XteaKey): ByteBuf { diff --git a/cache/src/test/kotlin/org/openrs2/cache/Js5CompressionTest.kt b/cache/src/test/kotlin/org/openrs2/cache/Js5CompressionTest.kt index 571d91a6e9..eec4cc545d 100644 --- a/cache/src/test/kotlin/org/openrs2/cache/Js5CompressionTest.kt +++ b/cache/src/test/kotlin/org/openrs2/cache/Js5CompressionTest.kt @@ -9,9 +9,8 @@ import java.io.IOException import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith -import kotlin.test.assertFalse import kotlin.test.assertNotEquals -import kotlin.test.assertTrue +import kotlin.test.assertNull class Js5CompressionTest { @Test @@ -266,7 +265,7 @@ class Js5CompressionTest { } assertFailsWith { - Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO)?.release() } } } @@ -279,7 +278,7 @@ class Js5CompressionTest { } assertFailsWith { - Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO)?.release() } } } @@ -291,7 +290,9 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @@ -311,7 +312,9 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @@ -322,7 +325,9 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @@ -333,7 +338,9 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @@ -344,7 +351,9 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @@ -355,7 +364,9 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @@ -366,92 +377,191 @@ class Js5CompressionTest { Js5Compression.uncompress(compressed.slice()).release() } - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { uncompressed -> + assertNull(uncompressed) + } } } @Test fun testNoneKeyValid() { - read("none.dat").use { compressed -> - assertFalse(Js5Compression.isEncrypted(compressed.slice())) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) + copiedBuffer("OpenRS2").use { expected -> + read("none.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } } } @Test fun testBzip2KeyValid() { - read("bzip2.dat").use { compressed -> - assertFalse(Js5Compression.isEncrypted(compressed.slice())) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) - } + copiedBuffer("OpenRS2").use { expected -> + read("bzip2.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertEquals(expected, actual) + } - read("bzip2-encrypted.dat").use { compressed -> - assertTrue(Js5Compression.isEncrypted(compressed.slice())) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } + + read("bzip2-encrypted.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } } read("bzip2-invalid-magic.dat").use { compressed -> - assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual -> + assertNull(actual) + } } } @Test fun testGzipKeyValid() { - read("gzip.dat").use { compressed -> - assertFalse(Js5Compression.isEncrypted(compressed.slice())) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) - } + copiedBuffer("OpenRS2").use { expected -> + read("gzip.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertEquals(expected, actual) + } - read("gzip-encrypted.dat").use { compressed -> - assertTrue(Js5Compression.isEncrypted(compressed.slice())) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } + + read("gzip-encrypted.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } } read("gzip-invalid-magic.dat").use { compressed -> - assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual -> + assertNull(actual) + } } read("gzip-invalid-method.dat").use { compressed -> - assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual -> + assertNull(actual) + } } } @Test fun testLzmaKeyValid() { - read("lzma.dat").use { compressed -> - assertFalse(Js5Compression.isEncrypted(compressed.slice())) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) - } + copiedBuffer("OpenRS2").use { expected -> + read("lzma.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } + + read("lzma-encrypted.dat").use { compressed -> + Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual -> + assertNull(actual) + } - read("lzma-encrypted.dat").use { compressed -> - assertTrue(Js5Compression.isEncrypted(compressed.slice())) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)) - assertTrue(Js5Compression.isKeyValid(compressed.slice(), KEY)) - assertFalse(Js5Compression.isKeyValid(compressed.slice(), INVALID_KEY)) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO).use { actual -> + assertNull(actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), KEY).use { actual -> + assertEquals(expected, actual) + } + + Js5Compression.uncompressIfKeyValid(compressed.slice(), INVALID_KEY).use { actual -> + assertNull(actual) + } + } } read("lzma-dict-size-negative.dat").use { compressed -> - assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual -> + assertNull(actual) + } } read("lzma-dict-size-larger-than-preset.dat").use { compressed -> - assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual -> + assertNull(actual) + } } read("lzma-invalid-pb.dat").use { compressed -> - assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO)) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual -> + assertNull(actual) + } } } @@ -459,7 +569,7 @@ class Js5CompressionTest { fun testKeyValidShorterThanTwoBlocks() { read("shorter-than-two-blocks.dat").use { compressed -> assertFailsWith { - Js5Compression.isKeyValid(compressed, XteaKey.ZERO) + Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO)?.release() } } } @@ -472,7 +582,7 @@ class Js5CompressionTest { } assertFailsWith { - Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO) + Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO)?.release() } } }