diff --git a/CHANGELOG.md b/CHANGELOG.md index aaabcfd2e9..33878802aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ This is a list of high-level changes for each release of AWX. A full list of commits can be found at `https://github.com/ansible/awx/releases/tag/`. +## 14.0.0 (Aug TBD, 2020) +- Fixed https://access.redhat.com/security/cve/cve-2020-14327 - Server-side request forgery on credentials +- Fixed https://access.redhat.com/security/cve/cve-2020-14328 - Server-side request forgery on webhooks +- Fixed https://access.redhat.com/security/cve/cve-2020-14329 - Sensitive data exposure on labels +- Fixed https://access.redhat.com/security/cve/cve-2020-14337 - Named URLs allow for testing the presence or absence of objects + ## 13.0.0 (Jun 23, 2020) - Added import and export commands to the official AWX CLI, replacing send and receive from the old tower-cli (https://github.com/ansible/awx/pull/6125). - Removed scripts as a means of running inventory updates of built-in types (https://github.com/ansible/awx/pull/6911) diff --git a/awx/api/generics.py b/awx/api/generics.py index c7e68bfc49..fce5bb9b49 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -51,6 +51,7 @@ from awx.main.utils import ( StubLicense ) from awx.main.utils.db import get_all_field_names +from awx.main.views import ApiErrorView from awx.api.serializers import ResourceAccessListElementSerializer, CopySerializer, UserSerializer from awx.api.versioning import URLPathVersioning from awx.api.metadata import SublistAttachDetatchMetadata, Metadata @@ -188,6 +189,29 @@ class APIView(views.APIView): ''' Log warning for 400 requests. Add header with elapsed time. ''' + + # + # If the URL was rewritten, and we get a 404, we should entirely + # replace the view in the request context with an ApiErrorView() + # Without this change, there will be subtle differences in the BrowseableAPIRenderer + # + # These differences could provide contextual clues which would allow + # anonymous users to determine if usernames were valid or not + # (e.g., if an anonymous user visited `/api/v2/users/valid/`, and got a 404, + # but also saw that the page heading said "User Detail", they might notice + # that's a difference in behavior from a request to `/api/v2/users/not-valid/`, which + # would show a page header of "Not Found"). Changing the view here + # guarantees that the rendered response will look exactly like the response + # when you visit a URL that has no matching URL paths in `awx.api.urls`. + # + if response.status_code == 404 and 'awx.named_url_rewritten' in request.environ: + self.headers.pop('Allow', None) + response = super(APIView, self).finalize_response(request, response, *args, **kwargs) + view = ApiErrorView() + setattr(view, 'request', request) + response.renderer_context['view'] = view + return response + if response.status_code >= 400: status_msg = "status %s received by user %s attempting to access %s from %s" % \ (response.status_code, request.user, request.path, request.META.get('REMOTE_ADDR', None)) diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 77481a3917..f6378f5282 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -14,6 +14,8 @@ import time from base64 import b64encode from collections import OrderedDict +from urllib3.exceptions import ConnectTimeoutError + # Django from django.conf import settings @@ -171,6 +173,15 @@ def api_exception_handler(exc, context): exc = ParseError(exc.args[0]) if isinstance(context['view'], UnifiedJobStdout): context['view'].renderer_classes = [renderers.BrowsableAPIRenderer, JSONRenderer] + if isinstance(exc, APIException): + req = context['request']._request + if 'awx.named_url_rewritten' in req.environ and not str(getattr(exc, 'status_code', 0)).startswith('2'): + # if the URL was rewritten, and it's not a 2xx level status code, + # revert the request.path to its original value to avoid leaking + # any context about the existance of resources + req.path = req.environ['awx.named_url_rewritten'] + if exc.status_code == 403: + exc = NotFound(detail=_('Not found.')) return exception_handler(exc, context) @@ -1397,10 +1408,18 @@ class CredentialExternalTest(SubDetailAPIView): obj.credential_type.plugin.backend(**backend_kwargs) return Response({}, status=status.HTTP_202_ACCEPTED) except requests.exceptions.HTTPError as exc: - message = 'HTTP {}\n{}'.format(exc.response.status_code, exc.response.text) + message = 'HTTP {}'.format(exc.response.status_code) return Response({'inputs': message}, status=status.HTTP_400_BAD_REQUEST) except Exception as exc: - return Response({'inputs': str(exc)}, status=status.HTTP_400_BAD_REQUEST) + message = exc.__class__.__name__ + args = getattr(exc, 'args', []) + for a in args: + if isinstance( + getattr(a, 'reason', None), + ConnectTimeoutError + ): + message = str(a.reason) + return Response({'inputs': message}, status=status.HTTP_400_BAD_REQUEST) class CredentialInputSourceDetail(RetrieveUpdateDestroyAPIView): @@ -1449,10 +1468,18 @@ class CredentialTypeExternalTest(SubDetailAPIView): obj.plugin.backend(**backend_kwargs) return Response({}, status=status.HTTP_202_ACCEPTED) except requests.exceptions.HTTPError as exc: - message = 'HTTP {}\n{}'.format(exc.response.status_code, exc.response.text) + message = 'HTTP {}'.format(exc.response.status_code) return Response({'inputs': message}, status=status.HTTP_400_BAD_REQUEST) except Exception as exc: - return Response({'inputs': str(exc)}, status=status.HTTP_400_BAD_REQUEST) + message = exc.__class__.__name__ + args = getattr(exc, 'args', []) + for a in args: + if isinstance( + getattr(a, 'reason', None), + ConnectTimeoutError + ): + message = str(a.reason) + return Response({'inputs': message}, status=status.HTTP_400_BAD_REQUEST) class HostRelatedSearchMixin(object): diff --git a/awx/main/access.py b/awx/main/access.py index 72f25bc914..4f54be6e12 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -2479,13 +2479,16 @@ class NotificationAccess(BaseAccess): class LabelAccess(BaseAccess): ''' - I can see/use a Label if I have permission to associated organization + I can see/use a Label if I have permission to associated organization, or to a JT that the label is on ''' model = Label prefetch_related = ('modified_by', 'created_by', 'organization',) def filtered_queryset(self): - return self.model.objects.all() + return self.model.objects.filter( + Q(organization__in=Organization.accessible_pk_qs(self.user, 'read_role')) | + Q(unifiedjobtemplate_labels__in=UnifiedJobTemplate.accessible_pk_qs(self.user, 'read_role')) + ) @check_superuser def can_add(self, data): diff --git a/awx/main/credential_plugins/aim.py b/awx/main/credential_plugins/aim.py index 23036efda1..7c99665bf0 100644 --- a/awx/main/credential_plugins/aim.py +++ b/awx/main/credential_plugins/aim.py @@ -1,4 +1,4 @@ -from .plugin import CredentialPlugin, CertFiles +from .plugin import CredentialPlugin, CertFiles, raise_for_status from urllib.parse import quote, urlencode, urljoin @@ -82,8 +82,9 @@ def aim_backend(**kwargs): timeout=30, cert=cert, verify=verify, + allow_redirects=False, ) - res.raise_for_status() + raise_for_status(res) return res.json()['Content'] diff --git a/awx/main/credential_plugins/conjur.py b/awx/main/credential_plugins/conjur.py index 718eebbc64..5cd87007fc 100644 --- a/awx/main/credential_plugins/conjur.py +++ b/awx/main/credential_plugins/conjur.py @@ -1,4 +1,4 @@ -from .plugin import CredentialPlugin, CertFiles +from .plugin import CredentialPlugin, CertFiles, raise_for_status import base64 from urllib.parse import urljoin, quote @@ -58,7 +58,8 @@ def conjur_backend(**kwargs): auth_kwargs = { 'headers': {'Content-Type': 'text/plain'}, - 'data': api_key + 'data': api_key, + 'allow_redirects': False, } with CertFiles(cacert) as cert: @@ -68,11 +69,12 @@ def conjur_backend(**kwargs): urljoin(url, '/'.join(['authn', account, username, 'authenticate'])), **auth_kwargs ) - resp.raise_for_status() + raise_for_status(resp) token = base64.b64encode(resp.content).decode('utf-8') lookup_kwargs = { 'headers': {'Authorization': 'Token token="{}"'.format(token)}, + 'allow_redirects': False, } # https://www.conjur.org/api.html#secrets-retrieve-a-secret-get @@ -88,7 +90,7 @@ def conjur_backend(**kwargs): with CertFiles(cacert) as cert: lookup_kwargs['verify'] = cert resp = requests.get(path, timeout=30, **lookup_kwargs) - resp.raise_for_status() + raise_for_status(resp) return resp.text diff --git a/awx/main/credential_plugins/hashivault.py b/awx/main/credential_plugins/hashivault.py index 6e033efb43..2406623231 100644 --- a/awx/main/credential_plugins/hashivault.py +++ b/awx/main/credential_plugins/hashivault.py @@ -3,7 +3,7 @@ import os import pathlib from urllib.parse import urljoin -from .plugin import CredentialPlugin, CertFiles +from .plugin import CredentialPlugin, CertFiles, raise_for_status import requests from django.utils.translation import ugettext_lazy as _ @@ -145,7 +145,10 @@ def kv_backend(**kwargs): cacert = kwargs.get('cacert', None) api_version = kwargs['api_version'] - request_kwargs = {'timeout': 30} + request_kwargs = { + 'timeout': 30, + 'allow_redirects': False, + } sess = requests.Session() sess.headers['Authorization'] = 'Bearer {}'.format(token) @@ -175,7 +178,7 @@ def kv_backend(**kwargs): with CertFiles(cacert) as cert: request_kwargs['verify'] = cert response = sess.get(request_url, **request_kwargs) - response.raise_for_status() + raise_for_status(response) json = response.json() if api_version == 'v2': @@ -198,7 +201,10 @@ def ssh_backend(**kwargs): role = kwargs['role'] cacert = kwargs.get('cacert', None) - request_kwargs = {'timeout': 30} + request_kwargs = { + 'timeout': 30, + 'allow_redirects': False, + } request_kwargs['json'] = {'public_key': kwargs['public_key']} if kwargs.get('valid_principals'): @@ -215,7 +221,7 @@ def ssh_backend(**kwargs): request_kwargs['verify'] = cert resp = sess.post(request_url, **request_kwargs) - resp.raise_for_status() + raise_for_status(resp) return resp.json()['data']['signed_key'] diff --git a/awx/main/credential_plugins/plugin.py b/awx/main/credential_plugins/plugin.py index def2676a02..fa5c770fd1 100644 --- a/awx/main/credential_plugins/plugin.py +++ b/awx/main/credential_plugins/plugin.py @@ -3,9 +3,19 @@ import tempfile from collections import namedtuple +from requests.exceptions import HTTPError + CredentialPlugin = namedtuple('CredentialPlugin', ['name', 'inputs', 'backend']) +def raise_for_status(resp): + resp.raise_for_status() + if resp.status_code >= 300: + exc = HTTPError() + setattr(exc, 'response', resp) + raise exc + + class CertFiles(): """ A context manager used for writing a certificate and (optional) key diff --git a/awx/main/middleware.py b/awx/main/middleware.py index 112ae17aa5..781266e8dd 100644 --- a/awx/main/middleware.py +++ b/awx/main/middleware.py @@ -14,7 +14,7 @@ from django.conf import settings from django.contrib.auth.models import User from django.db.migrations.executor import MigrationExecutor from django.db import connection -from django.shortcuts import get_object_or_404, redirect +from django.shortcuts import redirect from django.apps import apps from django.utils.deprecation import MiddlewareMixin from django.utils.translation import ugettext_lazy as _ @@ -148,7 +148,21 @@ class URLModificationMiddleware(MiddlewareMixin): def _named_url_to_pk(cls, node, resource, named_url): kwargs = {} if node.populate_named_url_query_kwargs(kwargs, named_url): - return str(get_object_or_404(node.model, **kwargs).pk) + match = node.model.objects.filter(**kwargs).first() + if match: + return str(match.pk) + else: + # if the name does *not* resolve to any actual resource, + # we should still attempt to route it through so that 401s are + # respected + # using "zero" here will cause the URL regex to match e.g., + # /api/v2/users//, but it also means that anonymous + # users will go down the path of having their credentials + # verified; in this way, *anonymous* users will that visit + # /api/v2/users/invalid-username/ *won't* see a 404, they'll + # see a 401 as if they'd gone to /api/v2/users/0/ + # + return '0' if resource == 'job_templates' and '++' not in named_url: # special case for deprecated job template case # will not raise a 404 on its own @@ -178,6 +192,7 @@ class URLModificationMiddleware(MiddlewareMixin): old_path = request.path_info new_path = self._convert_named_url(old_path) if request.path_info != new_path: + request.environ['awx.named_url_rewritten'] = request.path request.path = request.path.replace(request.path_info, new_path) request.path_info = new_path diff --git a/awx/main/notifications/grafana_backend.py b/awx/main/notifications/grafana_backend.py index 28fc1b90b3..8e8b648952 100644 --- a/awx/main/notifications/grafana_backend.py +++ b/awx/main/notifications/grafana_backend.py @@ -94,8 +94,8 @@ class GrafanaBackend(AWXBaseEmailBackend, CustomNotificationBase): headers=grafana_headers, verify=(not self.grafana_no_verify_ssl)) if r.status_code >= 400: - logger.error(smart_text(_("Error sending notification grafana: {}").format(r.text))) + logger.error(smart_text(_("Error sending notification grafana: {}").format(r.status_code))) if not self.fail_silently: - raise Exception(smart_text(_("Error sending notification grafana: {}").format(r.text))) + raise Exception(smart_text(_("Error sending notification grafana: {}").format(r.status_code))) sent_messages += 1 return sent_messages diff --git a/awx/main/notifications/mattermost_backend.py b/awx/main/notifications/mattermost_backend.py index 78a23c72d1..59a1c6f5e1 100644 --- a/awx/main/notifications/mattermost_backend.py +++ b/awx/main/notifications/mattermost_backend.py @@ -46,8 +46,8 @@ class MattermostBackend(AWXBaseEmailBackend, CustomNotificationBase): r = requests.post("{}".format(m.recipients()[0]), json=payload, verify=(not self.mattermost_no_verify_ssl)) if r.status_code >= 400: - logger.error(smart_text(_("Error sending notification mattermost: {}").format(r.text))) + logger.error(smart_text(_("Error sending notification mattermost: {}").format(r.status_code))) if not self.fail_silently: - raise Exception(smart_text(_("Error sending notification mattermost: {}").format(r.text))) + raise Exception(smart_text(_("Error sending notification mattermost: {}").format(r.status_code))) sent_messages += 1 return sent_messages diff --git a/awx/main/notifications/rocketchat_backend.py b/awx/main/notifications/rocketchat_backend.py index 1ad367fb57..df271bf80d 100644 --- a/awx/main/notifications/rocketchat_backend.py +++ b/awx/main/notifications/rocketchat_backend.py @@ -46,9 +46,9 @@ class RocketChatBackend(AWXBaseEmailBackend, CustomNotificationBase): if r.status_code >= 400: logger.error(smart_text( - _("Error sending notification rocket.chat: {}").format(r.text))) + _("Error sending notification rocket.chat: {}").format(r.status_code))) if not self.fail_silently: raise Exception(smart_text( - _("Error sending notification rocket.chat: {}").format(r.text))) + _("Error sending notification rocket.chat: {}").format(r.status_code))) sent_messages += 1 return sent_messages diff --git a/awx/main/notifications/webhook_backend.py b/awx/main/notifications/webhook_backend.py index b9c2c35d22..a33cf026f8 100644 --- a/awx/main/notifications/webhook_backend.py +++ b/awx/main/notifications/webhook_backend.py @@ -72,8 +72,8 @@ class WebhookBackend(AWXBaseEmailBackend, CustomNotificationBase): headers=self.headers, verify=(not self.disable_ssl_verification)) if r.status_code >= 400: - logger.error(smart_text(_("Error sending notification webhook: {}").format(r.text))) + logger.error(smart_text(_("Error sending notification webhook: {}").format(r.status_code))) if not self.fail_silently: - raise Exception(smart_text(_("Error sending notification webhook: {}").format(r.text))) + raise Exception(smart_text(_("Error sending notification webhook: {}").format(r.status_code))) sent_messages += 1 return sent_messages diff --git a/awx/main/tests/functional/test_named_url.py b/awx/main/tests/functional/test_named_url.py index dcf2111992..6482dac3a8 100644 --- a/awx/main/tests/functional/test_named_url.py +++ b/awx/main/tests/functional/test_named_url.py @@ -219,3 +219,27 @@ def test_credential(get, admin_user, credentialtype_ssh): url = reverse('api:credential_detail', kwargs={'pk': test_cred.pk}) response = get(url, user=admin_user, expect=200) assert response.data['related']['named_url'].endswith('/test_cred++Machine+ssh++/') + + +@pytest.mark.django_db +def test_403_vs_404(get): + cindy = User.objects.create( + username='cindy', + password='test_user', + is_superuser=False + ) + bob = User.objects.create( + username='bob', + password='test_user', + is_superuser=False + ) + + # bob cannot see cindy, pk lookup should be a 403 + url = reverse('api:user_detail', kwargs={'pk': cindy.pk}) + get(url, user=bob, expect=403) + + # bob cannot see cindy, username lookup should be a 404 + get('/api/v2/users/cindy/', user=bob, expect=404) + + get(f'/api/v2/users/{cindy.pk}/', expect=401) + get('/api/v2/users/cindy/', expect=404) diff --git a/awx/main/tests/functional/test_rbac_label.py b/awx/main/tests/functional/test_rbac_label.py index 955894c06f..ed819df9f0 100644 --- a/awx/main/tests/functional/test_rbac_label.py +++ b/awx/main/tests/functional/test_rbac_label.py @@ -20,8 +20,19 @@ def test_label_get_queryset_su(label, user): @pytest.mark.django_db -def test_label_access(label, user): +def test_label_read_access(label, user): access = LabelAccess(user('user', False)) + assert not access.can_read(label) + label.organization.member_role.members.add(user('user', False)) + assert access.can_read(label) + + +@pytest.mark.django_db +def test_label_jt_read_access(label, user, job_template): + access = LabelAccess(user('user', False)) + assert not access.can_read(label) + job_template.read_role.members.add(user('user', False)) + job_template.labels.add(label) assert access.can_read(label)