The smtp_password variable in the database is being stored as plain-text. This is not good. Please patch immediately.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Harold Villacorte’s picture

Status: Active » Needs review
FileSize
11.85 KB

Here is the "patch." It is actually a major addition to the module. There is still an issue of Drupal throwing notice errors but I have a solution for that. If the patch will be included in the next release we need to discuss on having the module create a cusom table. I also have other ideas for this project I would like to contribute but first I think this issue should be addressed.

The patch incorporates two-way encryption which does not completely secure the password but it does add a good layer of security.

Harold Villacorte’s picture

Priority: Critical » Normal
pandaPowder’s picture

I just read through a great discussion about this same issue http://drupal.org/node/117450 and I came to the same conclusion they did. As http://drupal.org/node/117450#comment-1118888 kylehase said, the module needs the password in plain text one way or another. In my opinion, since your patch stores the encryption key alongside the encrypted password itself, the protection you gain by doing it is not enough to justify the extra complexity in the module. Any savvy person with access to the database could decrypt the password and if the hacker has access to your database then the entire site is compromised anyway.

Harold Villacorte’s picture

You are right that using two way encryption does not necessarily secure the password but it does provide a high level of obscuration. If encryting is overkill then maybe salting it might be the solution. Even if for no other reason than obscruring it from some employee at a shared hosting company. As it is now all the information is just sitting there in plain sight.

pandaPowder’s picture

that's true. Perhaps that alone is worth it. If nothing else, it will ensure that only the serious attacker gets it.

amontero’s picture

I was googling for a way to store one password entered in a module settings page and I've come with this:
https://groups.drupal.org/node/258513
Implementing one of those would avoid storing the passwords in cleartext. The site owner can harden the encryption by using a key stored in a file, away from the database.
Also found this nice reading:
http://drupalscout.com/knowledge-base/storing-private-information-secure...
I think I'll be using Encrypt module, since looks like the upcoming 2.x release will add an '#encrypt' FAPI property that might help me elsewhere in the future.

bwood’s picture

Issue summary: View changes

Another option would be to store the username/password in custom webserver environment variables. This is the direction that the Pantheon hosting service likes. Providing the ability specify the keys for theses variables in the admin UI in this module, would be a nice addition.

Chris Burge’s picture

Another option is to take advantage of the Encrypt module and/or the AES module.

Seems fairly straightforward:

Encrypt

  // Encrypt data.
  $encrypted_text = encrypt('some string to encrypt');
  // Decrypt data.
  decrypt($encrypted_text);

AES

  // Encrypt data.
  $encrypted_data = aes_encrypt("mydata");
  // Decrypt data.
  $decrypted_to_plain_text = aes_decrypt($encrypted_data);
wundo’s picture

Status: Needs review » Postponed (maintainer needs more info)
DamienMcKenna’s picture

Title: Stored password in the database is NOT encrypted! » Encrypt the stored SMTP password
Version: 7.x-1.0-beta2 » 7.x-1.x-dev
Category: Task » Feature request
Status: Postponed (maintainer needs more info) » Needs work

This is a feature request.

Neo13’s picture

Hello,

any progress on this?

DamienMcKenna’s picture

Another approach would be to just handle it in the site's settings.php file using normal PHP and maybe an external file that wasn't stored in the site's repository. Certainly something that could be handled outside of the module on an as-needed basis.

wundo’s picture

Status: Needs work » Closed (works as designed)
amontero’s picture

Status: Closed (works as designed) » Active

Is this solved in any version? If not, a more appropiate "closed won't fix" would be in order.
Storing the password in the db without any mechanism to encrypt it and keep the key outside of the db (as the suggested encrypt module allows) coud provide an acceptable level of security.
Also, a security notice in the project page warning users about insecure password storage would avoid a false sense of security, since the module even does SSL. It can even encourage others to fix it, perhaps.

wundo’s picture

Status: Active » Closed (works as designed)
wundo’s picture

Status: Closed (works as designed) » Active
wundo’s picture

Title: Encrypt the stored SMTP password » Warn the admin that the password for the SMTP server is stored as plain text

  • wundo committed ee62c64 on 8.x-1.x
    Issue #1834604 by wundo, Harold Villacorte, DamienMcKenna: Warn the...
wundo’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Active » Fixed

Security is always a matter of compromises. The question is always what threats do you want to protect against and what level of hindrance are you taking.

If the attacker got access to your Drupal database (or worst, root access to your server) you have more important things to worry than the SMTP password.

I've updated the description of the password field on the settings form to mention that this password it's going to be stored plain text together with other Drupal's variables.

Marking this as fixed.

AjitS’s picture

Shouldn't this be back ported to D7?

