Allowing sending and receiving files is risky because it introduces new attack vectors:
DoS attacks
OOM attacks
de-anonymization by EXIF data
Exploiting image decoder/encoder library.
I try to mitigate these risks in my current implementation:
Ability to send and receive images can be toggled in settings.
For each outgoing and incoming image:
Assert max dimension of image.
Assert max file size.
Reencode input to strip EXIF data.
Outgoing image (PNG and JPEG allowed) are converted to JPEG before sending.
Incoming message can only be JPEG.
I would like some feedback on the current implementation:
//! Logic to handle encoding and decoding of attachments.
use image::{DynamicImage, GenericImageView, codecs::jpeg::JpegEncoder};
use crate::error::AttachmentError;
/// Reencode outgoing image to bytes.
/// This strips metadata.
pub fn reencode_image_to_bytes<P: AsRef<std::path::Path>>(
input: P,
) -> Result<Vec<u8>, AttachmentError> {
let path = input.as_ref();
// Check file size.
let metadata = std::fs::metadata(path)?;
if metadata.len() > 500 * 1024 {
return Err(AttachmentError::FileSizeExceedsLimit);
}
// Decode.
let image: DynamicImage = image::open(path)?;
reencode_image(image)
}
/// Reencode bytes of incoming image.
pub fn reencode_bytes(
input: Vec<u8>,
) -> Result<Vec<u8>, AttachmentError> {
// Check file size.
if input.len() > 500 * 1024 {
return Err(AttachmentError::FileSizeExceedsLimit);
}
// Incoming bytes should always be JPEG.
if input.len() < 2 || input[0] != 0xFF || input[1] != 0xD8 {
return Err(AttachmentError::FileUnsupportedFormat);
}
// Decode.
let image: DynamicImage = image::load_from_memory(&input)?;
reencode_image(image)
}
/// Shared reencode implementation.
fn reencode_image(
image: DynamicImage,
) -> Result<Vec<u8>, AttachmentError> {
// Check max width and height.
let (x, y) = image.dimensions();
if x > 1025 || y > 1025 {
return Err(AttachmentError::ImageDimensionsExceedsLimit);
}
// Copy to clean buffer.
let rgba = image.to_rgba8();
let buffer = DynamicImage::ImageRgba8(rgba);
// Convert to JPEG and return bytes.
let mut output = Vec::new();
// Low quality to reduce chance to image fingerprinting.
let mut encoder = JpegEncoder::new_with_quality(&mut output, 50);
encoder.encode_image(&buffer)?;
Ok(output)
}
Is this implementation okay? Can it be even more secure?
Wasm sandbox?
Currently I do not protect against the risk that an adversary could craft an image with bytes which could exploit a vulnerability in the image library to do e.g RCE.
The entry point for those attacks would be let image: DynamicImage = image::open(path)?; and let image: DynamicImage = image::load_from_memory(&input)?;.
I was thinking about using a simple wasm_sandbox to do the decoding. So that if the library to decode the images contains a vulnerability, an attack would be limited to the sandbox without affecting the host. Is that a good idea?
The snippet seems vulnerable to a TOCTOU attack. The image could be altered by other processes directly after the metadata check so there is still the possibility of using an invalid image in the code following. I think the solution would be to accquire an exclusive file lock for usage by both operations ( File in std::fs - Rust ).
Regarding your question:
I’m not a security expert, but is there a bug in the image crate that or is such a library especially vulnerable for RCE exploits during decoding?
Thanks for your reply, your note regarding a TOCTOU attack is interesting. I'll check that. I'm not sure if it's applicable to incoming messages since in that case we directly use the received raw bytes and no (temporary) file on the file system is used.
I’m not a security expert, but is there a bug in the image crate that or is such a library especially vulnerable for RCE exploits during decoding?
AFAIK there currently is no such vulnerability in Rust's image crate, But I want to sandbox it to protect against 0-days. But it is known that image libraries contain vulnerabilities like this which can be exploited. Whatsapp had something similar a while ago. 0-Click WhatsApp Vulnerability Exploited via Malicious DNG Image
The TOCTOU exploit does not refer to the wasm sandboxing, I just wanted to mention ist. In the code snippet, it very certainly is present. I am still not a security expert, but I think rust does mitigate lots of RCE possibilities caused by UB (Buffer overrun, general raw pointer stuff) by its safety and panicking mechanism, but I guess you already know that…
try_lock is not guaranteed to be mandatory, and in fact it's an advisory lock on some platforms (e.g. Linux). This means that it will prevent others from acquiring another lock, but may not prevent them from reading and writing to the file, which is what you wanted to avoid here anyway.
I think you'll have to read the file into memory, putting an upper bound on the amount of bytes allowed, then check if there are more to read. In the worst case you think you read all the bytes but you didn't and the image will fail decoding, but you would have raised an error anyway due to exceeding the maximum size allowed.
@HQ2000 I updated my code to avoid TOCTOU. I now read the bytes of the file into a buffer, assert the limits, and then create the image by loading it from those bytes. This way I only open the file once, and don't check the size decoupled from the opened file.
//! Logic to handle encoding and decoding of attachments.
use image::{DynamicImage, GenericImageView, codecs::jpeg::JpegEncoder};
use std::io::{Read, BufReader};
use crate::error::AttachmentError;
/// Max file size is 500 KiB.
const MAX_FILE_SIZE: usize = 500 * 1024;
/// Reencode outgoing image to bytes.
/// This strips metadata.
pub fn reencode_image_to_bytes<P: AsRef<std::path::Path>>(
input: P,
) -> Result<Vec<u8>, AttachmentError> {
let path = input.as_ref();
// Open once, and load image from memory to avoid TOCTOU.
let file = std::fs::File::open(path)?;
let mut reader = BufReader::new(file);
// Read file data into bytes.
let mut bytes = Vec::with_capacity(MAX_FILE_SIZE.min(64 * 1024));
reader
.by_ref()
.take((MAX_FILE_SIZE + 1) as u64) // MAX_FILE_SIZE + 1 to detect exceeding file size limit.
.read_to_end(&mut bytes)?;
// Check max file size.
if bytes.len() > MAX_FILE_SIZE {
return Err(AttachmentError::FileSizeExceedsLimit);
}
// Decode.
let image: DynamicImage = image::load_from_memory(&bytes)?;
reencode_image(image)
}
/// Reencode bytes of incoming message.
pub fn reencode_bytes(
input: Vec<u8>,
) -> Result<Vec<u8>, AttachmentError> {
// Check file size.
if input.len() > 500 * 1024 {
return Err(AttachmentError::FileSizeExceedsLimit);
}
// Incoming bytes should always be JPEG.
if input.len() < 2 || input[0] != 0xFF || input[1] != 0xD8 {
return Err(AttachmentError::FileUnsupportedFormat);
}
// Decode.
let image: DynamicImage = image::load_from_memory(&input)?;
reencode_image(image)
}
/// Shared reencode implementation.
fn reencode_image(
image: DynamicImage,
) -> Result<Vec<u8>, AttachmentError> {
// Check max width and height.
let (x, y) = image.dimensions();
if x > 1025 || y > 1025 {
return Err(AttachmentError::ImageDimensionsExceedsLimit);
}
// Copy to clean buffer.
let rgba = image.to_rgba8();
let buffer = DynamicImage::ImageRgba8(rgba);
// Convert to JPEG and return bytes.
let mut output = Vec::new();
// Low quality to reduce chance to image fingerprinting.
let mut encoder = JpegEncoder::new_with_quality(&mut output, 50);
encoder.encode_image(&buffer)?;
Ok(output)
}
Regarding this. I think I won't implement a wasm_sandbox. The crate image is written in native Rust which indeed should protect against buffer overflows etc mitigating RCE. I think adding a wasm_sandbox would not add any extra benefit.
Note that you don't need to reencode to strip the metadata blocks, in most formats.
You might want to also have a whitelist of allowed metadata blocks, since for example if you strip gAMA in PNG (or reencode it away) you're going to either have it display wrongly (or make it ugly from quantization issues).
Also, why not allow sending PNGs too? Reencoding them to JPEG would often be pretty annoying, since I probably used PNG for a reason.