Hello

I am making a secure chat messenger. I currently am working on a new and already working feature to send and receive images.

My messenger is focused on anonymity and security, these two properties have more importance than UX.

# Avoid search engine indexing.
View git repo (base64):
aHR0cHM6Ly9naXRodWIuY29tL05pZWxEdXlzdGVycy9hcnRpLWNoYXQ=

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?

1 post - 1 participant

Read full topic