amontero’s picture

Status: Fixed » Needs work

The patch is worth porting, in fact.
However, I've checked the final looks of the message and there is a risk of distracted/unexperienced/unsuspecting users missing the warning at all. Suggestions:

  • Emphasize text
  • Warn about the risk in the project page (this is the most security-minded, IMO). Damien's suggestion could be linked/documented as a mitigation measure in the docs. That way is how I handle most secrets nowadays.

I agree on security being a matter of tradeoffs, but at least the warning signs should be as visible as possible.

I'm also aware that implementing encryption requires time, so I would propose to postpone the issue in case someone wants to tackle it someday, if interested. Instead of confusingly retitling and closing it as if there was no longer any security risk.

One does not need to hack into your DB to gain access to the stored password. A badly sanitized db dump (module does not provide any drush sanitize hook) stored as a backup or handed to developers could leak the password. Even if some attacker gains access to your backups, for instance, there's no need to make it easier to him/her.

Drupal's core, for instance, makes hard to retrieve user's passwords even in possession of a db dump by salting password hashes. You can avoid this happening by warning more visibly your users and providing workarounds, at least.

Tentatively marking as 'Needs work'. But I think the issue title should be reverted to original title, which still stands and a new issue opened to deal with the nice mitigation measure in the works (and its needed backport).

sic’s picture

guys - storing the password in plaintext is really a bummer. what about the suggestions made here?

this must also be backported to 7!

estoyausente’s picture

I have another appreciation. The password is stored as a plain text and... exported with Configuration Manager, converted to a yml file and pushed to the repository too easily

Can we save the password in another place? I don't know, exists the way to save the password and do not export it using a standard (simple configuration api or config entities?

wundo’s picture

You could set it inside settings.php, that is what I do.

estoyausente’s picture

Yes I know, @wundo. I did it when I saw, but I think that the module could be avoid it.

But thanks anyway for the suggestion.

joelsteidl’s picture

Another option is considering a dependency on the https://www.drupal.org/project/key module.

This allows the user to decide where they should put it (including in a file outside the webroot).

Chris Burge’s picture

There's an issue for the Just SMTP module that uses the Encrypt module to encrypt the SMTP password prior to storing it. It could be adapted for this module, as well: #2800401: Provide Integration with Encrypt Module. The Key module also provides Encrypt integration.

amontero’s picture

How about placing a warning in the project page linking to this issue while it is being addressed? Sounds the most cautious and responsible stop-gap measure, by now.
At least, let's warn people and let them know (and hopefully, help with) the current security compromises.

sgp913’s picture

Could we just save it in settings.php? If it's set there, remove the field. If it's not in settings.php, tell admin to set it there or leave the field for those that don't want to/care. Maybe the admin account shouldn't have access to this (assuming important) password.

I'm sure lots of devs are using Google Apps/other for $5 email with only one account and don't want to save this PW in plaintext.

Doing all this DB encryption work now seems like a major change, but the DB password is in settings.php anyway...seems like a simple addition to greatly improve security. I'd prefer to keep it locked down in a file somewhere instead of floating around in the DB (in any form). Someone gets that password, maybe they can take over your external account and do some real damage.

I'm not sure about the scope of settings.php and where those variables are available though, so it would need to be accessible only by this module. If someone is doing this already, please share your code so we can commit this.

Another option is just making a plaintext file and hiding it somewhere, letting the module read that file for the PW (like #26 had mentioned). I think any approach is better than leaving it plaintext like this. The best thing would be to implement multiple ways and let the dev decide which is preferable for them.

Ruslan Piskarov’s picture

Confirming.
> The password is stored as a plain text and... exported with Configuration Manager.

settings

And think I can be dangerous and security issue because someone can grep by "smtp.settings.yml" on github for example.

Ruslan Piskarov’s picture

FileSize
28.31 KB
esolitos’s picture

Priority: Normal » Major

From this video seems like there is an integration with the Key module already?

In any case the key imho the key should not go to the configuration management, it's really easy to accidentally commit this and expose your own key without realising it.

therobyouknow’s picture

wundo commented (#22): https://www.drupal.org/project/smtp/issues/1834604#comment-11739760

You could set it inside settings.php, that is what I do.

@wundo - how specifically did you do that? Please provide example code snippets.

From my examination of the module, it would appear that it doesn't retrieve from settings.php but rather from the config.

See also my issue here: https://www.drupal.org/project/smtp/issues/3016831

Tim Bozeman’s picture

@therobyouknow google bra...

xeM8VfDh’s picture

this is a very old issue, so I'm guessing it's not a priority

but I'm wondering if instead of adding a warning we could store the password in some other way altogether (not plaintext)