From 602eb8aabdc04180962d5c3b6f1c66358a8d2690 Mon Sep 17 00:00:00 2001 From: Friedrich Weber Date: Tue, 11 Apr 2023 14:19:47 +0200 Subject: [PATCH] multipart upload: properly parse file parts without Content-Type As reported in the forum, multipart requests are parsed incorrectly if the file part header contains *only* Content-Disposition, but no other fields (in particular, no Content-Type). As a result, uploaded files are mangled: In most cases, an additional carriage return and line feed (\r\n) is prepended to the file contents. As an example, consider the following file part (with explicit \r\n for clarity): Content-Disposition: form-data; name=...; filename=...\r\n Content-Type: application/x-iso9660-image\r\n \r\n file contents... The current parsing code for file parts roughly works as follows: 1) Consume the Content-Disposition field including the trailing \r\n 2) Consume and ignore everything up to and including the next \r\n\r\n 3) Read the file contents This works fine in the example above. However, it has a bug in case Content-Disposition is the *only* header field: Content-Disposition: form-data; name=...; filename=...\r\n \r\n file contents... Now, step 1 already consumes the first half of the \r\n\r\n sequence that marks the end of the part headers. As a result, step 3 starts reading the file at a wrong offset: - If the remaining contents of the read buffer (currently sized 16KiB) contain \r\n\r\n, step 2 consumes everything up to and including this marker and step 3 starts reading file contents there. As a result, the uploaded file is truncated at its beginning. - Otherwise, step 2 is a noop and step 3 considers the remaining second half of the \r\n\r\n marker to be part of the file contents. As a result, the uploaded file is prepended with an extra \r\n. To fix this, modify step 1 to *not* consume the trailing \r\n. This keeps the \r\n\r\n marker intact, no matter whether additional header fields are present or not. Fixes: 3e3faddb4a3792557351f1a6e9f2685e4713eb24 Link: https://forum.proxmox.com/threads/125411/ Signed-off-by: Friedrich Weber --- src/PVE/APIServer/AnyEvent.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm index 1f9952f..ac48899 100644 --- a/src/PVE/APIServer/AnyEvent.pm +++ b/src/PVE/APIServer/AnyEvent.pm @@ -1222,7 +1222,7 @@ sub file_upload_multipart { $extract_form_disposition->('checksum-algorithm'); $extract_form_disposition->('checksum'); - if ($hdl->{rbuf} =~ s/^${delim_re}Content-Disposition: (.*?); name="(.*?)"; filename="([^"]+)"${newline_re}//s) { + if ($hdl->{rbuf} =~ s/^${delim_re}Content-Disposition: (.*?); name="(.*?)"; filename="([^"]+)"//s) { assert_form_disposition($1); die "wrong field name '$2' for file upload, expected 'filename'" if $2 ne "filename"; $rstate->{phase} = 2; -- 2.39.2