RFC: #12417 - Support for signing (including secure cookies)
Jannis Leidel
This is in continuation to Simon's previous efforts about adding tools
for easy signing, including secure cookies ([1], [2]).
Stephan (who is working on #9200 [3] -- improving the wizard in
contrib.formtools) and me just updated the patch attached to ticket
#12417 [4] with the recommended changes according to the mailing list
threads and the Trac ticket:
http://code.djangoproject.com/attachment/ticket/12417/ticket12417-v4.diff
The complete changes (as noted) are:
- Moved from django.utils.signed to django.core.signing
- Removed the seperator argument to the Signer.sign and Signer.unsign
methods and moved it to a class attribute
- Added key prefix ('django.http.cookies') to Signer instantiation
- Changed key ordering from `"signer" + key + salt` to `salt + "signer" + key`
to lower the chance for brute force attacks
- PEP8 and other style changes
- Use constant_time_compare from django.utils.crypto for timing attack
proof signature verification
- Updated and fixed docs
- Changed setting name from COOKIE_SIGNER_BACKEND to SIGNING_BACKEND
We'd appreciate any further comments before I commit this patch, given
the formtools wizard being dependent on it for its cookie storage backend.
As always don't hesitate to ask if there are any questions.
Best,
Stephan and Jannis
1: http://groups.google.com/group/django-developers/browse_thread/thread/d9d635afb6d1820f/
2: http://groups.google.com/group/django-developers/browse_thread/thread/297e8b22006f7f3a/
3: http://code.djangoproject.com/ticket/9200
4: http://code.djangoproject.com/ticket/12417
Luke Plant
> Hi all,
>
> This is in continuation to Simon's previous efforts about adding tools
> for easy signing, including secure cookies ([1], [2]).
>
> Stephan (who is working on #9200 [3] -- improving the wizard in
> contrib.formtools) and me just updated the patch attached to ticket
> #12417 [4] with the recommended changes according to the mailing list
> threads and the Trac ticket:
>
> http://code.djangoproject.com/attachment/ticket/12417/ticket12417-v4.diff
>
> The complete changes (as noted) are:
>
> - Moved from django.utils.signed to django.core.signing
> - Removed the seperator argument to the Signer.sign and Signer.unsign
> methods and moved it to a class attribute
> - Added key prefix ('django.http.cookies') to Signer instantiation
> - Changed key ordering from `"signer" + key + salt` to `salt + "signer" + key`
> to lower the chance for brute force attacks
> - PEP8 and other style changes
> - Use constant_time_compare from django.utils.crypto for timing attack
> proof signature verification
> - Updated and fixed docs
> - Changed setting name from COOKIE_SIGNER_BACKEND to SIGNING_BACKEND
>
> We'd appreciate any further comments before I commit this patch, given
> the formtools wizard being dependent on it for its cookie storage backend.
We already have one way of using hmac with unique keys for each
application - django.utils.crypto.salted_hmac. I looked at the code, and
can't see any reason we couldn't make Signer use that code path, rather
than having two very similar code paths. I see no reason for the new
code to directly use either settings.SECRET_KEY or hmac even once, and
doing so makes our code much harder to audit for security problems -
every place where we use SECRET_KEY or hmac directly is a place we have
to worry about.
Currently we have 4 uses of SECRET_KEY (one of them is in a deprecated
code path), and 1 use of hmac.new.
So I feel quite strongly that we should fix this code to use
salted_hmac. (Or fix salted_hmac if there is some problem with it, but
remembering that there is lots of data that depends on it).
I haven't had time to further review, but thought I'd get that out
straight away.
Regards,
Luke
--
Parenthetical remarks (however relevant) are unnecessary
Luke Plant || http://lukeplant.me.uk/
Paul McMillan
won't have a chance to do that for a day or two.
I agree with Luke re: salted hmac.
A couple of initial thoughts:
Since we aren't supporting Python 2.4 anymore, we have access to
hashlib. We should use SHA256 or 512.
Since these functions are for signing JSON objects, let's use the
existing format which is on the standards track - JSON Web Signature
(JWS)[1] for signing (and in the future, JWE for encryption). They
(along with JWT, the token format) are part of OAuth2.0, but are
getting standardized separately so they get done sooner.
It's mostly a formatting change to produce output that is compatible.
I'll be happy to write the change it if nobody complains about the
idea.
I like the idea of using a standard for this. I REALLY like the idea
that other people have been reviewing this standard with cryptographic
security in mind.
-Paul
[1] http://tools.ietf.org/html/draft-jones-json-web-signature
> --
> You received this message because you are subscribed to the Google Groups "Django developers" group.
> To post to this group, send email to django-d...@googlegroups.com.
> To unsubscribe from this group, send email to django-develop...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
>
>
Stephan Jäkel
> So I feel quite strongly that we should fix this code to use
> salted_hmac. (Or fix salted_hmac if there is some problem with it, but
> remembering that there is lots of data that depends on it).
I updated the patch and changed the way the hashes are generated. We now use
the salted_hmac method.
The SECRET_KEY is still in the get_cookie_signer() method because we add a
prefix to the secret. If we want to remove the settings.SECRET_KEY in
get_cookie_signer() too, we have to add some kind of a optional
secret_prefix to salted_hmac.
http://code.djangoproject.com/attachment/ticket/12417/ticket12417-v5.diff
Looking forward to any comments!
Cheers,
Stephan
Jannis Leidel
Agreed, see the patch Stephan just updated.
Jannis
Jannis Leidel
On 12.05.2011, at 14:17, Paul McMillan wrote:
> I'd also like to review this in depth before it gets committed, but
> won't have a chance to do that for a day or two.
>
> I agree with Luke re: salted hmac.
>
> A couple of initial thoughts:
>
> Since we aren't supporting Python 2.4 anymore, we have access to
> hashlib. We should use SHA256 or 512.
Yeah, SHA256 and SHA512 have the downsides of being rather long for
the type of application the signatures are going to be used for,
e.g. one-time URLs.
Simon discussed this a year ago, with the hint that using SHA1
together with HMAC isn't prone to the collision issue:
"I chose sha-1 over sha-256 for reasons of signature length. A base64
encoded signature generated with hmac/sha1 is 27 characters long. The
same thing using hmac/sha256 is 43 characters long. If you're planning
on using signatures in cookies and URLs that's quite a big difference
(43 characters is more than half of the maximum 80 characters needed
to safely transmit URLs in plain text e-mails, e.g. for account
recovery links)."
http://groups.google.com/group/django-developers/msg/2ceedd2dcb4feed7
In other words, if we go with SHA256 or even SHA512 we'll limit the
usefulness of this feature -- with only theoretical gain in security
per the discussion a year ago.
> Since these functions are for signing JSON objects, let's use the
> existing format which is on the standards track - JSON Web Signature
> (JWS)[1] for signing (and in the future, JWE for encryption). They
> (along with JWT, the token format) are part of OAuth2.0, but are
> getting standardized separately so they get done sooner.
>
> It's mostly a formatting change to produce output that is compatible.
> I'll be happy to write the change it if nobody complains about the
> idea.
If I read the draft spec correctly it's basically the same algorithm
as used in the current patch but SHA256 instead of SHA1. [1]
> I like the idea of using a standard for this. I REALLY like the idea
> that other people have been reviewing this standard with cryptographic
> security in mind.
Fair enough.
Jannis
1: http://tools.ietf.org/html/draft-jones-json-web-signature-02#section-7
Paul McMillan
> the type of application the signatures are going to be used for,
> e.g. one-time URLs.
That's fair. SHA1 is moving towards broken, but HMAC currently covers
that gap. Shorter is certainly a feature we want here.
> If I read the draft spec correctly it's basically the same algorithm
> as used in the current patch but SHA256 instead of SHA1. [1]
Yeah. Unfortunately JWS packages a fair bit of data which is not
strictly necessary (encryption protocol, etc.). In the interest of
letting users put as much data as possible in cookies, we probably
want to avoid that.
Baseconv is a clever bit of work. It should probably be using our full
base64 charset. We could also shave some digits in the time-limited
output there by subtracting from a more recent fixed value than the
unix epoch, the way the password reset token code does.
I'm not entirely happy with the way the salting works (or perhaps I've
misunderstood it). It seems more appropriate to generate the salt (let
the user specify the length) and store it with the string, the same
way you do salted passwords. The way the interface is currently
written, nobody will ever use a salt, or they'll try to use a static
string as a salt (the existing docs get it wrong in exactly this way),
which largely defeats the purpose. It would make more sense to be able
to say "I want salt" the way you say "I want a timestamp".
-Paul
Paul McMillan
even during debug (line 156 of signing.py):
Do it the same way as it is with the auth tokens, compute the expected
value inline without assigning it to a variable.
http://code.djangoproject.com/browser/django/trunk/django/contrib/auth/tokens.py#L34
-Paul
Jannis Leidel
> Baseconv is a clever bit of work. It should probably be using our full
> base64 charset. We could also shave some digits in the time-limited
> output there by subtracting from a more recent fixed value than the
> unix epoch, the way the password reset token code does.
The TimeStampSigner requires the seconds to correct determine whether the
signature is still valid, so I'm not sure if that's a good idea. If you
have an idea to have both thing, I'd appreciate any help.
> I'm not entirely happy with the way the salting works (or perhaps I've
> misunderstood it). It seems more appropriate to generate the salt (let
> the user specify the length) and store it with the string, the same
> way you do salted passwords. The way the interface is currently
> written, nobody will ever use a salt, or they'll try to use a static
> string as a salt (the existing docs get it wrong in exactly this way),
> which largely defeats the purpose. It would make more sense to be able
> to say "I want salt" the way you say "I want a timestamp".
Yeah, I agree the salt argument for the sign and unsign methods seems
a bit extensive, which is why I updated the code [1] to only have a
salt argument for initialization of the Signer class. Additionally it'll
generate a random salt if no salt is passed to __init__.
Yeah, good point, I updated the code for that, too.
Jannis
1: http://code.djangoproject.com/attachment/ticket/12417/ticket12417-v6.diff
Jannis Leidel
On 18.05.2011, at 20:19, Jannis Leidel wrote:
> On 13.05.2011, at 07:33, Paul McMillan wrote:
>
>> Baseconv is a clever bit of work. It should probably be using our full
>> base64 charset. We could also shave some digits in the time-limited
>> output there by subtracting from a more recent fixed value than the
>> unix epoch, the way the password reset token code does.
>
> The TimeStampSigner requires the seconds to correct determine whether the
> signature is still valid, so I'm not sure if that's a good idea. If you
> have an idea to have both thing, I'd appreciate any help.
Wow, ironically, this should have read "correctly" and "things".
Jannis
Paul McMillan
>> base64 charset. We could also shave some digits in the time-limited
>> output there by subtracting from a more recent fixed value than the
>> unix epoch, the way the password reset token code does.
>
> The TimeStampSigner requires the seconds to correct determine whether the
> signature is still valid, so I'm not sure if that's a good idea. If you
> have an idea to have both thing, I'd appreciate any help.
If we subtract 1e9 from our timestamp, we get a 5 digit timestamp
rather than 6 for the next 19 years. If we add - and _ to our allowed
characterset, we extend that to 24 years.
int(time.time()) == 1305761382
base62.encode(1305761382) == '1QMqBS'
base62.encode(1305761382) == 'KgwVC'
Shaving 1 character seems like an overoptimization, except that we're
talking about storing values in cookies, where space is already very
limited.
I'll see if I can find you on IRC to discuss the salting.
-Paul
Paul McMillan
> base62.encode(1305761382) == '1QMqBS'
> base62.encode(1305761382) == 'KgwVC'
Oops! meant to say
base62.encode(305761382) == 'KgwVC'
-Paul