diff --git a/openwisp_controller/config/controller/views.py b/openwisp_controller/config/controller/views.py index 2a852e3d2..994991428 100644 --- a/openwisp_controller/config/controller/views.py +++ b/openwisp_controller/config/controller/views.py @@ -5,7 +5,7 @@ from cache_memoize import cache_memoize from django.core.cache import cache -from django.core.exceptions import FieldDoesNotExist, ValidationError +from django.core.exceptions import ValidationError from django.db import transaction from django.db.models import Q from django.utils.decorators import method_decorator @@ -304,30 +304,38 @@ class DeviceRegisterView(UpdateLastIpMixin, CsrfExtemptMixin, View): UPDATABLE_FIELDS = ["name", "os", "model", "system"] + # device fields a device is allowed to set during self-registration; every + # other model field (``id``, ``key`` unless consistent registration is + # enabled, ``last_ip``, ``_is_deactivated``, ``group``, ``created``, + # ``modified``, ...) is set by the server or left at its default and must + # not be mass-assigned from the request POST data + REGISTRATION_FIELDS = [ + "name", + "mac_address", + "hardware_id", + "os", + "model", + "system", + "management_ip", + ] + def init_object(self, **kwargs): """ initializes Config object with incoming POST data """ device_model = self.model config_model = device_model.get_config_model() - options = {} - for attr in kwargs.keys(): - if attr in ["organization", "organization_id"]: - continue - # skip attributes that are not model fields - try: - device_model._meta.get_field(attr) - except FieldDoesNotExist: - continue - options[attr] = kwargs.get(attr) - # do not specify key if: - # app_settings.CONSISTENT_REGISTRATION is False - # if key is ``None`` (it would cause exception) - if "key" in options and ( - app_settings.CONSISTENT_REGISTRATION is False or options["key"] is None - ): - del options["key"] - if "hardware_id" in options and options["hardware_id"] == "": + # only accept an explicit allowlist of fields from the request, to + # avoid mass-assignment of internal fields (e.g. ``id``) + options = { + attr: kwargs[attr] for attr in self.REGISTRATION_FIELDS if attr in kwargs + } + # accept the device supplied key only if: + # app_settings.CONSISTENT_REGISTRATION is True + # and the key is not ``None`` (it would cause an exception) + if app_settings.CONSISTENT_REGISTRATION and kwargs.get("key") is not None: + options["key"] = kwargs["key"] + if options.get("hardware_id") == "": options["hardware_id"] = None config = config_model(device=device_model(**options), backend=kwargs["backend"]) config.organization = self.organization diff --git a/openwisp_controller/config/tests/test_controller.py b/openwisp_controller/config/tests/test_controller.py index 2fb85fe2a..2ddf179c4 100644 --- a/openwisp_controller/config/tests/test_controller.py +++ b/openwisp_controller/config/tests/test_controller.py @@ -394,6 +394,14 @@ def test_register_with_org_param(self): self.assertEqual(device.organization, org1) self.assertNotEqual(device.organization, org2) + def test_register_ignores_disallowed_fields(self): + # a registrant must not be able to mass-assign internal device fields + # such as the primary key: choosing an arbitrary UUID would let a + # registrant probe for cross-organization device existence + forged_id = "0" * 32 + device = self.test_register(id=forged_id) + self.assertNotEqual(device.pk.hex, forged_id) + def test_register_exceeds_org_device_limit(self): org = self._get_org() org.config_limits.device_limit = 1