Form Signatures listing different fields between form render and submit, hashing not working #998

Closed
cinaeco opened this Issue Jul 25, 2013 · 6 comments

Comments

Projects
None yet
4 participants
@cinaeco

cinaeco commented Jul 25, 2013

I was trying to use form signing when I ran into issues:

  1. template\helper\Security's filter on '_defaults' also picks up label names e.g. $state[$id]['fields'] might contain 'AddressesPostCode' and 'address.postCode' field names, whereas when checking the request data, only the 'address.postCode' field would exist. Signatures on forms with labels would never agree. I bypassed this temporarily by putting in a check for $params['method'] == 'label' and skipping those elements.
  2. When FormSignature::check() calls FormSignature::key(), it has it's fields array with field names as values, instead of as keys as template\helper\Security's filters do. I bypassed this temporarily by putting in an array_flip() before calling key().
  3. I then tried to submit a form, but first mangled a field name, thinking this would produce an error, but it didn't. The hashes for submitting a form with and without a mangled field name remain the same e.g.

original field (1 out of 14 others): 'parent.mobile'
original hash: '$2a$10$NuNTOeXv4OHpPJtbdAmfReEtZeKhI9F9xBL8qGvbO.kFHM4KzJoRK'

mangled field (1 out of 14 others): 'parensfsdfsdfsdfsdft.mobile'
mangled hash: '$2a$10$NuNTOeXv4OHpPJtbdAmfReEtZeKhI9F9xBL8qGvbO.kFHM4KzJoRK'

Have I misunderstood how this is supposed to work? Should the field names make a difference to the hash?

@cinaeco

This comment has been minimized.

Show comment
Hide comment
@cinaeco

cinaeco Jul 26, 2013

Okay, after some help from @dmondark, found that changing the default blowfish hash to an md5 hash allows me to detect the changes in fields. I'll just be making my own app extension for security\validation\FormSignature to change the hash to something else (which... I should have done anyway) and to add the array_flip().

The other two things mentioned are still problems though: 1) what's the best way to censor $params['method'] == 'label' and 2) there needs to be that array_flip().

Found another issue: if a form is a PUT, then the '_method' hidden input is added to the $locked array, but is not present in the $data array, so FormSignature::check() contains an array_intersect_assoc() check that will fail.

cinaeco commented Jul 26, 2013

Okay, after some help from @dmondark, found that changing the default blowfish hash to an md5 hash allows me to detect the changes in fields. I'll just be making my own app extension for security\validation\FormSignature to change the hash to something else (which... I should have done anyway) and to add the array_flip().

The other two things mentioned are still problems though: 1) what's the best way to censor $params['method'] == 'label' and 2) there needs to be that array_flip().

Found another issue: if a form is a PUT, then the '_method' hidden input is added to the $locked array, but is not present in the $data array, so FormSignature::check() contains an array_intersect_assoc() check that will fail.

@mariuswilms

This comment has been minimized.

Show comment
Hide comment
@mariuswilms

mariuswilms Jan 8, 2014

Member

@cinaeco First off thanks for reporting these issues. As a rule of thumb it's always best to only discuss one issue per ticket. That allows us to have a cleaner discussion about the issues as everybody knows what is being talked about.

However, I wanted to ask you if you can provide the changes you made to make form signatures work for you as a Pull Request (and reference this issue)?

Member

mariuswilms commented Jan 8, 2014

@cinaeco First off thanks for reporting these issues. As a rule of thumb it's always best to only discuss one issue per ticket. That allows us to have a cleaner discussion about the issues as everybody knows what is being talked about.

However, I wanted to ask you if you can provide the changes you made to make form signatures work for you as a Pull Request (and reference this issue)?

@mariuswilms

This comment has been minimized.

Show comment
Hide comment
Member

mariuswilms commented Sep 29, 2014

Refs #1106

koushki added a commit to koushki/lithium that referenced this issue Oct 3, 2014

koushki added a commit to koushki/lithium that referenced this issue Oct 3, 2014

koushki added a commit to koushki/lithium that referenced this issue Oct 3, 2014

koushki added a commit to koushki/lithium that referenced this issue Oct 3, 2014

koushki added a commit to koushki/lithium that referenced this issue Oct 3, 2014

@mariuswilms

This comment has been minimized.

Show comment
Hide comment
@mariuswilms

mariuswilms Oct 4, 2014

Member
  • label fields
  • no hash change (blowfish)
  • PUT method
  • array_flip?
Member

mariuswilms commented Oct 4, 2014

  • label fields
  • no hash change (blowfish)
  • PUT method
  • array_flip?

koushki added a commit to koushki/lithium that referenced this issue Oct 4, 2014

@koushki

This comment has been minimized.

Show comment
Hide comment
@koushki

koushki Oct 4, 2014

Contributor

Label fields fixed in #1132 .
no hash change (blowfish) fixed in #1130 .
PUT method fixed in #1131 .
array_flip fixed in #1130 .

Contributor

koushki commented Oct 4, 2014

Label fields fixed in #1132 .
no hash change (blowfish) fixed in #1130 .
PUT method fixed in #1131 .
array_flip fixed in #1130 .

koushki added a commit to koushki/lithium that referenced this issue Oct 4, 2014

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

Fixing test where FormSignature would fail on fields.
Fixes 2. of issue #998.

Refactoring into symmetric compile/parse methods.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

Fixing test where FormSignature would fail on fields.
Fixes 2. of issue #998.

Refactoring into symmetric compile/parse methods.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

Fixing test where FormSignature would fail on fields.
Fixes 2. of issue #998.

Refactoring into symmetric compile/parse methods.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

Fixing test where FormSignature would fail on fields.
Fixes 2. of issue #998.

Refactoring into symmetric compile/parse methods.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Oct 5, 2014

Fixing test where FormSignature would fail on fields.
Fixes 2. of issue #998.

Refactoring into symmetric compile/parse methods.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Mar 14, 2015

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.

@mariuswilms mariuswilms self-assigned this Mar 14, 2015

@nateabele

This comment has been minimized.

Show comment
Hide comment
@nateabele

nateabele Mar 31, 2015

Member

Fixed.

Member

nateabele commented Mar 31, 2015

Fixed.

@nateabele nateabele closed this Mar 31, 2015

mariuswilms added a commit that referenced this issue Mar 31, 2015

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.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Mar 31, 2015

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.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Mar 31, 2015

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.

mariuswilms added a commit to mariuswilms/lithium that referenced this issue Mar 31, 2015

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.

mariuswilms added a commit that referenced this issue Mar 31, 2015

Merge pull request #1173 from davidpersson/fix/form-signing
Changing form signature calculation logic. Fixes #839, #998. BC-break.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment