Ecodev / newsletter

TYPO3 extension to send newsletter
https://extensions.typo3.org/extension/newsletter/
25 stars 26 forks source link

Passwords Bounceaccount Plaintext stored #11

Closed PowerKiKi closed 8 years ago

PowerKiKi commented 10 years ago

Passwords for bounceaccount are stored in plaintext.

It should be a normal behaviour to store passwords salted.

History

1 Updated by Adrien Crivelli over 1 year ago Comment Edit I am not sure salted passwords is the solution here, as we need to be able to retrieve the original password. I had a look a few months ago in t3lib for some functions to do that, but I could not find out anything fitting our needs. Would you have anything specific to suggest ?

2 Updated by d.ros no-lastname-given over 1 year ago Comment Edit Dunno really. I had some time ago the need to implement RSA into an individual extension and made it with Portable PHP password hashing framework. -> http://www.openwall.com/phpass/

Greets

3 Updated by Adrien Crivelli over 1 year ago Comment Edit phpass is interesting, but again its for hashing, not encrypt/decrypt. So we can't get original password back with phpass.

I guess we'll have to look harder for something else or just come up with our own implementation...

stephankellermayr commented 10 years ago

hm, there is a hook processDatamap_preProcessFieldArray in TYPO3\CMS\Core\DataHandling\DataHandler which looks interesting.

with that and and some code for saltedpasswords i can write a hashed password into database [tested].

is that the correct way? or are there possibilities modifying this value in the database-model directly?

and: how can we decrypt the hashed password then?

PowerKiKi commented 10 years ago

how can we decrypt the hashed password then?

This is the question you must answer first. Hashing is not the solution. We need something revertible, and hence quite a bit insecure by definition. ROT13 or a variation of similar genre is what we need here.

stephankellermayr commented 10 years ago

yes, saltedpasswords is not the right tool, just tried it out. but what is the benefit from using ROT13? i may be wrong, but it can be easily decrypted, right?

PowerKiKi commented 10 years ago

It's the whole point, we must be able to retrieve the original password. Have a close look at https://github.com/Ecodev/newsletter/blob/master/Classes/BounceHandler.php#L93

stephankellermayr commented 10 years ago

So you want to encode the password with ROT13 when storing the record to the database and then you want to decode it with ROT13 when using fetchBouncedEmails. did i get it?

stephankellermayr commented 10 years ago

I think we should use the unique TYPO3-encryption-key, stored in $TYPO3_CONF_VARS['SYS']['encryptionKey']. with that we can do something like:

$encodedPassword = base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, md5($encryptionKey), $password, MCRYPT_MODE_CBC, md5(md5($encryptionKey))));
$decodedPassword = rtrim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, md5($encryptionKey), base64_decode($encodedPassword), MCRYPT_MODE_CBC, md5(md5($encryptionKey))), "\0");

this should result in a rather secure db-record which is only readable with the encryption-key stored in LocalConfiguration.php

what do you think of that?

PowerKiKi commented 10 years ago

That sounds like a good idea. "Locking" the password on a specific install makes it slightly more difficult to decrypt. I think it could be done in Tx_Newsletter_Domain_Model_BounceAccount::setPassword() and Tx_Newsletter_Domain_Model_BounceAccount::getPassword(), and, if needed, called from processDatamap_preProcessFieldArray (via a static method maybe ?)

stephankellermayr commented 10 years ago

i have tried with Tx_Newsletter_Domain_Model_BounceAccount yesterday with no success and could not found the bug. nothing happens if you change that file. ...any ideas?

stephankellermayr commented 10 years ago

...sorry, i mean: nothing happens when editing a record. thats why i mentioned the hook.

PowerKiKi commented 10 years ago

What I meant is we need to crypt/decrypt in two places, the model and the hook you mentioned. We should share the code between those places, so one way to do it would be via static method on the model, or a service called by the tow places.

stephankellermayr commented 9 years ago

I don't know where else it could be useful in the future, but i decided to put the static methods for encryption/decryption into Tx_Newsletter_Tools. Any objections?

PowerKiKi commented 9 years ago

I don't mind. Since we probably need to use it from a hook, it makes senses to have it easily accessible.

Don't forget to think about migration of existing data. Should we encrypt them automatically ? when and how ?

PowerKiKi commented 8 years ago

Released in 2.6.0