[Security] Changing related objects

[regqueryvalueex]

As mentioned here - #83, this package have a security issues, however original issue does not list them all

I created a simple repo with examples of issues - https://github.com/regqueryvalueex/nested-serializer-issues-example-, so pls consider this, because this is very important. You can find tests, that illustrates issues here - https://github.com/regqueryvalueex/nested-serializer-issues-example-/tree/master/example/tests

I'll describe it as well here and propose a solution.

models.py

from django.db import models


class Meeting(models.Model):
    title = models.CharField(max_length=200)
    time = models.DateTimeField(null=True, blank=True)


class Comment(models.Model):
    meeting = models.ForeignKey('Meeting', on_delete=models.CASCADE, related_name='comments')
    description = models.TextField(max_length=3000)

conftest.py

import pytest
from model_bakery import baker

pytestmark = [pytest.mark.django_db]


@pytest.fixture
def populated_meeting():
    meeting = baker.make('meetings.Meeting')
    baker.make(
        'meetings.Comment',
        meeting=meeting
    )
    return meeting

Example of direct relations changing

direct_relation_serializers.py

from rest_framework import serializers
from drf_writable_nested.serializers import WritableNestedModelSerializer

from meetings.models import Meeting, Comment


class MeetingSerializer(serializers.ModelSerializer):

    class Meta:
        model = Meeting
        fields = (
            'pk',
            'title',
        )


class CommentSerializer(WritableNestedModelSerializer):
    meeting = MeetingSerializer()

    class Meta:
        model = Comment
        fields = (
            'pk',
            'description',
            'meeting',
        )

test_updating_parent_instance.py

import pytest

from api.meetings.direct_relation_serializers import CommentSerializer

pytestmark = [pytest.mark.django_db]


def test_update_meeting_via_comment(populated_meeting):
    old_comment = populated_meeting.comments.get()

    assert populated_meeting.comments.count() == 1

    serializer = CommentSerializer(
        instance=old_comment,
        data={
            'description': 'editing meeting title',
            'meeting': {
                'pk': populated_meeting.pk,
                'title': 'Changed!'
            }
        }
    )

    assert serializer.is_valid()
    serializer.save()

    populated_meeting.refresh_from_db()
    assert populated_meeting.title == 'Changed!'


def test_create_new_meeting_via_comment(populated_meeting):
    old_comment = populated_meeting.comments.get()
    assert populated_meeting.comments.count() == 1

    serializer = CommentSerializer(
        instance=old_comment,
        data={
            'description': 'editing meeting title',
            'meeting': {
                'title': 'New one!'
            }
        }
    )

    assert serializer.is_valid()
    comment = serializer.save()

    populated_meeting.refresh_from_db()

    assert populated_meeting.title != 'New one!'
    assert populated_meeting.comments.count() == 0

    assert comment.meeting.id != populated_meeting.id


def test_steel_other_meeting_via_comment(populated_meeting):
    assert populated_meeting.comments.count() == 1

    serializer = CommentSerializer(
        data={
            'description': 'editing meeting title',
            'meeting': {
                'pk': populated_meeting.pk,
                'title': 'Stolen meeting!'
            }
        }
    )

    assert serializer.is_valid()
    comment = serializer.save()

    populated_meeting.refresh_from_db()

    assert populated_meeting.title == 'Stolen meeting!'
    assert populated_meeting.comments.count() == 2

    assert comment.meeting.id == populated_meeting.id

Issue here - it's possible via serializer for comment update parent meeting, also it's possible to change meeting that is related to other comment if you just pass pk of the other meeting. There should be a validation, that ensure, that comment belongs to that meeting. Personally i believe, that modifying objects via direct relations is bad idea by itself, because, generally you want to do it the other way around, but it seems like it intended feature.

Example of reversed relations changing

reversed_relation_serializers.py

from rest_framework import serializers
from drf_writable_nested.serializers import WritableNestedModelSerializer

from meetings.models import Meeting, Comment


class CommentSerializer(serializers.ModelSerializer):

    class Meta:
        model = Comment
        fields = (
            'pk',
            'description',
        )


class MeetingSerializer(WritableNestedModelSerializer):
    comments = CommentSerializer(many=True)

    class Meta:
        model = Meeting
        fields = (
            'pk',
            'title',
            'comments',
        )

test_reassigning_relations.py

import pytest

from api.meetings.reversed_relation_serializers import MeetingSerializer

pytestmark = [pytest.mark.django_db]


def test_steal_comment(populated_meeting):
    old_comment = populated_meeting.comments.get()

    assert populated_meeting.comments.count() == 1

    serializer = MeetingSerializer(
        data={
            'title': 'Hello',
            'comments': [
                {
                    'description': 'Stolen!',
                    'pk': old_comment.pk,
                },
                {
                    'description': 'got bored again',
                },
            ]
        }
    )

    assert serializer.is_valid()
    new_meeting = serializer.save()

    assert new_meeting.title == 'Hello'
    assert new_meeting.comments.count() == 2

    assert populated_meeting.comments.count() == 0  # 'Oh no, comment now is stolen'
    old_comment.refresh_from_db()
    assert old_comment.meeting == new_meeting

This is the exact issue, mentioned here - #83

It's possible to steel comments from other meetings just by passing pk from existing comment. This also should be validation, that comment belongs to the meeting

pls also consider validation for M2M fields.

Proposal: Add the validation, so package wont automatically reassign relations. This should be default behaviour, since most of the people, who use it will use it on default settings, because they are unaware of such issues, so their project will have security vulnerabilities

[ir4y]

Hi @regqueryvalueex
Thank you for the example provided. We will think about the fix and will let you know.
A pull request with the fix will be appreciated.

[ir4y]

Also, it will help a lot of you can create a pull request with the set of tests that describe the bug.
May I kindly ask you to do it @regqueryvalueex ?
The sample project you have provided is great, but a pull request with tests will be more useful.

[felipediel]

I came across this issue recently and I've been thinking of a way to fix it. The proposed solution seems reasonable. Another option is to add a filter_queryset() method that excludes from updates by default child instances that are not related to parent.