Merge pull request from GHSA-7j9p-67mm-5g87

* fix: Tool can only push grade to value in config

Before this commit, LTI tools were able to push grades to any block
simply by modifying or creating a new line item with a `resource_link_id` containing a valid block.

This commit closes that loophole and resolves
security advisory GHSA-7j9p-67mm-5g87.

* chore: create release version

Co-authored-by: Zach Hancock <zhancock@edx.org>

CHANGELOG.rst

@@ -16,6 +16,10 @@ Please See the [releases tab](https://github.com/openedx/xblock-lti-consumer/rel
 Unreleased
 ~~~~~~~~~~
 
+7.2.2 - 2023-01-12
+------------------
+* Fixes LTI 1.3 grade injection vulnerability that allowed LTI integrations to modify scores for any block.
+
 7.2.1 - 2023-01-10
 ------------------
 * Adds support for LTI_BASE and LTI_API_BASE Django settings to allow URL configuration independent of LMS settings.

lti_consumer/__init__.py

@@ -4,4 +4,4 @@ Runtime will load the XBlock class from here.
 from .apps import LTIConsumerApp
 from .lti_xblock import LtiConsumerXBlock
 
-__version__ = '7.2.1'
+__version__ = '7.2.2'

lti_consumer/signals/signals.py

@@ -22,16 +22,34 @@ def publish_grade_on_score_update(sender, instance, **kwargs):  # pylint: disabl
     in the LMS. Trying to trigger this signal from Studio (from the Django-admin interface, for example)
     throw an exception.
     """
+    line_item = instance.line_item
+    lti_config = line_item.lti_configuration
+
+    # Only save score if the `line_item.resource_link_id` is the same as
+    # `lti_configuration.location` to prevent LTI tools to alter grades they don't
+    # have permissions to.
+    # TODO: This security mechanism will need to be reworked once we enable LTI 1.3
+    # reusability to allow one configuration to save scores on multiple placements,
+    # but still locking down access to the items that are using the LTI configurtion.
+    if line_item.resource_link_id != lti_config.location:
+        log.warning(
+            "LTI tool tried publishing score %r to block %s (outside allowed scope of: %s).",
+            instance,
+            line_item.resource_link_id,
+            lti_config.location,
+        )
+        return
+
     # Before starting to publish grades to the LMS, check that:
     # 1. The grade being submitted in the final one - `FullyGraded`
     # 2. This LineItem is linked to a LMS grade - the `LtiResouceLinkId` field is set
     # 3. There's a valid grade in this score - `scoreGiven` is set
     if instance.grading_progress == LtiAgsScore.FULLY_GRADED \
-            and instance.line_item.resource_link_id \
+            and line_item.resource_link_id \
             and instance.score_given:
         try:
             # Load block using LMS APIs and check if the block is graded and still accept grades.
-            block = compat.load_block_as_user(instance.line_item.resource_link_id)
+            block = compat.load_block_as_user(line_item.resource_link_id)
             if block.has_score and (not block.is_past_due() or block.accept_grades_past_due):
                 # Map external ID to platform user
                 user = compat.get_user_from_external_user_id(instance.user_id)
@@ -58,7 +76,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs):  # pylint: disabl
             log.exception(
                 "Error while publishing score %r to block %s to LMS: %s",
                 instance,
-                instance.line_item.resource_link_id,
+                line_item.resource_link_id,
                 exc,
             )
             raise exc

lti_consumer/tests/unit/test_signals.py

+"""
+Tests for LTI Advantage Assignments and Grades Service views.
+"""
+from datetime import datetime
+from unittest.mock import patch, Mock
+
+from django.test import TestCase
+from opaque_keys.edx.keys import UsageKey
+
+from lti_consumer.models import LtiConfiguration, LtiAgsLineItem, LtiAgsScore
+
+
+class PublishGradeOnScoreUpdateTest(TestCase):
+    """
+    Test the `publish_grade_on_score_update` signal.
+    """
+
+    def setUp(self):
+        """
+        Set up resources for signal testing.
+        """
+        self.location = UsageKey.from_string(
+            "block-v1:course+test+2020+type@problem+block@test"
+        )
+
+        # Create configuration
+        self.lti_config = LtiConfiguration.objects.create(
+            location=self.location,
+            version=LtiConfiguration.LTI_1P3,
+        )
+
+        # Patch internal method to avoid calls to modulestore
+        self._block_mock = Mock()
+        compat_mock = patch("lti_consumer.signals.signals.compat")
+        self.addCleanup(compat_mock.stop)
+        self._compat_mock = compat_mock.start()
+        self._compat_mock.get_user_from_external_user_id.return_value = Mock()
+        self._compat_mock.load_block_as_user.return_value = self._block_mock
+
+    def test_grade_publish_not_done_when_wrong_line_item(self):
+        """
+        Test grade publish after for a different UsageKey than set on
+        `lti_config.location`.
+        """
+        # Create LineItem with `resource_link_id` != `lti_config.id`
+        line_item = LtiAgsLineItem.objects.create(
+            lti_configuration=self.lti_config,
+            resource_id="test",
+            resource_link_id=UsageKey.from_string(
+                "block-v1:course+test+2020+type@problem+block@different"
+            ),
+            label="test label",
+            score_maximum=100
+        )
+
+        # Save score and check that LMS method wasn't called.
+        LtiAgsScore.objects.create(
+            line_item=line_item,
+            score_given=1,
+            score_maximum=1,
+            activity_progress=LtiAgsScore.COMPLETED,
+            grading_progress=LtiAgsScore.FULLY_GRADED,
+            user_id="test",
+            timestamp=datetime.now(),
+        )
+
+        # Check that methods to save grades are not called
+        self._block_mock.set_user_module_score.assert_not_called()
+        self._compat_mock.get_user_from_external_user_id.assert_not_called()
+        self._compat_mock.load_block_as_user.assert_not_called()
+
+    def test_grade_publish(self):
+        """
+        Test grade publish after if the UsageKey is equal to
+        the one on `lti_config.location`.
+        """
+        # Create LineItem with `resource_link_id` != `lti_config.id`
+        line_item = LtiAgsLineItem.objects.create(
+            lti_configuration=self.lti_config,
+            resource_id="test",
+            resource_link_id=self.location,
+            label="test label",
+            score_maximum=100
+        )
+
+        # Save score and check that LMS method wasn't called.
+        LtiAgsScore.objects.create(
+            line_item=line_item,
+            score_given=1,
+            score_maximum=1,
+            activity_progress=LtiAgsScore.COMPLETED,
+            grading_progress=LtiAgsScore.FULLY_GRADED,
+            user_id="test",
+            timestamp=datetime.now(),
+        )
+
+        # Check that methods to save grades are called
+        self._block_mock.set_user_module_score.assert_called_once()
+        self._compat_mock.get_user_from_external_user_id.assert_called_once()
+        self._compat_mock.load_block_as_user.assert_called_once()