Improve order secret handling (#4139)

- use hmac.compare_digest for all secret comparisons
- use salted_hmac with sha256 instead of plain sha1 for hashed secrets
- move secret handling into helper functions

src/pretix/base/migrations/0264_order_internal_secret.py

@@ -0,0 +1,24 @@
+# Generated by Django 4.2.11 on 2024-05-16 11:07
+
+from django.db import migrations, models
+
+import pretix.base.models.orders
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("pretixbase", "0263_auto_20240409_0732"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="order",
+            name="internal_secret",
+            field=models.CharField(
+                default=None,
+                max_length=32,
+                null=True,
+            ),
+        ),
+    ]

src/pretix/base/models/orders.py

@@ -35,6 +35,7 @@
 
 import copy
 import hashlib
+import hmac
 import json
 import logging
 import operator
@@ -59,7 +60,7 @@ from django.db.models.functions import Coalesce, Greatest
 from django.db.models.signals import post_delete
 from django.dispatch import receiver
 from django.urls import reverse
-from django.utils.crypto import get_random_string
+from django.utils.crypto import get_random_string, salted_hmac
 from django.utils.encoding import escape_uri_path
 from django.utils.formats import date_format
 from django.utils.functional import cached_property
@@ -105,6 +106,35 @@ def generate_position_secret():
     raise TypeError("Function no longer exists, use secret generators")
 
 
