mirror of
https://github.com/signalapp/Signal-Android.git
synced 2026-09-28 07:51:42 +01:00
Improve webp sanitization pipeline.
This commit is contained in:
committed by
Michelle Tang
parent
1e38286260
commit
5c25ba8000
@@ -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<Bitmap> 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)));
|
||||
|
||||
+31
@@ -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<DecodeType : Any> : WebpSanResourceDecoder<ByteBuffer, DecodeType>() {
|
||||
|
||||
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())
|
||||
}
|
||||
+3
-18
@@ -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<InputStream, Bitmap> {
|
||||
|
||||
companion object {
|
||||
private val TAG = Log.tag(WebpSanDecoder::class.java)
|
||||
}
|
||||
class WebpSanDecoder<DecodeType : Any> : WebpSanResourceDecoder<InputStream, DecodeType>() {
|
||||
|
||||
/**
|
||||
* If the source is a webp, we sanitize it and block the load if the check fails.
|
||||
@@ -40,13 +30,8 @@ class WebpSanDecoder : ResourceDecoder<InputStream, Bitmap> {
|
||||
|
||||
!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<Bitmap>? {
|
||||
Log.w(TAG, "Image did not pass sanitizer")
|
||||
throw IOException("Unable to load image")
|
||||
}
|
||||
}
|
||||
|
||||
+27
@@ -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<Data : Any, DecodeType : Any> : ResourceDecoder<Data, DecodeType> {
|
||||
|
||||
protected val tag: String = Log.tag(this::class)
|
||||
|
||||
final override fun decode(source: Data, width: Int, height: Int, options: Options): Resource<DecodeType>? {
|
||||
Log.w(tag, "Image did not pass sanitizer")
|
||||
throw GlideException("Unable to load image")
|
||||
}
|
||||
}
|
||||
+3
-25
@@ -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<InputStreamFactory, Bitmap> {
|
||||
class WebpSanStreamFactoryDecoder<DecodeType : Any> : WebpSanResourceDecoder<InputStreamFactory, DecodeType>() {
|
||||
|
||||
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<Bitmap>? {
|
||||
Log.w(TAG, "Image did not pass sanitizer")
|
||||
throw GlideException("Unable to load image")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<InputStream, String>): DecodePath<InputStream, String, String> {
|
||||
return DecodePath(
|
||||
InputStream::class.java,
|
||||
String::class.java,
|
||||
String::class.java,
|
||||
decoders.toList(),
|
||||
{ toTranscode, _ -> toTranscode },
|
||||
FactoryPools.threadSafeList()
|
||||
)
|
||||
}
|
||||
|
||||
private fun ByteArray.asRewinder(): DataRewinder<InputStream> {
|
||||
return object : DataRewinder<InputStream> {
|
||||
override fun rewindAndGet(): InputStream = ByteArrayInputStream(this@asRewinder)
|
||||
override fun cleanup() = Unit
|
||||
}
|
||||
}
|
||||
|
||||
private class SentinelDecoder : ResourceDecoder<InputStream, String> {
|
||||
var decoded = false
|
||||
|
||||
override fun handles(source: InputStream, options: Options): Boolean = true
|
||||
|
||||
override fun decode(source: InputStream, width: Int, height: Int, options: Options): Resource<String> {
|
||||
decoded = true
|
||||
return SimpleResource("decoded")
|
||||
}
|
||||
}
|
||||
|
||||
private class IoExceptionDecoder : ResourceDecoder<InputStream, String> {
|
||||
override fun handles(source: InputStream, options: Options): Boolean = true
|
||||
|
||||
override fun decode(source: InputStream, width: Int, height: Int, options: Options): Resource<String> = throw IOException("rejected")
|
||||
}
|
||||
}
|
||||
+69
@@ -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<Any>()
|
||||
|
||||
@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())
|
||||
}
|
||||
}
|
||||
}
|
||||
+68
@@ -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<Any>()
|
||||
|
||||
@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())
|
||||
}
|
||||
}
|
||||
}
|
||||
Vendored
+10
-1
@@ -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<Any>()
|
||||
|
||||
@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)
|
||||
|
||||
Reference in New Issue
Block a user