Dominik Csapak [Mon, 7 Nov 2022 15:07:49 +0000 (16:07 +0100)]
upload: re-allow white space in filenames
Some fields (e.g. filename) can contain spaces, but our 'trim'
function, would only return the value until the first whitespace
character instead of removing leading/trailing white space. This lead
to passing the wrong filename to the API call (e.g. 'foo' instead of
'foo (1).iso'), which would then reject it because of the 'wrong'
extension.
Fix this by just using the battle proven trim from pve-common.
Fixes: 0fbcbc2 ("fix #3990: multipart upload: rework to fix uploading small files") Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
fix #3990: multipart upload: rework to fix uploading small files
== The problem
Upload of files smaller than ~16kb failed.
This was because the code assumed that it would be called
several times, and each time would do a certain action.
When the whole file was in the buffer, this failed because
the function would try parssing the first part in the payload and
then return, instead of parsing the rest of the available data.
== Why not just modifying the current code a bit?
The code had a lot of nested control statements and a
non-intuitive control flow (phase 0->2->1->1->1 and so on).
The way the phases and buffer content were checked made it
rather difficult to just fix a few lines.
== What was changed
* Part headers are parsed with a single regex line each,
which improves code readability.
* Parsing the content is done in order, so even if the whole data is in the buffer,
it can be read in one go. Files of arbitrary sizes can be uploaded.
Acknowledging the Content-Disposition header makes it possible for the
backend to tell the browser whether a file should be downloaded,
rather than displayed inline, and what it's default name should be.
Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
request: add missing early return to complete error check
While $self->error will immediately send out a 4xx or 5xx response
anyhow its still good to cover against possible side effects (e.g.,
from future code in that branch) on the server and return directly.
Note that this is mostly for completeness sake, we already have
another check that covers this one for relevant cases in commit 580d540ea907ba15f64379c5bb69ecf1a49a875f.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
requests: assert that theres no @ in the URLs authority
We don't expect any userinfo in the authority and t o avoid that this
allows some leverage in doing weird things later its better to error
out early on such requests.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
pass through streaming: only allow from privileged local pvedaemon
Ensures that no external request can control streaming on proxying
requests as safety net for when we'd have another issue in the
request handling part.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
basically only possible to trigger with chromium based browsers
(chrome, edge, opera) but besides those having the biggest usage
currently its not that nice in any way.
Users could inject headers in their response, which isn't really that
bad itself, as they won't really do anything at least for sane
browsers that don't allow setting third party cookies by default
(unlike again, chrome), in which case one can create huge cookies
that then trigger the max header size check on requests, DOS'ing an
user's access to a PVE interface if they can get them to visit a
malicious site (a clear cooki actione would allow visiting it again)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Reported-by: STAR Labs <info@starlabs.sg>
like the TLS <= 1.2 cipher list, but needs a different option since the
format and values are incompatible. AnyEvent doesn't yet handle this
directly like the cipher list, so set it directly on the context.
requires corresponding patch in pve-manager (which reads the config, and
passes relevant parts back to the API server).
fix #3807: don't attempt response on closed handle
if a client closes the connection while the API server is
waiting/stalling here, the handle will disappear, and sending a response
is no longer possible.
(this issue is only cosmetic, but if such clients are a regular
occurrence it might get quite noisy in the logs)
if the WS gets disconnected without any data having been sent first,
wbuf (and thus `length $wbuf`) is undef. the actual length of the buffer
is not relevant here anyway, just the fact that it's non-empty - so
avoid the undef warning by dropping the unnecessary comparison.
Stoiko Ivanov [Mon, 15 Nov 2021 20:50:43 +0000 (21:50 +0100)]
fix #3724: disable TLS renegotiation
The issue is probably not critical and best addressed by not running
the perl API servers in an exposed environment or when this needs to
be done by installing a reverse proxy in front of them.
The DOS potential of the perl daemons is limited more by the limited
number of parallel workers (and the memory constraints of starting
more of them), than by the CPU cycles wasted on TLS renegotiation.
Still disabling TLS renegotiation should show very little downside:
* it was removed in TLS 1.3 for security reasons
* it was the way nginx addressed this issue [1].
* we do not use client certificate authentication
Tested by running `openssl s_client -no_tls1_3 -connect 192.0.2.1:8006`
and issuing a `HEAD / HTTP/1.1\nR\n`
with and without the patch.
[1] 70bd187c4c386d82d6e4d180e0db84f361d1be02 at
https://github.com/nginx/nginx (although that code adapted to
the various changes in openssl API over the years) Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
for proxied requests, we usually tear down the proxy connection
immediately when closing the source connection. this is not the correct
course of action for bulk one-way data streams that are proxied, where
the source connection might be closed, but the proxy connection might
still have data in the write buffer that needs to be written out.
push_shutdown already handles this case (closing the socket/FH after it
has been fully drained).
one example for such a proxied data stream is the 'migrate' data for a
remote migration, which gets proxied over a websocket connection.
terminating the proxied connection early makes the target VM crash for
obvious reasons.
this major release still needs to have an incompatible client, the next
one can drop setting a protocol client-side, and the one after that can
remove the protocol handling on the server side.
Stoiko Ivanov [Wed, 5 May 2021 14:36:27 +0000 (16:36 +0200)]
access control: also include ipv6 in 'all'
Net::IP objects are bound to a version - 0/0 is treated as ipv4 only.
If 'all' is present in the allow_from/deny_from list we should also
add ::/0 for matching all ipv6 addresses.
Stoiko Ivanov [Wed, 5 May 2021 14:36:26 +0000 (16:36 +0200)]
access control: correctly match v4-mapped-v6 addresses
With recent changes to the listening socket code in pve-manager
the proxy daemons now usually bind to '::' and ipv4 clients are
read as v4-mapped-v6 addresses [0] from ::ffff:0:0/96.
This caused the allow_from/deny_from matching to break.
This patch addresses the issue by normalizing addresses from
::ffff:0:0/96 using Net::IP::ip_get_embedded_ipv4
(which roughly splits on ':' and checks if the last part looks like an
ipv4 address).
Issue was originally reported in our community forum [1]
Stefan Reiter [Thu, 22 Apr 2021 15:34:53 +0000 (17:34 +0200)]
allow stream download from path and over pvedaemon-proxy
Allow specifying a filepath for stream=1 instead of either a path or fh
with stream=1.
With this in place, we can also just return the path to the proxy in
case we want to stream a response back, and let it read from the file
itself. This way, the pvedaemon is cut out of the transfer pipe.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
Stefan Reiter [Thu, 22 Apr 2021 15:34:52 +0000 (17:34 +0200)]
support streaming data form fh to client
Use an explicit AnyEvent::Handle similar to websocket proxying.
Needs some special care to make sure we apply backpressure correctly to
avoid caching too much data. Note that because of AnyEvent restrictions,
specifying a "fh" to point to a file or a packet-based socket may result
in unwanted behaviour[0].
Stefan Reiter [Wed, 21 Apr 2021 11:15:35 +0000 (13:15 +0200)]
allow 'download' to be passed from API handler
PVE::HTTPServer in pve-manager wraps the API return value in a 'data'
element, look for a 'download' element there too to allow an API call to
instruct the HTTP server to return a file via path or filehandle.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
Stoiko Ivanov [Thu, 10 Dec 2020 14:02:50 +0000 (15:02 +0100)]
accept-phase: shutdown socket on early error
if an error happens before AnyEvent::Handle registers the cleanup
callback, we should shutdown/close the socket, when handling it.
Using close, instead of shutdown($sock, SHUT_WR) here, since we are in
an error-state, and would not read from the socket anyways.
(Additionally close sends just on packet (RST,ACK), vs shutdown
(FIN,ACK+RST,ACK) in its use here).
Stoiko Ivanov [Thu, 10 Dec 2020 14:02:49 +0000 (15:02 +0100)]
accept-phase: fix conn_count "leak"
When handling new connections in 'accept_connections' the number of
active connections (conn_count) got increased before the callback, which
would eventually decrease it got registered in AnyEvent::Handle->new.
Any error/die before registering the callback would skip the
decrement, and leave the process in an endless loop upon exiting in
wait_end_loop.
This can happen e.g. when the call to getpeername fails, or if the
connection is denied by the ALLOW_FROM/DENY_FROM settings in
'/etc/default/pveproxy' (which is also a simple reproducer for that).
Additionally it can cause a denial of service, by attempting to
connect from a denied ip until the connection count exeeds the maximum
connections of all child-processes.
This patch addresses the issue by incrementing the connection count
before attempting to create the handle, and decrementing it again, if
handle creation fails.
A warning is logged if 'conn_count' turns negative when decrementing
during cleanup on error/eof. In case creating a new handle during
initial accept_connection fails, a warning is logged as well, but
'conn_count' is not decremented.
Reported via our community-forum:
https://forum.proxmox.com/threads/pveproxy-eats-available-ram.79617/
increase max headers to 64 to cope with modern browsers + proxy combinations
This is mostly a "do not allow infinity headers" limit in the sense
of "it's good to have limits". With modern browsers and users behind
proxies we may actually get over 30 headers, so increase it for now
to 64 - hopefully enough for another decade ;)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Reported-by: Victor Hooi <victorhooi@yahoo.com>
> The Ping frame contains an opcode of 0x9.
> [...]
> The Pong frame contains an opcode of 0xA.
-- Section 5.5.2 cf. https://tools.ietf.org/html/rfc6455#section-5.5.2
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Tim Marx [Tue, 21 Jan 2020 12:54:19 +0000 (13:54 +0100)]
allow ticket in auth header as fallback
based on idea & RFC by Tim Marx, incorporating feedback by Thomas
Lamprecht. this will be extended to support API tokens in the
Authorization header as well, so make it generic.
Thomas Lamprecht [Fri, 25 Oct 2019 15:34:27 +0000 (17:34 +0200)]
Revert "tls: make dh to openssl 1.1 compatible"
The libanyevent-perl version 7.140-3 included a fix for this.
It migrated to the then still testing (buster was not yet released)
on 07.04.2019, and so we can safely revert this workaround again
here.
Albeit this was fixed since Buster was officially released, still
bump the version dependency to libanyevent-perl in debian/control.
A future libanyevent-perl will use "ffdhe3072" for DH; another good
reason to revert this, to not keep hardcoded parameters with possible
(future) security implications here.
Dominik Csapak [Thu, 10 Oct 2019 09:21:51 +0000 (11:21 +0200)]
AnyEvent: extend send_file with content-type
for pmg, we need to send temporary files (for the attachment quarantine),
but we cannot know beforehand what content-type it is, so we
optionally give it to send_file_start
we give a hash with the open filehandle and the content-type in that case
this also removes the unnecessary open on the filename, since we open
it in send_file_start anyway...
Avoids syslog/journal warning like:
> Use of uninitialized value $v in substitution (s///) at
> /usr/share/perl5/PVE/APIServer/AnyEvent.pm line 648.
If one passes a "value-less" GET argument to a request, e.g.,
GET /?debug
Besides the fact that this allows us to even use such arguments it
also is a general improvement against a slight "syslog DOS attack",
because anybody can pass such parameters to the '/' page, and all
proxmox daemons providing a API/UI using libpve-http-server-perl
allow to do such requests unauthenticated (which itself is OK, as
else one could not show the login window at all). As each of such
request produces two log lines in the syslog/journal it's far from
ideal.
A simple reproducer of the possible outcome can be seen with the
following shell script using curl:
> PVEURL='127.0.0.1'
> ARGS='?a'; # send multiple args at once to amplify the per-connection cost
> for c in {a..z}; do for i in {0..9}; do ARGS="$ARGS&$c$i"; done; done
> while true; do curl --insecure --silent --output /dev/null "https://$PVEURL:8006$ARGS"; done
Not really bad, but not nice either, as logging is not too cheap this
has some resource usage cost and noise in the syslog is never nice.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Wed, 22 May 2019 06:07:17 +0000 (08:07 +0200)]
anyevent: rpcenv is optional and from our child instance
This fixes the simple-demo, which was regressed with commit 8782148642e4a09c5852781ec057017cc1145f17 were we falsely assumed that
we always have an rpcenv instance here, but actually that's just
optional as it comes from our child class instance
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Tue, 21 May 2019 19:37:12 +0000 (21:37 +0200)]
buildsys: fix dependencies for bootstrap files
avoid removing and unzipping the bootstrap source archive as many
times as ${BTDATA} has file entries, add an intermediate target for
the directory, which is the producer for all those BTDATA files, and
that directory then depends on the zip archive.
I mean, it would be even better to just use the libjs-bootstrap
package (and jQuery for that matter) but that a little bigger change
for now
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>