From 5c25ba8000538cdb4e23b2a4cf5d1a7423776bc1 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 18 Sep 2026 10:35:38 -0400 Subject: [PATCH] Improve webp sanitization pipeline. --- .../glide/SignalGlideComponents.java | 11 +- .../glide/cache/WebpSanByteBufferDecoder.kt | 31 +++++ .../securesms/glide/cache/WebpSanDecoder.kt | 21 +--- .../glide/cache/WebpSanResourceDecoder.kt | 27 +++++ .../cache/WebpSanStreamFactoryDecoder.kt | 28 +---- .../load/engine/WebpSanDecodePathTest.kt | 110 ++++++++++++++++++ .../cache/WebpSanByteBufferDecoderTest.kt | 69 +++++++++++ .../glide/cache/WebpSanDecoderTest.kt | 68 +++++++++++ .../cache/WebpSanStreamFactoryDecoderTest.kt | 11 +- 9 files changed, 330 insertions(+), 46 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoder.kt create mode 100644 app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanResourceDecoder.kt create mode 100644 app/src/test/java/com/bumptech/glide/load/engine/WebpSanDecodePathTest.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoderTest.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoderTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/glide/SignalGlideComponents.java b/app/src/main/java/org/thoughtcrime/securesms/glide/SignalGlideComponents.java index d451a9786b..136271c9a0 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/glide/SignalGlideComponents.java +++ b/app/src/main/java/org/thoughtcrime/securesms/glide/SignalGlideComponents.java @@ -44,6 +44,7 @@ import org.thoughtcrime.securesms.glide.cache.EncryptedGifDrawableResourceEncode import org.thoughtcrime.securesms.glide.cache.InputStreamFactoryBitmapDecoder; import org.thoughtcrime.securesms.glide.cache.StreamBitmapDecoder; import org.thoughtcrime.securesms.glide.cache.StreamFactoryGifDecoder; +import org.thoughtcrime.securesms.glide.cache.WebpSanByteBufferDecoder; import org.thoughtcrime.securesms.glide.cache.WebpSanDecoder; import org.thoughtcrime.securesms.glide.cache.WebpSanStreamFactoryDecoder; import org.thoughtcrime.securesms.mms.RegisterGlideComponents; @@ -55,6 +56,7 @@ import org.thoughtcrime.securesms.util.ConversationShortcutPhoto; import java.io.File; import java.io.InputStream; +import java.nio.ByteBuffer; /** * The core logic for {@link SignalGlideModule}. This is a separate class because it uses @@ -69,10 +71,15 @@ public class SignalGlideComponents implements RegisterGlideComponents { registry.prepend(File.class, File.class, UnitModelLoader.Factory.getInstance()); - WebpSanStreamFactoryDecoder webpSanStreamFactoryDecoder = new WebpSanStreamFactoryDecoder(); - registry.prepend(InputStream.class, Bitmap.class, new WebpSanDecoder()); + WebpSanStreamFactoryDecoder webpSanStreamFactoryDecoder = new WebpSanStreamFactoryDecoder<>(); + registry.prepend(InputStream.class, Bitmap.class, new WebpSanDecoder<>()); registry.prepend(InputStreamFactory.class, Bitmap.class, webpSanStreamFactoryDecoder); + // Need to use these bucket labels to not break GIF loading + registry.prepend(Registry.BUCKET_BITMAP_DRAWABLE, InputStream.class, BitmapDrawable.class, new WebpSanDecoder<>()); + registry.prepend(Registry.BUCKET_BITMAP, ByteBuffer.class, Bitmap.class, new WebpSanByteBufferDecoder<>()); + registry.prepend(Registry.BUCKET_BITMAP_DRAWABLE, ByteBuffer.class, BitmapDrawable.class, new WebpSanByteBufferDecoder<>()); + registry.prepend(InputStream.class, new EncryptedCacheEncoder(secret, glide.getArrayPool())); registry.prepend(File.class, Bitmap.class, new EncryptedCacheDecoder<>(secret, new StreamBitmapDecoder(context, glide, registry))); diff --git a/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoder.kt b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoder.kt new file mode 100644 index 0000000000..5e4f7e1ac0 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoder.kt @@ -0,0 +1,31 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.glide.cache + +import com.bumptech.glide.load.Options +import com.bumptech.glide.util.ByteBufferUtil +import org.signal.core.util.logging.Log +import java.io.InputStream +import java.nio.ByteBuffer + +/** + * See [WebpSanResourceDecoder] + */ +class WebpSanByteBufferDecoder : WebpSanResourceDecoder() { + + override fun handles(source: ByteBuffer, options: Options): Boolean { + return try { + val isWebp = source.stream().use { WebpSanitizerCheck.isWebp(it) } + + isWebp && source.stream().use { !WebpSanitizerCheck.isSanitized(it) } + } catch (e: Exception) { + Log.w(tag, "Failed to check buffer, blocking load.", e) + true + } + } + + private fun ByteBuffer.stream(): InputStream = ByteBufferUtil.toStream(duplicate()) +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoder.kt b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoder.kt index 7dd104e860..78304dad93 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoder.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoder.kt @@ -5,24 +5,14 @@ package org.thoughtcrime.securesms.glide.cache -import android.graphics.Bitmap import com.bumptech.glide.load.Options -import com.bumptech.glide.load.ResourceDecoder -import com.bumptech.glide.load.engine.Resource import org.signal.core.util.logging.Log -import java.io.IOException import java.io.InputStream /** - * Uses WebpSanitizer to check for invalid webp. - * - * See [WebpSanStreamFactoryDecoder] for the equivalent that operates on the [org.signal.glide.common.io.InputStreamFactory] model chain. + * See [WebpSanResourceDecoder] */ -class WebpSanDecoder : ResourceDecoder { - - companion object { - private val TAG = Log.tag(WebpSanDecoder::class.java) - } +class WebpSanDecoder : WebpSanResourceDecoder() { /** * If the source is a webp, we sanitize it and block the load if the check fails. @@ -40,13 +30,8 @@ class WebpSanDecoder : ResourceDecoder { !sanitized } catch (e: Exception) { - Log.w(TAG, "Failed to check stream, blocking load.", e) + Log.w(tag, "Failed to check stream, blocking load.", e) true } } - - override fun decode(source: InputStream, width: Int, height: Int, options: Options): Resource? { - Log.w(TAG, "Image did not pass sanitizer") - throw IOException("Unable to load image") - } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanResourceDecoder.kt b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanResourceDecoder.kt new file mode 100644 index 0000000000..b3551aa9f1 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanResourceDecoder.kt @@ -0,0 +1,27 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.glide.cache + +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.ResourceDecoder +import com.bumptech.glide.load.engine.GlideException +import com.bumptech.glide.load.engine.Resource +import org.signal.core.util.logging.Log + +/** + * Base class for webp sanitization. It's a funny decoder with an inverted contract: + * - if it's a bad webp, return true from [handles] + * - if we handle it, this decoder just insta-fails to prevent anyone from displaying it. + */ +abstract class WebpSanResourceDecoder : ResourceDecoder { + + protected val tag: String = Log.tag(this::class) + + final override fun decode(source: Data, width: Int, height: Int, options: Options): Resource? { + Log.w(tag, "Image did not pass sanitizer") + throw GlideException("Unable to load image") + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoder.kt b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoder.kt index 7cf6fa75ca..ed83de6946 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoder.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoder.kt @@ -5,45 +5,23 @@ package org.thoughtcrime.securesms.glide.cache -import android.graphics.Bitmap import com.bumptech.glide.load.Options -import com.bumptech.glide.load.ResourceDecoder -import com.bumptech.glide.load.engine.GlideException -import com.bumptech.glide.load.engine.Resource import org.signal.core.util.logging.Log import org.signal.glide.common.io.InputStreamFactory /** - * The [InputStreamFactory] equivalent of [WebpSanDecoder]. Because we can create as many streams as we like, we don't have to deal with marking and resetting - * the caller's stream. + * See [WebpSanResourceDecoder] */ -class WebpSanStreamFactoryDecoder : ResourceDecoder { +class WebpSanStreamFactoryDecoder : WebpSanResourceDecoder() { - companion object { - private val TAG = Log.tag(WebpSanStreamFactoryDecoder::class) - } - - /** - * If the source is a webp, we sanitize it and block the load if the check fails. - */ override fun handles(source: InputStreamFactory, options: Options): Boolean { return try { val isWebp = source.create().buffered().use { WebpSanitizerCheck.isWebp(it) } isWebp && source.create().buffered().use { !WebpSanitizerCheck.isSanitized(it) } } catch (e: Exception) { - Log.w(TAG, "Failed to check stream, blocking load.", e) + Log.w(tag, "Failed to check stream, blocking load.", e) true } } - - /** - * Note that we throw a [GlideException] rather than an [java.io.IOException] on purpose. Glide swallows IOExceptions from a decoder and simply moves on to - * the next decoder registered for the same data/resource pair, which would let the unsanitized image load anyway. A GlideException aborts the entire decode - * path. - */ - override fun decode(source: InputStreamFactory, width: Int, height: Int, options: Options): Resource? { - Log.w(TAG, "Image did not pass sanitizer") - throw GlideException("Unable to load image") - } } diff --git a/app/src/test/java/com/bumptech/glide/load/engine/WebpSanDecodePathTest.kt b/app/src/test/java/com/bumptech/glide/load/engine/WebpSanDecodePathTest.kt new file mode 100644 index 0000000000..cfd907c5ce --- /dev/null +++ b/app/src/test/java/com/bumptech/glide/load/engine/WebpSanDecodePathTest.kt @@ -0,0 +1,110 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package com.bumptech.glide.load.engine + +import android.app.Application +import assertk.assertThat +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.ResourceDecoder +import com.bumptech.glide.load.data.DataRewinder +import com.bumptech.glide.load.resource.SimpleResource +import com.bumptech.glide.util.pool.FactoryPools +import org.junit.Assert.assertThrows +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.thoughtcrime.securesms.glide.cache.WebpSanDecoder +import java.io.ByteArrayInputStream +import java.io.IOException +import java.io.InputStream + +/** + * Pins the Glide behavior that [WebpSanDecoder] relies on: rejecting an image has to stop the whole decode path, not just skip to the next decoder registered + * for the same data/resource pair. Lives in Glide's package because [DecodePath.DecodeCallback] is package-private. + */ +@RunWith(RobolectricTestRunner::class) +@Config(manifest = Config.NONE, application = Application::class) +class WebpSanDecodePathTest { + + companion object { + /** An 8x8 lossless webp. */ + private val VALID_WEBP = byteArrayOf( + 82, 73, 70, 70, 28, 0, 0, 0, 87, 69, 66, 80, 86, 80, 56, 76, 15, 0, + 0, 0, 47, 7, -64, 1, 0, 7, 16, -3, -113, -2, 7, 34, -94, -1, 1, 0 + ) + + private val MALFORMED_WEBP = "RIFF".toByteArray() + byteArrayOf(0x40, 0x00, 0x00, 0x00) + "WEBP".toByteArray() + ByteArray(56) + } + + private val sentinel = SentinelDecoder() + + @Test + fun `a rejected webp never reaches a later decoder`() { + val path = decodePath(WebpSanDecoder(), sentinel) + + assertThrows(GlideException::class.java) { + path.decode(MALFORMED_WEBP.asRewinder(), 100, 100, Options()) { it } + } + + assertThat(sentinel.decoded).isFalse() + } + + @Test + fun `an accepted webp still reaches a later decoder`() { + val path = decodePath(WebpSanDecoder(), sentinel) + + path.decode(VALID_WEBP.asRewinder(), 100, 100, Options()) { it } + + assertThat(sentinel.decoded).isTrue() + } + + @Test + fun `an IOException from the gate would let a later decoder run`() { + val path = decodePath(IoExceptionDecoder(), sentinel) + + path.decode(MALFORMED_WEBP.asRewinder(), 100, 100, Options()) { it } + + assertThat(sentinel.decoded).isTrue() + } + + private fun decodePath(vararg decoders: ResourceDecoder): DecodePath { + return DecodePath( + InputStream::class.java, + String::class.java, + String::class.java, + decoders.toList(), + { toTranscode, _ -> toTranscode }, + FactoryPools.threadSafeList() + ) + } + + private fun ByteArray.asRewinder(): DataRewinder { + return object : DataRewinder { + override fun rewindAndGet(): InputStream = ByteArrayInputStream(this@asRewinder) + override fun cleanup() = Unit + } + } + + private class SentinelDecoder : ResourceDecoder { + var decoded = false + + override fun handles(source: InputStream, options: Options): Boolean = true + + override fun decode(source: InputStream, width: Int, height: Int, options: Options): Resource { + decoded = true + return SimpleResource("decoded") + } + } + + private class IoExceptionDecoder : ResourceDecoder { + override fun handles(source: InputStream, options: Options): Boolean = true + + override fun decode(source: InputStream, width: Int, height: Int, options: Options): Resource = throw IOException("rejected") + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoderTest.kt b/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoderTest.kt new file mode 100644 index 0000000000..d057fe86e0 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanByteBufferDecoderTest.kt @@ -0,0 +1,69 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.glide.cache + +import assertk.assertThat +import assertk.assertions.isEqualTo +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.engine.GlideException +import org.junit.Assert.assertThrows +import org.junit.Test +import java.nio.ByteBuffer + +class WebpSanByteBufferDecoderTest { + + companion object { + /** An 8x8 lossless webp. */ + private val VALID_WEBP = byteArrayOf( + 82, 73, 70, 70, 28, 0, 0, 0, 87, 69, 66, 80, 86, 80, 56, 76, 15, 0, + 0, 0, 47, 7, -64, 1, 0, 7, 16, -3, -113, -2, 7, 34, -94, -1, 1, 0 + ) + } + + private val decoder = WebpSanByteBufferDecoder() + + @Test + fun `handles - non-webp data is not blocked`() { + val png = byteArrayOf(0x89.toByte(), 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A) + ByteArray(64) + + assertThat(decoder.handles(ByteBuffer.wrap(png), Options())).isFalse() + } + + @Test + fun `handles - data too short to have a magic number is blocked`() { + assertThat(decoder.handles(ByteBuffer.wrap(byteArrayOf(0x52, 0x49, 0x46, 0x46)), Options())).isTrue() + } + + @Test + fun `handles - valid webp is not blocked`() { + assertThat(decoder.handles(ByteBuffer.wrap(VALID_WEBP), Options())).isFalse() + } + + @Test + fun `handles - malformed webp is blocked`() { + val malformed = "RIFF".toByteArray() + byteArrayOf(0x40, 0x00, 0x00, 0x00) + "WEBP".toByteArray() + ByteArray(56) + + assertThat(decoder.handles(ByteBuffer.wrap(malformed), Options())).isTrue() + } + + @Test + fun `handles - leaves the caller's buffer position untouched`() { + val buffer = ByteBuffer.wrap(VALID_WEBP) + + decoder.handles(buffer, Options()) + + assertThat(buffer.position()).isEqualTo(0) + } + + @Test + fun `decode - throws GlideException so that Glide aborts the decode path`() { + assertThrows(GlideException::class.java) { + decoder.decode(ByteBuffer.wrap(VALID_WEBP), 100, 100, Options()) + } + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoderTest.kt b/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoderTest.kt new file mode 100644 index 0000000000..7c430794e6 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanDecoderTest.kt @@ -0,0 +1,68 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.thoughtcrime.securesms.glide.cache + +import assertk.assertThat +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import com.bumptech.glide.load.Options +import com.bumptech.glide.load.engine.GlideException +import org.junit.Assert.assertThrows +import org.junit.Test +import java.io.ByteArrayInputStream + +class WebpSanDecoderTest { + + companion object { + /** An 8x8 lossless webp. */ + private val VALID_WEBP = byteArrayOf( + 82, 73, 70, 70, 28, 0, 0, 0, 87, 69, 66, 80, 86, 80, 56, 76, 15, 0, + 0, 0, 47, 7, -64, 1, 0, 7, 16, -3, -113, -2, 7, 34, -94, -1, 1, 0 + ) + } + + private val decoder = WebpSanDecoder() + + @Test + fun `handles - non-webp data is not blocked`() { + val png = byteArrayOf(0x89.toByte(), 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A) + ByteArray(64) + + assertThat(decoder.handles(ByteArrayInputStream(png), Options())).isFalse() + } + + @Test + fun `handles - data too short to have a magic number is blocked`() { + assertThat(decoder.handles(ByteArrayInputStream(byteArrayOf(0x52, 0x49, 0x46, 0x46)), Options())).isTrue() + } + + @Test + fun `handles - valid webp is not blocked`() { + assertThat(decoder.handles(ByteArrayInputStream(VALID_WEBP), Options())).isFalse() + } + + @Test + fun `handles - malformed webp is blocked`() { + val malformed = "RIFF".toByteArray() + byteArrayOf(0x40, 0x00, 0x00, 0x00) + "WEBP".toByteArray() + ByteArray(56) + + assertThat(decoder.handles(ByteArrayInputStream(malformed), Options())).isTrue() + } + + @Test + fun `handles - unreadable stream is blocked`() { + val stream = object : ByteArrayInputStream(VALID_WEBP) { + override fun read(b: ByteArray, off: Int, len: Int): Int = throw IllegalStateException("nope") + } + + assertThat(decoder.handles(stream, Options())).isTrue() + } + + @Test + fun `decode - throws GlideException so that Glide aborts the decode path`() { + assertThrows(GlideException::class.java) { + decoder.decode(ByteArrayInputStream(VALID_WEBP), 100, 100, Options()) + } + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoderTest.kt b/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoderTest.kt index 29cf079a83..94a90db1d4 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoderTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/glide/cache/WebpSanStreamFactoryDecoderTest.kt @@ -9,6 +9,8 @@ import assertk.assertThat import assertk.assertions.isFalse import assertk.assertions.isTrue import com.bumptech.glide.load.Options +import com.bumptech.glide.load.engine.GlideException +import org.junit.Assert.assertThrows import org.junit.Test import org.signal.glide.common.io.InputStreamFactory import java.io.ByteArrayInputStream @@ -24,7 +26,7 @@ class WebpSanStreamFactoryDecoderTest { ) } - private val decoder = WebpSanStreamFactoryDecoder() + private val decoder = WebpSanStreamFactoryDecoder() @Test fun `handles - non-webp data is not blocked`() { @@ -59,6 +61,13 @@ class WebpSanStreamFactoryDecoderTest { assertThat(decoder.handles(factory, Options())).isTrue() } + @Test + fun `decode - throws GlideException so that Glide aborts the decode path`() { + assertThrows(GlideException::class.java) { + decoder.decode(VALID_WEBP.asStreamFactory(), 100, 100, Options()) + } + } + private fun ByteArray.asStreamFactory(): InputStreamFactory { return object : InputStreamFactory { override fun create(): InputStream = ByteArrayInputStream(this@asStreamFactory)