public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH http-server] multipart upload: properly parse file parts without Content-Type
@ 2023-04-11 12:19 Friedrich Weber
  2023-04-11 12:43 ` [pve-devel] applied: " Thomas Lamprecht
  0 siblings, 1 reply; 2+ messages in thread
From: Friedrich Weber @ 2023-04-11 12:19 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 2+ messages in thread

* [pve-devel] applied: [PATCH http-server] multipart upload: properly parse file parts without Content-Type
  2023-04-11 12:19 [pve-devel] [PATCH http-server] multipart upload: properly parse file parts without Content-Type Friedrich Weber
@ 2023-04-11 12:43 ` Thomas Lamprecht
  0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2023-04-11 12:43 UTC (permalink / raw)
  To: Proxmox VE development discussion, Friedrich Weber

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!




^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2023-04-11 12:44 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-11 12:19 [pve-devel] [PATCH http-server] multipart upload: properly parse file parts without Content-Type Friedrich Weber
2023-04-11 12:43 ` [pve-devel] applied: " Thomas Lamprecht

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal