From 29ed82a3595e0cdd39deb914dc38002478f89f97 Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Tue, 25 May 2021 03:48:57 -0700 Subject: [PATCH 1/4] Add support for v2 attachment upload APIs Upstream PR: https://github.com/bitwarden/server/pull/1229 --- src/api/core/ciphers.rs | 204 ++++++++++++++++++++++++++++++++---- src/api/core/sends.rs | 2 +- src/crypto.rs | 7 +- src/db/models/attachment.rs | 21 ++-- 4 files changed, 200 insertions(+), 34 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 3832aeb4..12d739b5 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -6,7 +6,6 @@ use rocket::{http::ContentType, request::Form, Data, Route}; use rocket_contrib::json::Json; use serde_json::Value; -use data_encoding::HEXLOWER; use multipart::server::{save::SavedData, Multipart, SaveResult}; use crate::{ @@ -40,8 +39,10 @@ pub fn routes() -> Vec { post_ciphers_create, post_ciphers_import, get_attachment, - post_attachment, - post_attachment_admin, + post_attachment_v2, + post_attachment_v2_data, + post_attachment, // legacy + post_attachment_admin, // legacy post_attachment_share, delete_attachment_post, delete_attachment_post_admin, @@ -755,6 +756,12 @@ fn share_cipher_by_uuid( Ok(Json(cipher.to_json(&headers.host, &headers.user.uuid, &conn))) } +/// v2 API for downloading an attachment. This just redirects the client to +/// the actual location of an attachment. +/// +/// Upstream added this v2 API to support direct download of attachments from +/// their object storage service. For self-hosted instances, it basically just +/// redirects to the same location as before the v2 API. #[get("/ciphers//attachment/")] fn get_attachment(uuid: String, attachment_id: String, headers: Headers, conn: DbConn) -> JsonResult { match Attachment::find_by_id(&attachment_id, &conn) { @@ -764,16 +771,79 @@ fn get_attachment(uuid: String, attachment_id: String, headers: Headers, conn: D } } -#[post("/ciphers//attachment", format = "multipart/form-data", data = "")] -fn post_attachment( +#[derive(Deserialize)] +#[allow(non_snake_case)] +struct AttachmentRequestData { + Key: String, + FileName: String, + FileSize: i32, + // We check org owner/admin status via is_write_accessible_to_user(), + // so we can just ignore this field. + // + // AdminRequest: bool, +} + +enum FileUploadType { + Direct = 0, + // Azure = 1, // only used upstream +} + +/// v2 API for creating an attachment associated with a cipher. +/// This redirects the client to the API it should use to upload the attachment. +/// For upstream's cloud-hosted service, it's an Azure object storage API. +/// For self-hosted instances, it's another API on the local instance. +#[post("/ciphers//attachment/v2", data = "")] +fn post_attachment_v2( uuid: String, - data: Data, - content_type: &ContentType, + data: JsonUpcase, headers: Headers, conn: DbConn, - nt: Notify, ) -> JsonResult { let cipher = match Cipher::find_by_uuid(&uuid, &conn) { + Some(cipher) => cipher, + None => err!("Cipher doesn't exist"), + }; + + if !cipher.is_write_accessible_to_user(&headers.user.uuid, &conn) { + err!("Cipher is not write accessible") + } + + let attachment_id = crypto::generate_file_id(); + let data: AttachmentRequestData = data.into_inner().data; + let attachment = + Attachment::new(attachment_id.clone(), cipher.uuid.clone(), data.FileName, data.FileSize, Some(data.Key)); + attachment.save(&conn).expect("Error saving attachment"); + + let url = format!("/ciphers/{}/attachment/{}", cipher.uuid, attachment_id); + + Ok(Json(json!({ // AttachmentUploadDataResponseModel + "Object": "attachment-fileUpload", + "AttachmentId": attachment_id, + "Url": url, + "FileUploadType": FileUploadType::Direct as i32, + "CipherResponse": cipher.to_json(&headers.host, &headers.user.uuid, &conn), + "CipherMiniResponse": null, + }))) +} + +/// Saves the data content of an attachment to a file. This is common code +/// shared between the v2 and legacy attachment APIs. +/// +/// When used with the legacy API, this function is responsible for creating +/// the attachment database record, so `attachment` is None. +/// +/// When used with the v2 API, post_attachment_v2() has already created the +/// database record, which is passed in as `attachment`. +fn save_attachment( + mut attachment: Option, + cipher_uuid: String, + data: Data, + content_type: &ContentType, + headers: &Headers, + conn: &DbConn, + nt: Notify, +) -> Result { + let cipher = match Cipher::find_by_uuid(&cipher_uuid, conn) { Some(cipher) => cipher, None => err_discard!("Cipher doesn't exist", data), }; @@ -782,10 +852,6 @@ fn post_attachment( err_discard!("Cipher is not write accessible", data) } - let mut params = content_type.params(); - let boundary_pair = params.next().expect("No boundary provided"); - let boundary = boundary_pair.1; - let size_limit = if let Some(ref user_uuid) = cipher.user_uuid { match CONFIG.user_attachment_limit() { Some(0) => err_discard!("Attachments are disabled", data), @@ -814,7 +880,11 @@ fn post_attachment( err_discard!("Cipher is neither owned by a user nor an organization", data); }; - let base_path = Path::new(&CONFIG.attachments_folder()).join(&cipher.uuid); + let mut params = content_type.params(); + let boundary_pair = params.next().expect("No boundary provided"); + let boundary = boundary_pair.1; + + let base_path = Path::new(&CONFIG.attachments_folder()).join(&cipher_uuid); let mut attachment_key = None; let mut error = None; @@ -830,11 +900,20 @@ fn post_attachment( } } "data" => { - // This is provided by the client, don't trust it - let name = field.headers.filename.expect("No filename provided"); + // In the legacy API, this is the encrypted filename + // provided by the client, stored to the database as-is. + // In the v2 API, this value doesn't matter, as it was + // already provided and stored via an earlier API call. + let encrypted_filename = field.headers.filename; - let file_name = HEXLOWER.encode(&crypto::get_random(vec![0; 10])); - let path = base_path.join(&file_name); + // This random ID is used as the name of the file on disk. + // In the legacy API, we need to generate this value here. + // In the v2 API, we use the value from post_attachment_v2(). + let file_id = match &attachment { + Some(attachment) => attachment.id.clone(), // v2 API + None => crypto::generate_file_id(), // Legacy API + }; + let path = base_path.join(&file_id); let size = match field.data.save().memory_threshold(0).size_limit(size_limit).with_path(path.clone()) { @@ -856,9 +935,50 @@ fn post_attachment( } }; - let mut attachment = Attachment::new(file_name, cipher.uuid.clone(), name, size); - attachment.akey = attachment_key.clone(); - attachment.save(&conn).expect("Error saving attachment"); + if let Some(attachment) = &mut attachment { + // v2 API + + // Check the actual size against the size initially provided by + // the client. Upstream allows +/- 1 MiB deviation from this + // size, but it's not clear when or why this is needed. + const LEEWAY: i32 = 1024 * 1024; // 1 MiB + let min_size = attachment.file_size - LEEWAY; + let max_size = attachment.file_size + LEEWAY; + + if min_size <= size && size <= max_size { + if size != attachment.file_size { + // Update the attachment with the actual file size. + attachment.file_size = size; + attachment.save(conn).expect("Error updating attachment"); + } + } else { + std::fs::remove_file(path).ok(); + attachment.delete(conn).ok(); + + let err_msg = "Attachment size mismatch".to_string(); + error!("{} (expected within [{}, {}], got {})", err_msg, min_size, max_size, size); + error = Some(err_msg); + } + } else { + // Legacy API + + if encrypted_filename.is_none() { + error = Some("No filename provided".to_string()); + return; + } + if attachment_key.is_none() { + error = Some("No attachment key provided".to_string()); + return; + } + let attachment = Attachment::new( + file_id, + cipher_uuid.clone(), + encrypted_filename.unwrap(), + size, + attachment_key.clone(), + ); + attachment.save(conn).expect("Error saving attachment"); + } } _ => error!("Invalid multipart name"), } @@ -871,6 +991,50 @@ fn post_attachment( nt.send_cipher_update(UpdateType::CipherUpdate, &cipher, &cipher.update_users_revision(&conn)); + Ok(cipher) +} + +/// v2 API for uploading the actual data content of an attachment. +/// This route needs a rank specified so that Rocket prioritizes the +/// /ciphers//attachment/v2 route, which would otherwise conflict +/// with this one. +#[post("/ciphers//attachment/", format = "multipart/form-data", data = "", rank = 1)] +fn post_attachment_v2_data( + uuid: String, + attachment_id: String, + data: Data, + content_type: &ContentType, + headers: Headers, + conn: DbConn, + nt: Notify, +) -> EmptyResult { + let attachment = match Attachment::find_by_id(&attachment_id, &conn) { + Some(attachment) if uuid == attachment.cipher_uuid => Some(attachment), + Some(_) => err!("Attachment doesn't belong to cipher"), + None => err!("Attachment doesn't exist"), + }; + + save_attachment(attachment, uuid, data, content_type, &headers, &conn, nt)?; + + Ok(()) +} + +/// Legacy API for creating an attachment associated with a cipher. +#[post("/ciphers//attachment", format = "multipart/form-data", data = "")] +fn post_attachment( + uuid: String, + data: Data, + content_type: &ContentType, + headers: Headers, + conn: DbConn, + nt: Notify, +) -> JsonResult { + // Setting this as None signifies to save_attachment() that it should create + // the attachment database record as well as saving the data to disk. + let attachment = None; + + let cipher = save_attachment(attachment, uuid, data, content_type, &headers, &conn, nt)?; + Ok(Json(cipher.to_json(&headers.host, &headers.user.uuid, &conn))) } diff --git a/src/api/core/sends.rs b/src/api/core/sends.rs index 25ad774c..7b2f2c4d 100644 --- a/src/api/core/sends.rs +++ b/src/api/core/sends.rs @@ -173,7 +173,7 @@ fn post_send_file(data: Data, content_type: &ContentType, headers: Headers, conn // Create the Send let mut send = create_send(data.data, headers.user.uuid.clone())?; - let file_id: String = data_encoding::HEXLOWER.encode(&crate::crypto::get_random(vec![0; 32])); + let file_id = crate::crypto::generate_file_id(); if send.atype != SendType::File as i32 { err!("Send content is not a file"); diff --git a/src/crypto.rs b/src/crypto.rs index 43b8fc7d..2b946b0b 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -3,6 +3,7 @@ // use std::num::NonZeroU32; +use data_encoding::HEXLOWER; use ring::{digest, hmac, pbkdf2}; use crate::error::Error; @@ -28,8 +29,6 @@ pub fn verify_password_hash(secret: &[u8], salt: &[u8], previous: &[u8], iterati // HMAC // pub fn hmac_sign(key: &str, data: &str) -> String { - use data_encoding::HEXLOWER; - let key = hmac::Key::new(hmac::HMAC_SHA1_FOR_LEGACY_USE_ONLY, key.as_bytes()); let signature = hmac::sign(&key, data.as_bytes()); @@ -52,6 +51,10 @@ pub fn get_random(mut array: Vec) -> Vec { array } +pub fn generate_file_id() -> String { + HEXLOWER.encode(&get_random(vec![0; 16])) // 128 bits +} + pub fn generate_token(token_size: u32) -> Result { // A u64 can represent all whole numbers up to 19 digits long. if token_size > 19 { diff --git a/src/db/models/attachment.rs b/src/db/models/attachment.rs index 76bc474e..f34150a1 100644 --- a/src/db/models/attachment.rs +++ b/src/db/models/attachment.rs @@ -12,7 +12,7 @@ db_object! { pub struct Attachment { pub id: String, pub cipher_uuid: String, - pub file_name: String, + pub file_name: String, // encrypted pub file_size: i32, pub akey: Option, } @@ -20,13 +20,13 @@ db_object! { /// Local methods impl Attachment { - pub const fn new(id: String, cipher_uuid: String, file_name: String, file_size: i32) -> Self { + pub const fn new(id: String, cipher_uuid: String, file_name: String, file_size: i32, akey: Option) -> Self { Self { id, cipher_uuid, file_name, file_size, - akey: None, + akey, } } @@ -34,18 +34,17 @@ impl Attachment { format!("{}/{}/{}", CONFIG.attachments_folder(), self.cipher_uuid, self.id) } + pub fn get_url(&self, host: &str) -> String { + format!("{}/attachments/{}/{}", host, self.cipher_uuid, self.id) + } + pub fn to_json(&self, host: &str) -> Value { - use crate::util::get_display_size; - - let web_path = format!("{}/attachments/{}/{}", host, self.cipher_uuid, self.id); - let display_size = get_display_size(self.file_size); - json!({ "Id": self.id, - "Url": web_path, + "Url": self.get_url(host), "FileName": self.file_name, "Size": self.file_size.to_string(), - "SizeName": display_size, + "SizeName": crate::util::get_display_size(self.file_size), "Key": self.akey, "Object": "attachment" }) @@ -91,7 +90,7 @@ impl Attachment { } } - pub fn delete(self, conn: &DbConn) -> EmptyResult { + pub fn delete(&self, conn: &DbConn) -> EmptyResult { db_run! { conn: { crate::util::retry( || diesel::delete(attachments::table.filter(attachments::id.eq(&self.id))).execute(conn), From 5fef7983f4e3bc942ec0f029037454edfb057cad Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Tue, 25 May 2021 22:13:04 -0700 Subject: [PATCH 2/4] Clean up attachment error handling --- src/api/core/ciphers.rs | 10 ++++------ src/db/models/attachment.rs | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 12d739b5..c8bc1ea0 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -1,5 +1,5 @@ use std::collections::{HashMap, HashSet}; -use std::path::Path; +use std::path::{Path, PathBuf}; use chrono::{NaiveDateTime, Utc}; use rocket::{http::ContentType, request::Form, Data, Route}; @@ -885,6 +885,7 @@ fn save_attachment( let boundary = boundary_pair.1; let base_path = Path::new(&CONFIG.attachments_folder()).join(&cipher_uuid); + let mut path = PathBuf::new(); let mut attachment_key = None; let mut error = None; @@ -913,23 +914,20 @@ fn save_attachment( Some(attachment) => attachment.id.clone(), // v2 API None => crypto::generate_file_id(), // Legacy API }; - let path = base_path.join(&file_id); + path = base_path.join(&file_id); let size = match field.data.save().memory_threshold(0).size_limit(size_limit).with_path(path.clone()) { SaveResult::Full(SavedData::File(_, size)) => size as i32, SaveResult::Full(other) => { - std::fs::remove_file(path).ok(); error = Some(format!("Attachment is not a file: {:?}", other)); return; } SaveResult::Partial(_, reason) => { - std::fs::remove_file(path).ok(); error = Some(format!("Attachment size limit exceeded with this file: {:?}", reason)); return; } SaveResult::Error(e) => { - std::fs::remove_file(path).ok(); error = Some(format!("Error: {:?}", e)); return; } @@ -952,7 +950,6 @@ fn save_attachment( attachment.save(conn).expect("Error updating attachment"); } } else { - std::fs::remove_file(path).ok(); attachment.delete(conn).ok(); let err_msg = "Attachment size mismatch".to_string(); @@ -986,6 +983,7 @@ fn save_attachment( .expect("Error processing multipart data"); if let Some(ref e) = error { + std::fs::remove_file(path).ok(); err!(e); } diff --git a/src/db/models/attachment.rs b/src/db/models/attachment.rs index f34150a1..cf25acdc 100644 --- a/src/db/models/attachment.rs +++ b/src/db/models/attachment.rs @@ -1,3 +1,5 @@ +use std::io::ErrorKind; + use serde_json::Value; use super::Cipher; @@ -98,8 +100,19 @@ impl Attachment { ) .map_res("Error deleting attachment")?; - crate::util::delete_file(&self.get_file_path())?; - Ok(()) + let file_path = &self.get_file_path(); + + match crate::util::delete_file(file_path) { + // Ignore "file not found" errors. This can happen when the + // upstream caller has already cleaned up the file as part of + // its own error handling. + Err(e) if e.kind() == ErrorKind::NotFound => { + debug!("File '{}' already deleted.", file_path); + Ok(()) + } + Err(e) => Err(e.into()), + _ => Ok(()), + } }} } From c2ef331df9d2a1a3e50ed8129b07cca0a52e6f41 Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Tue, 25 May 2021 23:15:24 -0700 Subject: [PATCH 3/4] Rework file ID generation --- src/api/core/ciphers.rs | 4 ++-- src/api/core/sends.rs | 2 +- src/crypto.rs | 14 ++++++++++++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index c8bc1ea0..f2631984 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -808,7 +808,7 @@ fn post_attachment_v2( err!("Cipher is not write accessible") } - let attachment_id = crypto::generate_file_id(); + let attachment_id = crypto::generate_attachment_id(); let data: AttachmentRequestData = data.into_inner().data; let attachment = Attachment::new(attachment_id.clone(), cipher.uuid.clone(), data.FileName, data.FileSize, Some(data.Key)); @@ -912,7 +912,7 @@ fn save_attachment( // In the v2 API, we use the value from post_attachment_v2(). let file_id = match &attachment { Some(attachment) => attachment.id.clone(), // v2 API - None => crypto::generate_file_id(), // Legacy API + None => crypto::generate_attachment_id(), // Legacy API }; path = base_path.join(&file_id); diff --git a/src/api/core/sends.rs b/src/api/core/sends.rs index 7b2f2c4d..21d1706c 100644 --- a/src/api/core/sends.rs +++ b/src/api/core/sends.rs @@ -173,7 +173,7 @@ fn post_send_file(data: Data, content_type: &ContentType, headers: Headers, conn // Create the Send let mut send = create_send(data.data, headers.user.uuid.clone())?; - let file_id = crate::crypto::generate_file_id(); + let file_id = crate::crypto::generate_send_id(); if send.atype != SendType::File as i32 { err!("Send content is not a file"); diff --git a/src/crypto.rs b/src/crypto.rs index 2b946b0b..61e55649 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -51,8 +51,18 @@ pub fn get_random(mut array: Vec) -> Vec { array } -pub fn generate_file_id() -> String { - HEXLOWER.encode(&get_random(vec![0; 16])) // 128 bits +pub fn generate_id(num_bytes: usize) -> String { + HEXLOWER.encode(&get_random(vec![0; num_bytes])) +} + +pub fn generate_send_id() -> String { + // Send IDs are globally scoped, so make them longer to avoid collisions. + generate_id(32) // 256 bits +} + +pub fn generate_attachment_id() -> String { + // Attachment IDs are scoped to a cipher, so they can be smaller. + generate_id(10) // 80 bits } pub fn generate_token(token_size: u32) -> Result { From 3f7e4712cd9e62ce85f58e449074b98e5c95a142 Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Tue, 25 May 2021 23:17:22 -0700 Subject: [PATCH 4/4] Fix attachment size limit calculation for v2 uploads --- src/api/core/ciphers.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index f2631984..88dfa3fa 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -852,11 +852,18 @@ fn save_attachment( err_discard!("Cipher is not write accessible", data) } + // In the v2 API, the attachment record has already been created, + // so the size limit needs to be adjusted to account for that. + let size_adjust = match &attachment { + None => 0, // Legacy API + Some(a) => a.file_size as i64, // v2 API + }; + let size_limit = if let Some(ref user_uuid) = cipher.user_uuid { match CONFIG.user_attachment_limit() { Some(0) => err_discard!("Attachments are disabled", data), Some(limit_kb) => { - let left = (limit_kb * 1024) - Attachment::size_by_user(user_uuid, &conn); + let left = (limit_kb * 1024) - Attachment::size_by_user(user_uuid, &conn) + size_adjust; if left <= 0 { err_discard!("Attachment size limit reached! Delete some files to open space", data) } @@ -868,7 +875,7 @@ fn save_attachment( match CONFIG.org_attachment_limit() { Some(0) => err_discard!("Attachments are disabled", data), Some(limit_kb) => { - let left = (limit_kb * 1024) - Attachment::size_by_org(org_uuid, &conn); + let left = (limit_kb * 1024) - Attachment::size_by_org(org_uuid, &conn) + size_adjust; if left <= 0 { err_discard!("Attachment size limit reached! Delete some files to open space", data) }