Merge pull request #4464 from ryanpetrello/named-url-vuln-372

update the named URL code to properly return 404 vs 403

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))

awx/api/views/__init__.py

@@ -171,6 +171,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)
 
 

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/<integer>/, 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
 

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)