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 <gpe@openrs2.org>

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 = Js5Compression.uncompressUnlessEncrypted(buf.slice())
-                val uncompressed = if (Js5Compression.isEncrypted(buf.slice())) {
-                    null
-                } else {
-                    Js5Compression.uncompress(buf.slice())
-                }
-
                 return Group(archive, group, buf.retain(), uncompressed, version, versionTruncated)
             }
         } catch (ex: IOException) {

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 = Js5Compression.uncompressUnlessEncrypted(response.data.slice())
-            val uncompressed = if (Js5Compression.isEncrypted(response.data.slice())) {
-                null
-            } else {
-                Js5Compression.uncompress(response.data.slice())
-            }
-
             groups += CacheImporter.Group(
                 response.archive,
                 response.group,

archive/src/main/kotlin/org/openrs2/archive/key/KeyBruteForcer.kt

@@ -259,23 +259,10 @@ 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
-        }
-
-        // TODO(gpe): avoid uncompressing twice (we do it here and in isKeyValid)
-        var len = 0
-        var crc32 = 0
-
         Unpooled.wrappedBuffer(data).use { buf ->
-            Js5Compression.uncompress(buf, key).use { uncompressed ->
+            Js5Compression.uncompressIfKeyValid(buf, key).use { uncompressed ->
-                len = uncompressed.readableBytes()
+                if (uncompressed == null) {
-                crc32 = uncompressed.crc32()
+                    return false
-            }
                 }
 
                 connection.prepareStatement(
@@ -286,14 +273,16 @@ public class KeyBruteForcer @Inject constructor(
                 """.trimIndent()
                 ).use { stmt ->
                     stmt.setLong(1, keyId)
-            stmt.setInt(2, len)
+                    stmt.setInt(2, uncompressed.readableBytes())
-            stmt.setInt(3, crc32)
+                    stmt.setInt(3, uncompressed.crc32())
                     stmt.setLong(4, containerId)
                     stmt.execute()
                 }
 
                 return true
             }
+        }
+    }
 
     private companion object {
         private const val BATCH_SIZE = 1024

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 {
+    public fun uncompressUnlessEncrypted(input: ByteBuf): ByteBuf? {
-        return !isKeyValid(input, XteaKey.ZERO)
+        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)
 
+            /**
+             * 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 {
-                OutputStream.nullOutputStream().use { output ->
                     type.createInputStream(ByteBufInputStream(plaintext, len), uncompressedLen).use { inputStream ->
-                        if (inputStream.transferTo(output) != uncompressedLen.toLong()) {
+                        var remaining = uncompressedLen
-                            return false
+                        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 false
+                    return null
-            }
                 }
 
-        return true
+                return output.retain()
+            }
+        }
     }
 
     private fun decrypt(buf: ByteBuf, len: Int, key: XteaKey): ByteBuf {

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<IOException> {
-                Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)
+                Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO)?.release()
             }
         }
     }
@@ -279,7 +278,7 @@ class Js5CompressionTest {
             }
 
             assertFailsWith<IOException> {
-                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() {
+        copiedBuffer("OpenRS2").use { expected ->
             read("none.dat").use { compressed ->
-            assertFalse(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertEquals(expected, actual)
-            assertFalse(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)
+                }
+            }
         }
     }
 
     @Test
     fun testBzip2KeyValid() {
+        copiedBuffer("OpenRS2").use { expected ->
             read("bzip2.dat").use { compressed ->
-            assertFalse(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertEquals(expected, actual)
-            assertFalse(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 ->
-            assertTrue(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertNull(actual)
-            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("bzip2-invalid-magic.dat").use { compressed ->
-            assertFalse(Js5Compression.isKeyValid(compressed, XteaKey.ZERO))
+            Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO).use { actual ->
+                assertNull(actual)
+            }
         }
     }
 
     @Test
     fun testGzipKeyValid() {
+        copiedBuffer("OpenRS2").use { expected ->
             read("gzip.dat").use { compressed ->
-            assertFalse(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertEquals(expected, actual)
-            assertFalse(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 ->
-            assertTrue(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertNull(actual)
-            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("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() {
+        copiedBuffer("OpenRS2").use { expected ->
             read("lzma.dat").use { compressed ->
-            assertFalse(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertTrue(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertEquals(expected, actual)
-            assertFalse(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("lzma-encrypted.dat").use { compressed ->
-            assertTrue(Js5Compression.isEncrypted(compressed.slice()))
+                Js5Compression.uncompressUnlessEncrypted(compressed.slice()).use { actual ->
-            assertFalse(Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO))
+                    assertNull(actual)
-            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<IOException> {
-                Js5Compression.isKeyValid(compressed, XteaKey.ZERO)
+                Js5Compression.uncompressIfKeyValid(compressed, XteaKey.ZERO)?.release()
             }
         }
     }
@@ -472,7 +582,7 @@ class Js5CompressionTest {
             }
 
             assertFailsWith<IOException> {
-                Js5Compression.isKeyValid(compressed.slice(), XteaKey.ZERO)
+                Js5Compression.uncompressIfKeyValid(compressed.slice(), XteaKey.ZERO)?.release()
             }
         }
     }