Do not read whole webxdc file into memory

This seems not only wasteful but genuinly has the risk someone makes
their device useless by accidentally adding a huge file.

This also re-structures the checks a little: The if-conditions are
flattened out and cheap checks are done before more expensive ones.
This commit is contained in:
Floris Bruynooghe
2022-03-06 19:36:02 +01:00
parent 33ba8dabe0
commit b2fe723570
2 changed files with 55 additions and 39 deletions

View File

@@ -2,6 +2,7 @@
use std::collections::{HashMap, HashSet}; use std::collections::{HashMap, HashSet};
use std::future::Future; use std::future::Future;
use std::io::Cursor;
use std::pin::Pin; use std::pin::Pin;
use anyhow::{bail, Result}; use anyhow::{bail, Result};
@@ -1020,8 +1021,9 @@ impl MimeMessage {
if decoded_data.is_empty() { if decoded_data.is_empty() {
return; return;
} }
let reader = Cursor::new(decoded_data);
let msg_type = if context let msg_type = if context
.is_webxdc_file(filename, decoded_data) .is_webxdc_file(filename, reader)
.await .await
.unwrap_or(false) .unwrap_or(false)
{ {

View File

@@ -10,13 +10,13 @@ use crate::{chat, EventType};
use anyhow::{bail, ensure, format_err, Result}; use anyhow::{bail, ensure, format_err, Result};
use async_std::path::PathBuf; use async_std::path::PathBuf;
use deltachat_derive::FromSql; use deltachat_derive::FromSql;
use lettre_email::mime::{self}; use lettre_email::mime;
use lettre_email::PartBuilder; use lettre_email::PartBuilder;
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use serde_json::Value; use serde_json::Value;
use std::convert::TryFrom; use std::convert::TryFrom;
use std::fs::File; use std::fs::File;
use std::io::Read; use std::io::{Read, Seek, SeekFrom};
use zip::ZipArchive; use zip::ZipArchive;
pub const WEBXDC_SUFFIX: &str = "xdc"; pub const WEBXDC_SUFFIX: &str = "xdc";
@@ -32,12 +32,12 @@ const WEBXDC_DEFAULT_ICON: &str = "__webxdc__/default-icon.png";
/// ///
/// The limit is also an experiment to see how small we can go; /// The limit is also an experiment to see how small we can go;
/// it is planned to raise that limit as needed in subsequent versions. /// it is planned to raise that limit as needed in subsequent versions.
const WEBXDC_SENDING_LIMIT: usize = 655360; const WEBXDC_SENDING_LIMIT: u64 = 655360;
/// Be more tolerant for .xdc sizes on receiving - /// Be more tolerant for .xdc sizes on receiving -
/// might be, the senders version uses already a larger limit /// might be, the senders version uses already a larger limit
/// and not showing the .xdc on some devices would be even worse ux. /// and not showing the .xdc on some devices would be even worse ux.
const WEBXDC_RECEIVING_LIMIT: usize = 4194304; const WEBXDC_RECEIVING_LIMIT: u64 = 4194304;
/// Raw information read from manifest.toml /// Raw information read from manifest.toml
#[derive(Debug, Deserialize)] #[derive(Debug, Deserialize)]
@@ -123,46 +123,56 @@ pub(crate) struct StatusUpdateItemAndSerial {
impl Context { impl Context {
/// check if a file is an acceptable webxdc for sending or receiving. /// check if a file is an acceptable webxdc for sending or receiving.
pub(crate) async fn is_webxdc_file(&self, filename: &str, buf: &[u8]) -> Result<bool> { pub(crate) async fn is_webxdc_file<R>(&self, filename: &str, mut reader: R) -> Result<bool>
if filename.ends_with(WEBXDC_SUFFIX) { where
let reader = std::io::Cursor::new(buf); R: Read + Seek,
if let Ok(mut archive) = zip::ZipArchive::new(reader) { {
if let Ok(_index_html) = archive.by_name("index.html") { if !filename.ends_with(WEBXDC_SUFFIX) {
if buf.len() <= WEBXDC_RECEIVING_LIMIT { return Ok(false);
return Ok(true); }
} else {
let size = reader.seek(SeekFrom::End(0))?;
if size > WEBXDC_RECEIVING_LIMIT {
info!( info!(
self, self,
"{} exceeds receiving limit of {} bytes", "{} exceeds receiving limit of {} bytes", &filename, WEBXDC_RECEIVING_LIMIT
&filename,
WEBXDC_RECEIVING_LIMIT
); );
return Ok(false);
} }
} else {
info!(self, "{} misses index.html", &filename); reader.seek(SeekFrom::Start(0))?;
} let mut archive = match zip::ZipArchive::new(reader) {
} else { Ok(archive) => archive,
Err(_) => {
info!(self, "{} cannot be opened as zip-file", &filename); info!(self, "{} cannot be opened as zip-file", &filename);
return Ok(false);
} }
};
if archive.by_name("index.html").is_err() {
info!(self, "{} misses index.html", &filename);
return Ok(false);
} }
Ok(false)
Ok(true)
} }
/// ensure that a file is an acceptable webxdc for sending /// ensure that a file is an acceptable webxdc for sending
/// (sending has more strict size limits). /// (sending has more strict size limits).
pub(crate) async fn ensure_sendable_webxdc_file(&self, path: &PathBuf) -> Result<()> { pub(crate) async fn ensure_sendable_webxdc_file(&self, path: &PathBuf) -> Result<()> {
let mut file = std::fs::File::open(path)?; let mut file = std::fs::File::open(path)?;
let mut buf = Vec::new();
file.read_to_end(&mut buf)?;
if !self if !self
.is_webxdc_file(path.to_str().unwrap_or_default(), &buf) .is_webxdc_file(path.to_str().unwrap_or_default(), &mut file)
.await? .await?
{ {
bail!( bail!(
"{} is not a valid webxdc file", "{} is not a valid webxdc file",
path.to_str().unwrap_or_default() path.to_str().unwrap_or_default()
); );
} else if buf.len() > WEBXDC_SENDING_LIMIT { }
let size = file.seek(SeekFrom::End(0))?;
if size > WEBXDC_SENDING_LIMIT {
bail!( bail!(
"webxdc {} exceeds acceptable size of {} bytes", "webxdc {} exceeds acceptable size of {} bytes",
path.to_str().unwrap_or_default(), path.to_str().unwrap_or_default(),
@@ -538,7 +548,11 @@ impl Message {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use std::io::Cursor;
use async_std::fs::File;
use async_std::io::WriteExt;
use crate::chat::{ use crate::chat::{
add_contact_to_chat, create_group_chat, forward_msgs, send_msg, send_text_msg, ChatId, add_contact_to_chat, create_group_chat, forward_msgs, send_msg, send_text_msg, ChatId,
ProtectionStatus, ProtectionStatus,
@@ -547,8 +561,8 @@ mod tests {
use crate::contact::Contact; use crate::contact::Contact;
use crate::dc_receive_imf::dc_receive_imf; use crate::dc_receive_imf::dc_receive_imf;
use crate::test_utils::TestContext; use crate::test_utils::TestContext;
use async_std::fs::File;
use async_std::io::WriteExt; use super::*;
#[allow(clippy::assertions_on_constants)] #[allow(clippy::assertions_on_constants)]
#[async_std::test] #[async_std::test]
@@ -566,35 +580,35 @@ mod tests {
assert!( assert!(
!t.is_webxdc_file( !t.is_webxdc_file(
"bad-ext-no-zip.txt", "bad-ext-no-zip.txt",
include_bytes!("../test-data/message/issue_523.txt") Cursor::new(include_bytes!("../test-data/message/issue_523.txt"))
) )
.await? .await?
); );
assert!( assert!(
!t.is_webxdc_file( !t.is_webxdc_file(
"bad-ext-good-zip.txt", "bad-ext-good-zip.txt",
include_bytes!("../test-data/webxdc/minimal.xdc") Cursor::new(include_bytes!("../test-data/webxdc/minimal.xdc"))
) )
.await? .await?
); );
assert!( assert!(
!t.is_webxdc_file( !t.is_webxdc_file(
"good-ext-no-zip.xdc", "good-ext-no-zip.xdc",
include_bytes!("../test-data/message/issue_523.txt") Cursor::new(include_bytes!("../test-data/message/issue_523.txt"))
) )
.await? .await?
); );
assert!( assert!(
!t.is_webxdc_file( !t.is_webxdc_file(
"good-ext-no-index-html.xdc", "good-ext-no-index-html.xdc",
include_bytes!("../test-data/webxdc/no-index-html.xdc") Cursor::new(include_bytes!("../test-data/webxdc/no-index-html.xdc"))
) )
.await? .await?
); );
assert!( assert!(
t.is_webxdc_file( t.is_webxdc_file(
"good-ext-good-zip.xdc", "good-ext-good-zip.xdc",
include_bytes!("../test-data/webxdc/minimal.xdc") Cursor::new(include_bytes!("../test-data/webxdc/minimal.xdc"))
) )
.await? .await?
); );