[Django] #33199: Consider making Signer parameters keyword-only
Django
------------------------------------------+------------------------
Reporter: Daniel Samuels | Owner: nobody
Type: New feature | Status: new
Component: Core (Other) | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
We discovered a vulnerability in one of our applications recently which
was caused by an inaccurate instantiation of `django.core.signing.Signer`.
The developer intended to use the user's email address as the salt for the
Signing instance but instead caused it to be used as the key. Here's an
example code block that demonstrates the problem:
{{{#!python
signer = Signer(self.context['request'].user.email)
signed_data = signer.sign_object(dict(
license_number='...',
product_id='...',
device_count='...'
))
}}}
In our case, this signed data was then being used to verify a later
request and generate an active license. This meant that an attacker could
feasibly generate their own licenses if they realised that their email
address was the key. The fix for this was to add `salt=` in front of the
email variable. It occurred to us that this is a relatively easy mistake
to make and could be avoided if the signature of `Signer.__init__` was
changed thusly:
{{{#!diff
- def __init__(self, key=None, sep=':', salt=None, algorithm=None):
+ def __init__(self, *, key=None, sep=':', salt=None, algorithm=None):
}}}
That is, adding a `*` after `self` to force the developer to name the
parameters.
--
Ticket URL: <https://code.djangoproject.com/ticket/33199>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------+------------------------------------
Reporter: Daniel Samuels | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Florian Apolloner (added)
* version: 3.2 => dev
* stage: Unreviewed => Accepted
Comment:
Agreed. This is a backward incompatible change so release notes and a
`versionchanged` annotation is necessary, we also need to audit all
[https://docs.djangoproject.com/en/stable/topics/signing/#using-the-low-
level-api documented] uses.
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:1>
Django
--------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniel Samuels):
* cc: Daniel Samuels (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:2>
Django
--------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Daniel, would you like to prepare a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:3>
Django
--------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Bill Collins):
* cc: Bill Collins (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:4>
Django
--------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniel Samuels):
I've opened a PR to address this ticket:
https://github.com/django/django/pull/14995
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:5>
Django
Reporter: Daniel Samuels | Owner: Daniel Samuels
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: nobody => Daniel Samuels
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:6>
Django
--------------------------------+------------------------------------------
Reporter: Daniel Samuels | Owner: Daniel Samuels
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:7>
Django
--------------------------------+------------------------------------------
Reporter: Daniel Samuels | Owner: Daniel Samuels
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:8>
Django
--------------------------------+------------------------------------------
Reporter: Daniel Samuels | Owner: Daniel Samuels
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Abhyudai):
* cc: Abhyudai (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:9>
Django
Reporter: Daniel Samuels | Owner: Nikita
| Marchant
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Nikita Marchant):
* owner: Daniel Samuels => Nikita Marchant
Comment:
Hi!
It looks like Daniel
[https://github.com/django/django/pull/14995#issuecomment-989645451 does
not have time] to handle this ticket so i would like to have a try.
I spent a little time thinking about the best way to do this and I see a
few options:
1. Write a decorator that makes the function accept positional arguments,
sends a warning and rewrites the call to using kwargs only
([https://paste.awesom.eu/62le Proof of concept])
2. Add `*args` to the signature and use a trick with `locals()` to inject
those into the local variables ([https://paste.awesom.eu/WqQ8 Proof of
concept], thanks to [https://github.com/t00n t00n] for the idea and code)
3. Change the signature to `__init__(self, *args, **kwargs)`
1 and 2 have the advantage of not changing the signature and body of the
function too much so it will be easy to remove when the depreciation
period ends but are using maybe too much of dark magic. 3 is the opposite.
What do you think would be best in Django's case ? When i know what
approach to take, i'd be happy to update
[https://github.com/django/django/pull/14995 Daniel's PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Samuels | Owner: Nikita
| Marchant
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob Walls):
I think prepending `*args` before the existing arguments is good, but if
you can avoid tinkering with `locals()`, the better, I think.
What about something like:
{{{
diff --git a/django/core/signing.py b/django/core/signing.py
index 5ee19a9336..724a7df507 100644
--- a/django/core/signing.py
+++ b/django/core/signing.py
@@ -37,10 +37,12 @@ import base64
import datetime
import json
import time
+import warnings
import zlib
from django.conf import settings
from django.utils.crypto import constant_time_compare, salted_hmac
+from django.utils.deprecation import RemovedInDjango50Warning
from django.utils.encoding import force_bytes
from django.utils.module_loading import import_string
from django.utils.regex_helper import _lazy_re_compile
@@ -145,16 +147,25 @@ def loads(s, key=None, salt='django.core.signing',
serializer=JSONSerializer, ma
class Signer:
- def __init__(self, key=None, sep=':', salt=None, algorithm=None):
+ def __init__(self, *args, key=None, sep=':', salt=None,
algorithm=None):
self.key = key or settings.SECRET_KEY
self.sep = sep
+ self.salt = salt or '%s.%s' % (self.__class__.__module__,
self.__class__.__name__)
+ self.algorithm = algorithm or 'sha256'
+ if args:
+ warnings.warn(
+ 'Providing positional arguments to Signer is
deprecated.',
+ RemovedInDjango50Warning,
+ stacklevel=2,
+ )
+ for arg, attr in zip(args, ['key', 'sep', 'salt',
'algorithm']):
+ if arg or attr == 'sep':
+ setattr(self, attr, arg)
if _SEP_UNSAFE.match(self.sep):
raise ValueError(
'Unsafe Signer separator: %r (cannot be empty or consist
of '
'only A-z0-9-_=)' % sep,
)
- self.salt = salt or '%s.%s' % (self.__class__.__module__,
self.__class__.__name__)
- self.algorithm = algorithm or 'sha256'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:11>
Django
Reporter: Daniel Samuels | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Nikita Marchant => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:12>
Django
Reporter: Daniel Samuels | Owner: Abhinav Yadav
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Abhinav Yadav):
* owner: (none) => Abhinav Yadav
* needs_better_patch: 1 => 0
* status: new => assigned
Comment:
[https://github.com/django/django/pull/16343 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:13>
Django
--------------------------------+-----------------------------------------
Reporter: Daniel Samuels | Owner: Abhinav Yadav
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:14>
Django
--------------------------------+-----------------------------------------
Reporter: Daniel Samuels | Owner: Abhinav Yadav
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Abhinav Yadav):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:15>
Django
Reporter: Daniel Samuels | Owner: Abhinav
| Yadav
Type: New feature | Status: assigned
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Samuels | Owner: Abhinav
| Yadav
Component: Core (Other) | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b8738aea14446b267a47087b52b38a98b440a6aa" b8738aea]:
{{{
#!CommitTicketReference repository=""
revision="b8738aea14446b267a47087b52b38a98b440a6aa"
Fixed #33199 -- Deprecated passing positional arguments to
Signer/TimestampSigner.
Thanks Jacob Walls for the implementation idea.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Daniel Samuels | Owner: Abhinav
| Yadav
Type: New feature | Status: closed
Component: Core (Other) | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3a3e737694b89e29308e54f12faaa375226b7060" 3a3e737]:
{{{
#!CommitTicketReference repository=""
revision="3a3e737694b89e29308e54f12faaa375226b7060"
Refs #33199 -- Removed support for passing positional arguments to
Signer/TimestampSigner.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33199#comment:18>