wpsharks / mail-remix

0 stars 0 forks source link

Email Parsing; Security Considerations? #5

Open jaswrks opened 9 years ago

jaswrks commented 9 years ago

I'm curious to know if enabling these features will allow for things like PHP evaluation, shortcode parsing, and Markdown parsing for just any email? Is there any additional syntax needed to enable this parsing, or is it just "on" if I enable it?

I'd suggest that there be some sort of additional syntax to enable these functionalities in a particular email. For instance, if I want PHP code evaluated, I can enable this in the options. However, in order to get PHP evaluated there should be some sort of wrapper needed in my email body.

<!--php-enable-->

Something along the lines of this. Otherwise, if a plugin that I'm running happens to send an email through with a PHP tag by mistake, or because some sort of user input contained a PHP tag... my site would open to a security issue. Or no?

2014-11-03_14-27-03


Even with a special token/wrapper being required, that may not always guard against every scenario. For instance, in Comment Mail we're going to have WordPress send emails w/ clips of a comment that somebody left on the site; i.e. user-supplied input is going to make it into emails. Of course, we are going to sanitize this data so PHP tags won't be in there. However, that may not be the case w/ every plugin, or in every possible scenario.

jaswrks commented 9 years ago

It might be best to simply enable these for the templates you provide; i.e. so that you control which emails are allowed to use this sort of additional parsing/functionality. Is that what you've done?

brucewrks commented 9 years ago

Right now I address this issue by allowing it to be turned on and off. If someone has a potential security hole here, it would be in their best interest to just turn off PHP execution. The execution is done within the core templating class, so that replacement codes can be used as variables in the PHP execution.

If someone needs to have a special filter on their emails to decide whether or not to execute PHP in their emails, they can do that by filtering the content itself via Must-Use Plugin. There are a few filters already built-in and I'll be adding more before I release the plugin.

The most I see necessary is a warning that there is the potential for a security hole if an admin enables PHP Execution on a site with user input being sent via email. What do you think?

jaswrks commented 9 years ago

The problem I see with this, is that even if I (as someone who knows the security implications this may have), decide to enable this; and then later I'm working to enhance my site with "this plugin and that theme", etc, etc... I can never be absolutely sure that user-defined content is not going to accidentally slip through in an email message, where these parsing routines may allow for some serious exploits.

I think there are many folks who will simply not recognize the security issues associated with turning things like this on; especially while they are also running BuddyPress, bbPress, Comment Mail, s2Member; or any other plugin that might make extensive use of wp_mail() in a creative way.


That said, the ability to parse Markdown, PHP, shortcodes; that's awesome! And, it can be awesome w/o security issues, so long as there are only specific emails where this is enabled, and not just a blanket thrown over all wp_mail() usage.

I'd suggest that you limit these parsers/filters to only the emails that you allow them to configure; or in the portions of the template where they have complete control over what makes it into the message body in those particular fragments.

Another reason to do this, is to avoid plugin conflicts. For instance, in the case of Markdown, if I enable Markdown globally, and then bbPress sends a message to somebody with asterisks in a title (or something like this); that's going to get converted to bold text inadvertently; i.e. it was not intended by the plugin for that to occur.

brucewrks commented 9 years ago

Gotcha. I'll leave this open and come back to this before the initial release.