mirror of
https://github.com/inventree/InvenTree.git
synced 2026-09-10 06:37:17 +00:00
* Fix security hole for metadata API
* Regression test for field injection
(cherry picked from commit 694d890ce5)
Co-authored-by: Oliver <oliver.henry.walters@gmail.com>
This commit is contained in:
co-authored by
Oliver
parent
2ddc615ca2
commit
1c4cfd8a47
@@ -877,6 +877,9 @@ class GenericMetadataView(RetrieveUpdateAPI):
|
|||||||
serializer_class = MetadataSerializer
|
serializer_class = MetadataSerializer
|
||||||
permission_classes = [InvenTree.permissions.ContentTypePermission]
|
permission_classes = [InvenTree.permissions.ContentTypePermission]
|
||||||
|
|
||||||
|
# Enforce limited range of lookup fields to prevent arbitrary queryset filtering
|
||||||
|
ALLOWED_LOOKUP_FIELDS = {'pk', 'key'}
|
||||||
|
|
||||||
def get_permission_model(self):
|
def get_permission_model(self):
|
||||||
"""Return the 'permission' model associated with this view."""
|
"""Return the 'permission' model associated with this view."""
|
||||||
model_name = self.kwargs.get('model', None)
|
model_name = self.kwargs.get('model', None)
|
||||||
@@ -921,6 +924,17 @@ class GenericMetadataView(RetrieveUpdateAPI):
|
|||||||
)
|
)
|
||||||
return super().dispatch(request, *args, **kwargs)
|
return super().dispatch(request, *args, **kwargs)
|
||||||
|
|
||||||
|
def initial(self, request, *args, **kwargs):
|
||||||
|
"""Validate the lookup field before any queryset is touched.
|
||||||
|
|
||||||
|
This runs inside APIView's own exception handling (unlike dispatch(),
|
||||||
|
which runs before it), and *before* permission checks - so an invalid
|
||||||
|
lookup field is rejected without ever executing a query.
|
||||||
|
"""
|
||||||
|
if self.lookup_field not in self.ALLOWED_LOOKUP_FIELDS:
|
||||||
|
raise ValidationError(f"Invalid lookup field '{self.lookup_field}'")
|
||||||
|
return super().initial(request, *args, **kwargs)
|
||||||
|
|
||||||
|
|
||||||
class SimpleGenericMetadataView(GenericMetadataView):
|
class SimpleGenericMetadataView(GenericMetadataView):
|
||||||
"""Simplified version of GenericMetadataView which always uses 'pk' as the lookup field."""
|
"""Simplified version of GenericMetadataView which always uses 'pk' as the lookup field."""
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ from base64 import b64encode
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from tempfile import TemporaryDirectory
|
from tempfile import TemporaryDirectory
|
||||||
|
|
||||||
|
from django.contrib.auth import get_user_model
|
||||||
from django.core.exceptions import AppRegistryNotReady
|
from django.core.exceptions import AppRegistryNotReady
|
||||||
from django.test import TestCase
|
from django.test import TestCase
|
||||||
from django.urls import reverse
|
from django.urls import reverse
|
||||||
@@ -15,6 +16,7 @@ from InvenTree.api_version import INVENTREE_API_VERSION
|
|||||||
from InvenTree.exceptions import exception_handler
|
from InvenTree.exceptions import exception_handler
|
||||||
from InvenTree.unit_test import InvenTreeAPITestCase, InvenTreeTestCase
|
from InvenTree.unit_test import InvenTreeAPITestCase, InvenTreeTestCase
|
||||||
from InvenTree.version import inventreeApiText, parse_version_text
|
from InvenTree.version import inventreeApiText, parse_version_text
|
||||||
|
from users.models import ApiToken
|
||||||
from users.ruleset import RULESET_NAMES
|
from users.ruleset import RULESET_NAMES
|
||||||
from users.tasks import update_group_roles
|
from users.tasks import update_group_roles
|
||||||
|
|
||||||
@@ -658,3 +660,56 @@ class GeneralApiTests(InvenTreeAPITestCase):
|
|||||||
self.assertIn('bom-exporter', keys)
|
self.assertIn('bom-exporter', keys)
|
||||||
self.assertIn('inventree-ui-notification', keys)
|
self.assertIn('inventree-ui-notification', keys)
|
||||||
self.assertIn('inventreelabel', keys)
|
self.assertIn('inventreelabel', keys)
|
||||||
|
|
||||||
|
def test_generic_metadata_lookup_field_injection(self):
|
||||||
|
"""The generic metadata endpoint must not accept arbitrary lookup expressions.
|
||||||
|
|
||||||
|
Regression test for a vulnerability where 'lookup_field' was passed
|
||||||
|
straight from the URL into the ORM (e.g. 'key__regex'), letting an
|
||||||
|
unprivileged user turn distinguishable 403 / 404 / 500 responses into
|
||||||
|
an oracle to recover another user's raw API token, byte by byte.
|
||||||
|
"""
|
||||||
|
victim = get_user_model().objects.create_user(
|
||||||
|
username='metadata_victim', password='hunter2', email='victim@example.org'
|
||||||
|
)
|
||||||
|
token = ApiToken.objects.create(user=victim)
|
||||||
|
|
||||||
|
def lookup_url(model, lookup_field, lookup_value):
|
||||||
|
return reverse(
|
||||||
|
'api-generic-metadata',
|
||||||
|
kwargs={
|
||||||
|
'model': model,
|
||||||
|
'lookup_field': lookup_field,
|
||||||
|
'lookup_value': lookup_value,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
# A pattern which *would* match the victim's token if 'key__regex'
|
||||||
|
# were actually applied as a regex filter against ApiToken.key
|
||||||
|
matching_pattern = f'^{token.key}$'
|
||||||
|
non_matching_pattern = '^this-will-never-match-anything$'
|
||||||
|
|
||||||
|
responses = set()
|
||||||
|
|
||||||
|
for pattern in (matching_pattern, non_matching_pattern):
|
||||||
|
for lookup_field in ('key__regex', 'key__contains', 'key__startswith'):
|
||||||
|
response = self.get(
|
||||||
|
lookup_url('apitoken', lookup_field, pattern), expected_code=400
|
||||||
|
)
|
||||||
|
self.assertIn('Invalid lookup field', str(response.data))
|
||||||
|
responses.add(response.status_code)
|
||||||
|
|
||||||
|
# Matching and non-matching patterns must be indistinguishable
|
||||||
|
self.assertEqual(len(responses), 1)
|
||||||
|
|
||||||
|
# The exact-match 'key' lookup stays permitted (used elsewhere, e.g.
|
||||||
|
# by plugin config lookups) - a non-admin still can't read another
|
||||||
|
# user's token metadata through it, since object permissions apply.
|
||||||
|
self.get(lookup_url('apitoken', 'key', token.key), expected_code=403)
|
||||||
|
|
||||||
|
# Not apitoken-specific: any model/lookup-type combination outside
|
||||||
|
# the allow-list is rejected the same way.
|
||||||
|
response = self.get(
|
||||||
|
lookup_url('user', 'password__startswith', 'x'), expected_code=400
|
||||||
|
)
|
||||||
|
self.assertIn('Invalid lookup field', str(response.data))
|
||||||
|
|||||||
Reference in New Issue
Block a user