Skip to content

Commit

Permalink
Merge pull request from GHSA-q684-4jjh-83g6
Browse files Browse the repository at this point in the history
S3 storages support user-specified endpoint URLs, and Azure storages support
user-specified connection strings (which can contain endpoint URLs), so they
are susceptible to SSRF. Make S3 and Azure requests go through smokescreen
to fix this.

AFAIK, there is no way to configure a custom URL for Google Cloud storages,
so those aren't vulnerable.

Co-authored-by: Nikita Manovich <nikita@cvat.ai>
  • Loading branch information
SpecLad and nmanovic authored Jun 13, 2024
1 parent cf2f329 commit f234693
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 4 deletions.
4 changes: 4 additions & 0 deletions changelog.d/20240610_143947_roman_ssrf_s3.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Security

- Fixed an SSRF vulnerability with custom cloud storage endpoints
(<https://github.com/cvat-ai/cvat/security/advisories/GHSA-q684-4jjh-83g6>)
15 changes: 11 additions & 4 deletions cvat/apps/engine/cloud_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from azure.storage.blob import BlobServiceClient, ContainerClient, PublicAccess
from azure.storage.blob._list_blobs_helper import BlobPrefix
from boto3.s3.transfer import TransferConfig
from botocore.client import Config
from botocore.exceptions import ClientError
from botocore.handlers import disable_signing
from datumaro.util import take_by # can be changed to itertools.batched after migration to python3.12
Expand All @@ -32,6 +33,7 @@
from cvat.apps.engine.log import ServerLogManager
from cvat.apps.engine.models import CloudProviderChoice, CredentialsTypeChoice
from cvat.apps.engine.utils import get_cpu_number
from cvat.utils.http import PROXIES_FOR_UNTRUSTED_URLS

class NamedBytesIO(BytesIO):
@property
Expand Down Expand Up @@ -442,7 +444,9 @@ def __init__(self,
kwargs[key] = arg_v

session = boto3.Session(**kwargs)
self._s3 = session.resource("s3", endpoint_url=endpoint_url)
self._s3 = session.resource("s3", endpoint_url=endpoint_url,
config=Config(proxies=PROXIES_FOR_UNTRUSTED_URLS or {}),
)

# anonymous access
if not any([access_key_id, secret_key, session_token]):
Expand Down Expand Up @@ -644,11 +648,14 @@ def __init__(
super().__init__(prefix=prefix)
self._account_name = account_name
if connection_string:
self._blob_service_client = BlobServiceClient.from_connection_string(connection_string)
self._blob_service_client = BlobServiceClient.from_connection_string(
connection_string, proxies=PROXIES_FOR_UNTRUSTED_URLS)
elif sas_token:
self._blob_service_client = BlobServiceClient(account_url=self.account_url, credential=sas_token)
self._blob_service_client = BlobServiceClient(
account_url=self.account_url, credential=sas_token, proxies=PROXIES_FOR_UNTRUSTED_URLS)
else:
self._blob_service_client = BlobServiceClient(account_url=self.account_url)
self._blob_service_client = BlobServiceClient(
account_url=self.account_url, proxies=PROXIES_FOR_UNTRUSTED_URLS)
self._client = self._blob_service_client.get_container_client(container)

@property
Expand Down
3 changes: 3 additions & 0 deletions supervisord/worker.export.conf
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@ environment=VECTOR_EVENT_HANDLER="SynchronousLogstashHandler",CVAT_POSTGRES_APPL
numprocs=%(ENV_NUMPROCS)s
process_name=%(program_name)s-%(process_num)d
autorestart=true

[program:smokescreen]
command=smokescreen --listen-ip=127.0.0.1 %(ENV_SMOKESCREEN_OPTS)s
3 changes: 3 additions & 0 deletions tests/allow_minio.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Same as allow_webhooks_receiver.sh, but for minio.
minio_ip_addr="$(getent hosts minio | head -1 | awk '{ print $1 }')"
export SMOKESCREEN_OPTS="$SMOKESCREEN_OPTS --allow-address=\"$minio_ip_addr\""
10 changes: 10 additions & 0 deletions tests/docker-compose.minio.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@
x-allow-minio: &allow-minio
depends_on:
- minio
volumes:
- ./tests/allow_minio.sh:/etc/cvat/init.d/allow_minio.sh:ro

services:
cvat_server: *allow-minio
cvat_worker_export: *allow-minio
cvat_worker_import: *allow-minio

minio:
image: quay.io/minio/minio:RELEASE.2022-09-17T00-09-45Z
hostname: minio
Expand Down

0 comments on commit f234693

Please sign in to comment.