Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Reduce overhead in Form Signing #839

Closed
Ciaro opened this Issue · 11 comments

4 participants

@Ciaro

Hello guys

The current implementation of CSRF protection in Lithium seems to have a lot of unneeded overhead. It uses sha512 by default, but even if you define md5 as a type, you are still getting tokens generated by 'php::crypt'. This can add up if you use a couple of forms one a page.

Could we not accomplish the same functionality by just using plain-old md5 tokens?

Please do correct me if I'm wrong about this :)

Best regards,

Ciaro

@nateabele
Owner

I would need to do more research into (a) what the real hashing needs are for CSRF tokens, and (b) the overall time difference of each hashing function.

If someone wants to help with a breakdown of these, that would be great.

@Ciaro

(a) in http://shiflett.org/articles/cross-site-request-forgeries, the author uses md5 in his example.
(b) on a default nginx setup (dev and production) it's between 50ms and 100ms per token generated by 'php::crypt'. Using password::hash with an md5 salt is 16ms for the first, then around 7-8 for anything after it, so acceptable?

Hope this helps.

@nateabele
Owner
@Ciaro

@nateabele Where you able to talk to Chris about this yet? Thanks.

@davidpersson

@Ciaro I think the performance problem is not coming from using sha512 for the request token but from using blowfish for the form signing in:
https://github.com/UnionOfRAD/lithium/blob/master/security/validation/FormSignature.php#L39

Also see:
http://php.net/manual/en/function.crypt.php

$2a$10$ is blowfish with 10 rounds and $6$ would be sha512. The question now is if we really need BF which is meant to be slow?

@davidpersson

Refs 60bbe47

@Ciaro

@davidpersson I don't see why plain md5 shouldn't work for CSRF?

@davidpersson

@Ciaro I'd say the tiny performance improvement switching sha512 to md5 isn't worth what we eventually lose in security. Also as I mentioned the real problem here is the usage of blowfish.

@Ciaro

@davidpersson I was suggesting to bypass 'php::crypt' completely for the CSRF tokens and just use plain md5 for those.

@gwoo
Owner
@davidpersson davidpersson added this to the 1.0 milestone
@davidpersson davidpersson changed the title from Reduce overhead in CSRF protection to Reduce overhead in Form Signing
@davidpersson davidpersson referenced this issue from a commit in davidpersson/lithium
@davidpersson davidpersson Changing form signature calculation logic. Fixes #839, #998. BC-break.
The calculation logic now uses HMAC which requires a secret key. Such
a unique key must be set in userland before starting to use form
signature generation or verification.

The new logic is basically modelled after message signatures and its
purpose is to ensure integrity of the compiled form signature string.

Adding test to prove overflowing is prevented.
7a950d2
@davidpersson davidpersson self-assigned this
@nateabele
Owner

Fixed.

@nateabele nateabele closed this
@davidpersson davidpersson referenced this issue from a commit
@davidpersson davidpersson Changing form signature calculation logic. Fixes #839, #998. BC-break.
The calculation logic now uses HMAC which requires a secret key. Such
a unique key must be set in userland before starting to use form
signature generation or verification.

The new logic is basically modelled after message signatures and its
purpose is to ensure integrity of the compiled form signature string.

Adding test to prove overflowing is prevented.
ba42674
@davidpersson davidpersson referenced this issue from a commit in davidpersson/lithium
@davidpersson davidpersson Changing form signature calculation logic. Fixes #839, #998. BC-break.
The calculation logic now uses HMAC which requires a secret key. Such
a unique key must be set in userland before starting to use form
signature generation or verification.

The new logic is basically modelled after message signatures and its
purpose is to ensure integrity of the compiled form signature string.

Adding test to prove overflowing is prevented.

Adding changelog entries.
070a3fa
@davidpersson davidpersson referenced this issue from a commit in davidpersson/lithium
@davidpersson davidpersson Changing form signature calculation logic. Fixes #839, #998. BC-break.
The calculation logic now uses HMAC which requires a secret key. Such
a unique key must be set in userland before starting to use form
signature generation or verification.

The new logic is basically modelled after message signatures and its
purpose is to ensure integrity of the compiled form signature string.

Adding test to prove overflowing is prevented.

Adding changelog entries.
f33f3b0
@davidpersson davidpersson referenced this issue from a commit in davidpersson/lithium
@davidpersson davidpersson Changing form signature calculation logic. Fixes #839, #998. BC-break.
The calculation logic now uses HMAC which requires a secret key. Such
a unique key must be set in userland before starting to use form
signature generation or verification.

The new logic is basically modelled after message signatures and its
purpose is to ensure integrity of the compiled form signature string.

Adding test to prove overflowing is prevented.

Adding changelog entries.
bc4fe9f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.