[pve-devel] [PATCH http-server] multipart upload: properly parse file parts without Content-Type

[pve-devel] [PATCH http-server] multipart upload: properly parse file parts without Content-Type

[Friedrich Weber]

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 <f.weber@proxmox.com>
---
 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.30.2

[pve-devel] applied: [PATCH http-server] multipart upload: properly parse file parts without Content-Type

[Thomas Lamprecht]

Am 11/04/2023 um 14:19 schrieb Friedrich Weber:
> 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 <f.weber@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!