Opened 2 years ago

Closed 17 months ago

Last modified 15 months ago

#22804 closed Cleanup/optimization (fixed)

A warning should be issued when an invalid separator is passed to signing.Signer

Reported by: David Wolever Owned by: Jaap Roes
Component: Core (Other) Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Currently there is no warning or error when an invalid value (ex, -) is passed to django.core.signing.Signer.

See https://github.com/django/django/pull/2784 for a patch which adds a warning.

An exception might be even better, but that would break backwards compatibility.

Attachments (1)

0001-Warn-on-unsafe-value-of-sep-in-Signer.patch (2.9 KB) - added by David Wolever 2 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 2 years ago by David Wolever

Summary: A warning should be issued when an invalid separator is passed to crypto.SignerA warning should be issued when an invalid separator is passed to signing.Signer

comment:2 Changed 2 years ago by Baptiste Mispelon

Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Hi,

I'll mark this as accepted on the basis that the current behavior leaves the user open to serious issues.

However, wouldn't it be better to go through the deprecation process and raise an error eventually? I can't think of a reason why a user would want to pass a separator that has a chance to appear in the encoded data.

Thanks.

comment:3 Changed 2 years ago by Mark Lavin

If you can't distinguish the data from the signature then the signature can't be verified. A warning is not strong enough. This should be an exception.

comment:4 Changed 2 years ago by David Wolever

Yep - would definitely agree it should go through the deprecation process and raise an error eventually. I'd be -0 on raising an error immediately because invalid separators can appear to work (I was using "-" as a separator on a low traffic internal thing), and halting application entirely could be even less helpful. Proposal for now:

1) Change warning to deprecation warning

2) If a bad signature error is raised, check to see if the sep is invalid and augment the error message if it is (ex, InavlidSignature("the signature was invalid; NOTE: the separator '-' is not valid; this may be the cause of the invalid signature"))

3) Raise an exception in the future (as per whatever deprecation timeline is sensible)

comment:5 Changed 2 years ago by Erik Romijn

I agree with the latest comment by wolever. We should not allow broken behaviour, so raising an exception is right in the long term, but a little harsh to do in 1.8 straight away. I suggest an accelerated deprecation, so DeprecationWarning in 1.8, exception in 1.9. This will then also have to be added to the 1.8 release notes, and the deprecation timeline.

Changed 2 years ago by David Wolever

comment:6 Changed 2 years ago by David Wolever

Okay, attached is a patch which correctly implements and tests invalid separators. Please forgive the small micro-optimization (sep != ":' and ...). I don't know the correct procedure for adding to the release notes or deprecation timeline, so if someone could give me a hand with that it would be great (and, specifically: if someone just wants to go ahead and commit this, I don't care if my name isn't in the commit log).

comment:7 Changed 18 months ago by Tim Graham

Easy pickings: set

Marking as easy pickings since updating the initial pull request for the comments shouldn't be too difficult.

comment:8 Changed 17 months ago by Jaap Roes

I've rebased the original patch and then applied the patch in this ticket, with some minor changes by myself. If this patch is acceptable I'll update the release notes.

comment:9 Changed 17 months ago by Jaap Roes

Owner: changed from nobody to Jaap Roes
Status: newassigned

comment:10 Changed 17 months ago by Tim Graham

Patch needs improvement: unset

comment:11 Changed 17 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 0d71349:

Fixed #22804 -- Added warning for unsafe value of 'sep' in Signer

Thanks Jaap Roes for completing the patch.

comment:12 Changed 15 months ago by Tim Graham <timograham@…>

In e6cfa08:

Refs #22804 -- Made an unsafe value of 'sep' in Signer an exception.

Per deprecation timeline.

Note: See TracTickets for help on using tickets.
Back to Top