+class OrderQuerySet(models.QuerySet):
+    def get_with_secret_check(self, code, received_secret, tag, secret_length=64):
+        dummy = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"[:secret_length]
+        try:
+            order = self.get(code=code)
+            if not hmac.compare_digest(
+                order.tagged_secret(tag, secret_length) if tag else order.secret,
+                received_secret[:secret_length].lower() if tag else received_secret.lower()
+            ) and not (
+                    # TODO: remove this clause after a while (compatibility with old secrets currently in flight)
+                    tag and hmac.compare_digest(
+                        hashlib.sha1(order.secret.lower().encode()).hexdigest(),
+                        received_secret.lower()
+                    )
+            ):
+                raise Order.DoesNotExist
+            return order
+        except Order.DoesNotExist:
+            # Do a hash comparison as well to harden against timing attacks
+            if hmac.compare_digest(
+                salted_hmac(key_salt=b"", value=tag, algorithm="sha256",
+                            secret=dummy).hexdigest()[:secret_length],
+                received_secret[:secret_length]
+            ):
+                raise Order.DoesNotExist
+            else:
+                raise Order.DoesNotExist
+
+
 class Order(LockModel, LoggedModel):
     """
     An order is created when a user clicks 'buy' on his cart. It holds
@@ -223,6 +253,7 @@ class Order(LockModel, LoggedModel):
         verbose_name=_('Locale')
     )
     secret = models.CharField(max_length=32, default=generate_secret)
+    internal_secret = models.CharField(null=True, blank=True, max_length=32, default=generate_secret)
     datetime = models.DateTimeField(
         verbose_name=_("Date"), db_index=False
     )
@@ -285,7 +316,7 @@ class Order(LockModel, LoggedModel):
         default=False,
     )
 
-    objects = ScopedManager(organizer='event__organizer')
+    objects = ScopedManager(OrderQuerySet.as_manager().__class__, organizer='event__organizer')
 
     class Meta:
         verbose_name = _("Order")
@@ -1223,6 +1254,10 @@ class Order(LockModel, LoggedModel):
         _transactions_mark_order_clean(self.pk)
         return create
 
+    def tagged_secret(self, tag, secret_length=64):
+        return salted_hmac(value=tag, key_salt=b"", algorithm="sha256",
+                           secret=self.internal_secret or self.secret).hexdigest()[:secret_length]
+
 
 def answerfile_name(instance, filename: str) -> str:
     secret = get_random_string(length=32, allowed_chars=string.ascii_letters + string.digits)

src/pretix/plugins/paypal2/payment.py

@@ -19,7 +19,6 @@
 # You should have received a copy of the GNU Affero General Public License along with this program.  If not, see
 # <https://www.gnu.org/licenses/>.
 #
-import hashlib
 import json
 import logging
 import urllib.parse
@@ -1096,5 +1095,5 @@ class PaypalAPM(PaypalMethod):
         return eventreverse(self.event, 'plugins:paypal2:pay', kwargs={
             'order': payment.order.code,
             'payment': payment.pk,
-            'hash': hashlib.sha1(payment.order.secret.lower().encode()).hexdigest(),
+            'hash': payment.order.tagged_secret('plugins:paypal2:pay'),
         })

src/pretix/plugins/paypal2/views.py

@@ -31,7 +31,6 @@
 # Unless required by applicable law or agreed to in writing, software distributed under the Apache License 2.0 is
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations under the License.
-import hashlib
 import json
 import logging
 from decimal import Decimal
@@ -81,15 +80,11 @@ logger = logging.getLogger('pretix.plugins.paypal2')
 class PaypalOrderView:
     def dispatch(self, request, *args, **kwargs):
         try:
-            self.order = request.event.orders.get(code=kwargs['order'])
-            if hashlib.sha1(self.order.secret.lower().encode()).hexdigest() != kwargs['hash'].lower():
-                raise Http404('Unknown order')
+            self.order = request.event.orders.get_with_secret_check(
+                code=kwargs['order'], received_secret=kwargs['hash'].lower(), tag='plugins:paypal2:pay'
+            )
         except Order.DoesNotExist:
-            # Do a hash comparison as well to harden timing attacks
-            if 'abcdefghijklmnopq'.lower() == hashlib.sha1('abcdefghijklmnopq'.encode()).hexdigest():
-                raise Http404('Unknown order')
-            else:
-                raise Http404('Unknown order')
+            raise Http404('Unknown order')
         return super().dispatch(request, *args, **kwargs)
 
     @cached_property

src/pretix/plugins/stripe/payment.py

@@ -32,7 +32,6 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations under the License.
 
-import hashlib
 import json
 import logging
 import re
@@ -636,7 +635,7 @@ class StripeMethod(BasePaymentProvider):
             'order': payment.order,
             'payment': payment,
             'payment_info': payment_info,
-            'payment_hash': hashlib.sha1(payment.order.secret.lower().encode()).hexdigest()
+            'payment_hash': payment.order.tagged_secret('plugins:stripe')
         }
         return template.render(ctx)
 
@@ -890,7 +889,7 @@ class StripeMethod(BasePaymentProvider):
                     return_url=build_absolute_uri(self.event, 'plugins:stripe:sca.return', kwargs={
                         'order': payment.order.code,
                         'payment': payment.pk,
-                        'hash': hashlib.sha1(payment.order.secret.lower().encode()).hexdigest(),
+                        'hash': payment.order.tagged_secret('plugins:stripe'),
                     }),
                     expand=['latest_charge'],
                     **params
@@ -988,7 +987,7 @@ class StripeMethod(BasePaymentProvider):
         url = build_absolute_uri(self.event, 'plugins:stripe:sca', kwargs={
             'order': payment.order.code,
             'payment': payment.pk,
-            'hash': hashlib.sha1(payment.order.secret.lower().encode()).hexdigest(),
+            'hash': payment.order.tagged_secret('plugins:stripe'),
         })
         if not self.redirect_in_widget_allowed and request.session.get('iframe_session', False):
             return build_absolute_uri(self.event, 'plugins:stripe:redirect') + '?data=' + signing.dumps({
@@ -1009,7 +1008,7 @@ class StripeMethod(BasePaymentProvider):
                 return_url=build_absolute_uri(self.event, 'plugins:stripe:sca.return', kwargs={
                     'order': payment.order.code,
                     'payment': payment.pk,
-                    'hash': hashlib.sha1(payment.order.secret.lower().encode()).hexdigest(),
+                    'hash': payment.order.tagged_secret('plugins:stripe'),
                 }),
                 expand=["latest_charge"],
                 **self.api_kwargs
@@ -1829,7 +1828,7 @@ class StripeMultibanco(StripeSourceMethod):
                 'return_url': build_absolute_uri(self.event, 'plugins:stripe:return', kwargs={
                     'order': payment.order.code,
                     'payment': payment.pk,
-                    'hash': hashlib.sha1(payment.order.secret.lower().encode()).hexdigest(),
+                    'hash': payment.order.tagged_secret('plugins:stripe'),
                 })
             },
             **self.api_kwargs

src/pretix/plugins/stripe/views.py

@@ -32,7 +32,6 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations under the License.
 
-import hashlib
 import json
 import logging
 import urllib.parse
@@ -486,15 +485,11 @@ def oauth_disconnect(request, **kwargs):
 class StripeOrderView:
     def dispatch(self, request, *args, **kwargs):
         try:
-            self.order = request.event.orders.get(code=kwargs['order'])
-            if hashlib.sha1(self.order.secret.lower().encode()).hexdigest() != kwargs['hash'].lower():
-                raise Http404('')
+            self.order = request.event.orders.get_with_secret_check(
+                code=kwargs['order'], received_secret=kwargs['hash'].lower(), tag='plugins:stripe'
+            )
         except Order.DoesNotExist:
-            # Do a hash comparison as well to harden timing attacks
-            if 'abcdefghijklmnopq'.lower() == hashlib.sha1('abcdefghijklmnopq'.encode()).hexdigest():
-                raise Http404('')
-            else:
-                raise Http404('')
+            raise Http404('Unknown order')
         self.payment = get_object_or_404(
             self.order.payments,
             pk=self.kwargs['payment'],

src/pretix/presale/views/order.py

@@ -33,6 +33,7 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations under the License.
 import copy
+import hmac
 import inspect
 import json
 import mimetypes
@@ -102,18 +103,12 @@ class OrderDetailMixin(NoSearchIndexViewMixin):
 
     @cached_property
     def order(self):
-        order = self.request.event.orders.filter(code=self.kwargs['order']).select_related('event').first()
-        if order:
-            if order.secret.lower() == self.kwargs['secret'].lower():
-                return order
-            else:
-                return None
-        else:
-            # Do a comparison as well to harden timing attacks
-            if 'abcdefghijklmnopq'.lower() == self.kwargs['secret'].lower():
-                return None
-            else:
-                return None
+        try:
+            return self.request.event.orders.filter().select_related('event').get_with_secret_check(
+                code=self.kwargs['order'], received_secret=self.kwargs['secret'], tag=None,
+            )
+        except Order.DoesNotExist:
+            return None
 
     def get_order_url(self):
         return eventreverse(self.request.event, 'presale:event.order', kwargs={
@@ -133,13 +128,13 @@ class OrderPositionDetailMixin(NoSearchIndexViewMixin):
         ).select_related('order', 'order__event')
         p = qs.first()
         if p:
-            if p.web_secret.lower() == self.kwargs['secret'].lower():
+            if hmac.compare_digest(p.web_secret.lower(), self.kwargs['secret'].lower()):
                 return p
             else:
                 return None
         else:
             # Do a comparison as well to harden timing attacks
-            if 'abcdefghijklmnopq'.lower() == self.kwargs['secret'].lower():
+            if hmac.compare_digest('abcdefghijklmnopq'.lower(), self.kwargs['secret'].lower()):
                 return None
             else:
                 return None