Comments

yannisc’s picture

I put some watchdog messages in the _echo_access function to check what gets wrong. It appears that $_REQUEST['title'], $_REQUEST['content'] and $_REQUEST['theme'] are set, but $access = cache_get($key) returns false.

I don't know what that means.

sanderjp’s picture

Same here..

ymdavoust’s picture

same issue for me!

pillarsdotnet’s picture

Version: 7.x-1.8 » 7.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new1.53 KB

Perhaps your cache implementation is not working?

The echo_themed_page() function contains the following cache_set() call:

  $key = sha1($title . $content . $theme);
  cache_set($key, $key, 'cache', REQUEST_TIME + ini_get('max_execution_time'));

Then the echo_access() function merely performs a matching cache_get():

  $key = sha1(
    $_REQUEST['title'] . $_REQUEST['content'] . $_REQUEST['theme']
  );
  if ($access = cache_get($key)) {
    if ($access->data == $key) {
      return TRUE;
    }
  }

Hmm... This check will also fail if you have multiple Drupal servers behind a front-end round-robin proxy, and they do not share a common cache implementation.

Gonna add a sanity-check echo_requirements() function.

damien_vancouver’s picture

I had this problem too. XDebug wouldn't work in the access callback (because it is a drupal_http_request from code, not from my local browser). But some PHP error_logging and standalone PHP page testing gave me the following (confusing) results:

  • In echo_themed_page(), the cache key is being set with an expiry of REQUEST_TIME + ini_get('max_execution_time'). However when I assign ini_get('max_execution_time') to a variable and then echo it.. it's always 0. (????) But, A standalone PHP test page shows ini_get('max_execution_time') works.
  • the cache_get($key) in the access callback is receiving FALSE as a return value. By looking in the database I can clearly see the key row (with my extended 300 sec timeout) in the "cache" table. My standalone test PHP page can retrieve cache_get($key) no problem using the same string for the key.

I'm currently at a loss to explain either of these behaviors.

As a workaround for now, I just added "return TRUE;" to the top of function _echo_access(). I will try some more testing next week when I have more time, and see if I can't narrow down what is causing the above. Note that if you put a return TRUE; in the access callback, you are opening yourself up to some nasty security issues. For example: Someone using /echo to echo back a fake login form using your site's current theme, that pointed to a malicious destination. (a phishing attack, but using your servers to make sure it looks exactly right). So beware and do not use that workaround as a permanent solution.

I've attached my patch to work around the ini_get issue, which might be useful because "max_execution_time" defaults to 0 or -1 in most CGI (FCGI as well?) php.ini's. So if PHP returned 0 or -1, that would be a nonsensical value and echo should probably use a sane default instead. This patch did not fix the problem for me on its own though!

I'll report back here when I find out anything further. I have plans to debug this properly next week, after a looming Monday demo deadline.

damien_vancouver’s picture

StatusFileSize
new1.07 KB

@pillarsdotnet,

I tried your patch from #4, hoping my environment would at least fail the requirements test.

But, the test worked and it passes requirements, reporting that the cache system is working. This matches my testing in a standalone PHP page (see comments in #5). The failure only seems to occur inside the access callback which answers the drupal_http_request from echo_themed_page().

Attached is a re-roll of your #4 with a missing <?php opening tag added and no other changes. (The missing tag was causing the text of the .install file to show up at the top of pages).

If we can nail down exactly what is broken, we can then improve your test to hopefully catch what is really wrong. Or if it turns out to be something else (like a bug introduced in recent 7.x versions of Drupal, as I am starting to suspect), then the test is fine and is likely something that should be included as-is.

Has anyone on this thread got it working for them after it was failing?

pillarsdotnet’s picture

StatusFileSize
new2.43 KB

Thirty seconds ought to be enough for anyone. (tm)

Are you using DrupalDatabaseCache or some other DrupalCacheInterface implementation?

Are successive queries handled by the same PHP instance or by separate instances?

Does your hostname resolve to the PHP server or to a caching/proxy front-end?

damien_vancouver’s picture

EDIT: removed duplicate posts, Drupal.org having problems today.

damien_vancouver’s picture

I'm using DrupalDatabaseCache with a single instance and no proxies. One thing it might be is that I did have memcache running on this Drupal install before. I've since disabled the module and removed the settings.php cache entries. However, that's definitely near the top of my list of suspects. I'm not sure if I actually ran the uninstall, as I was planning on turning it back on later...

Just tried the uninstall of memcache / memcache_admin and.... YES!!!!! That was it!! Working now.

Then this is looking like it's really a memcache module issue instead - if that thing is disabled it shouldn't be interfering with normal cache operation. I will try and reproduce that failure on its own and then I'll start an issue there if there isn't one already.

Ideally we could have the cache check in requirements detect that failing configuration, if we can detect that. I can use a database dump from my production server to recreate that configuration, and I'll see if I can figure that out. Meanwhile I don't see any reason to hold up committing #7. I'll submit a patch here for a better test if I can work one out, and also a crosslink to the eventual memcache issue.

Thanks for your help and the enhancements!

damien_vancouver’s picture

EDIT: Please ignore double post.. Drupal.org having issues today.

damien_vancouver’s picture

Sorry for that triplicate last post. Seems some of those error pages were actually saving!

Sadly, the problem is still here. I think what actually happened was my memcache uninstall did some kind of cache clearing that fixed it temporarily. I tried clearing all caches right before the page hit that sends the email, and tried again, and then I got a new strange result: An email back with the contents only, as if I wasn't using the echo module at all. So no access denied, but all the cached site theme stuff was suspiciously missing.

Anyway I'll test this more next week, starting with a fresh D7 install, and work my way back to where I am now with it failing. That will help us narrow it down further.

*rolls the "Mission Accomplished" banner back up for now, and puts it back in its box*

pillarsdotnet’s picture

StatusFileSize
new2.8 KB

Here's a slightly more robust approach.

damien_vancouver’s picture

Nice one! Patch #11 applies cleanly, but shows it passing. I stepped through in XDebug and verified to myself the full HTML was returning with no Access Denied in there anywhere.

So I'm certain something else I have installed or something htmlmail is doing must be causing it. I wouldn't spend any more time on it until I chase down what that is, since it's clearly working in a simple use case. Thanks again for your expedient help so far!

pillarsdotnet’s picture

Status: Needs review » Postponed (maintainer needs more info)

Oh, well; I guess it won't hurt, even if it doesn't help.

Released 6.x-1.9 / 7.x-1.9 / 8.x-1.9 with the patch from #11 applied.

Setting status back to "Needs more info" until you (or someone else) gets around to testing further.

tomgf’s picture

I am using this version (7.x-1.9) and still having this problem (sometimes…).

I also had memcached installed (v. 7.x-1.0), which now I have disabled…

Any further advanced…?

pillarsdotnet’s picture

If you have any suggestions as to how I can rewrite the _echo_access() function to reject requests from outside hosts (including other virtual hosts on the same shared-hosting server), I'd love to hear them.

I realize that depending on a working cache system is not an ideal solution, but I've been unable to come up with a better one.

tomgf’s picture

What about a configuration page where you can set a valid_host variable…?

I have written a patch with these changes:

  1. a configuration page where you can set a “valid hostname”
  2. a function that returns the “actual hostname”
  3. the way you allow / deny the use of the module: now it's comparing the values coming from #1 and #2

I think this could work.

tomgf’s picture

Looking at the patch, I have noticed two things:

  1. the variable could have a more pertinent name: echo_valid_hostname
  2. it's necessary to call hook_uninstall() to get rid of the variable after uninstalling

I have attached here a new version of the patch with these two changes.

pillarsdotnet’s picture

You have totally misunderstood the problem.

Please read the security report at https://security.drupal.org/node/61334.

In short, your changes remove the existing protection against XSS attacks.

The $_SERVER['HTTP_HOST'] variable reports the internal name of the server executing the current script. OF COURSE that will be the same server that the REST of Drupal is in, unless you've inserted Echo module code into Joomla, for instance.

The security concern is whether the request ORIGINATED on the same host that is SERVING it. If you can guarantee that no other website is sharing the same IP address, you can check whether $_SERVER['REMOTE_ADDR'] is equal to $_SERVER['SERVER_ADDR'].

However, if I released code like that, I'd have a Drupal Security Advisory published against my module, warning that it is insecure in a shared-hosting environment.

Unfortunately, if cache_clear_all() runs during the fraction of a second between cache_set() and cache_get(), then you'll get an Access Denied error. No way around it that I can see. Setting a minimum cache lifetime won't even help.

This is a known issue and is being worked on.

Unfortunately, a real fix would require a dedicated table just for holding Echo tokens. And I'd have to implement my own cache clearing logic.

(sigh) This was supposed to be such a beautifully simple module.

tomgf’s picture

I am sorry if I misunderstood the problem. I have attempted to help with a solution (that obviously won't work).

I thought that according to what you've explained as “to reject requests from outside hosts”, one could to set an option specifying which is the valid hostname, leaving everything else in the outside world. According to that, what I have tried to proposed was to check $_SERVER['SERVER_NAME'] against this configured value stored by the administrator on the DB as a variable. After your comment, I understand that the $_SERVER variables, either SERVER_NAME or HTTP_HOST can be also spoofed… (I am not authorized to access https://security.drupal.org/node/61334, though).

Using the CACHE_PERMANENT option –that won't be clean unless explicitly told– wouldn't work…?

Extra note: in my attempt to produce this patch I have noticed that the git command git clone --branch 7.x-1.x http://git.drupal.org/project/echo.git is pulling a version that it's not including echo.install

tomgf’s picture

I am giving it another try.

In this case, I have added a particular cache bin for this module. It's a work in progress –as there is no garbage collector and so– but maybe this provides a better approach.

And at least this cache is not emptied with the other ones. Actually, so far it's not emptied at all… (sth that I could work on if this is the way to go)

pillarsdotnet’s picture

StatusFileSize
new7.86 KB

Here's a copy of the security report.

kevinquillen’s picture

Had this same problem. Emails would be fine for a period in time, then clearing Varnish/site cache would result in any outgoing email having Access Denied page inside of it instead.

I simply threw 'return TRUE' at the start of _echo_access() to get around this for now. Its being used on a production site that sends a few dozen emails a day.

bsztreha’s picture

Same problem, and cache working:
Cache system - Working
The Echo module requires a working cache system.

applicity_sam’s picture

I have also seen this and spent the morning tracking it down. I think the issue is as follows:

A module on your site creates a transaction using db_transaction. (in my case it was commerce).
echo calls cache_set which puts the data into the database that echo needs to check access control.
As the transaction is open this is not committed.
A sub request is spun up which starts a new drupal process which is then trying to read from the cache. At this point the second process cannot see the cache data as it hasn't yet been committed. So it returns an access denied.
The original drupal process completes and the the data is then visible in the cache.

In my scenario this only breaks if I have something in my cart, as this causes an order to be loaded which holds a transaction open. I am going to log that as an issue on commerce.

mlecha’s picture

As soon as I apply a theme in Step 2 of htmlmail config:

404 Not Found

The server can not find the requested page:

dev.cultureworks.ca/echo (port 80)

However "No Theme" works fine.

Using Commerce Kickstart 7.x-1.11 install profile.

Site status reports "Cache system Working".

Adding "return TRUE;" to _echo_access() doesn't work for me.

jos_s’s picture

I have this issue too, but it seems to depend on the server the website is running on. I have a local development server that gives no problems, but when I copy the site to the production server (not mine), I get the Access Denied messages in the html mail. (using echo 7.x-1.9)

Edit 17 January:
Does anybody have any ideas how this can depend on the server where the Drupal site is installed? I have spent days looking for a solution, but haven't a clue where to look for. This is a show stopper for the website I am building.

kevinquillen’s picture

Lets back up for a second- what is the reason behind the /echo callback? If its just to theme the email message with the site email, something seems a bit off here to go through a lot of access checks.

pillarsdotnet’s picture

@#28 -- It is not possible for Drupal to switch themes without a callback. A callback without an access check was deemed unsafe by the security team.

jos_s’s picture

After doing some more research I am wondering if issues with drupal_http_request() could be causing this (see http://drupal.org/node/1664784).

If no HTTP request can be made from the server, the echo will return the "Access denied" message in stead of the HTML newsletter.

Is there a way around this?

jos_s’s picture

I finally found what caused my Access Denied errors. It appeared that the production server had Apache mod_security.c activated. Adding the following lines to .htaccess solved it:

<IfModule mod_security.c>
  SecFilterEngine Off
  SecFilterScanPOST Off
</IfModule>

Found the solution by studying the server log files and then finding this page: http://drupal.org/node/109058

TamB’s picture

Version: 7.x-1.x-dev » 7.x-1.9

I seem to be still having this problem with 7.x-1.9. Wondering if anyone has found a viable solution as yet.

What are the security implications of #31. Is it a long term solution or can it be used only as a stop gap?

jacob.embree’s picture

Issue summary: View changes
StatusFileSize
new731 bytes

#25 is correct. This patch doesn't fix anything but will log a warning if the external request returns a 403 and a DB transaction is open.

letapjar’s picture

Why can't echo store a variable in the db i.e. 'echo_key' and then use that variable to munge the
sha1 calls in both the echo_themed_page function as well as in the access callback?
When making the httprequest - you pass along the munged token - then re-calculate the token in the access callback and compare it to what was passed in.

This requires that during install - the 'echo_key' variable gets set, but it gets around the cache clearing issue and I *think* it serves the purpose.

letapjar’s picture

Here's a patch that implements what #34 is talking about. I could be missing the entire issue here. But this avoids the cache altogether.

letapjar’s picture

Re-rolling patch from #35 to 1) get rid of cache_set as it's not needed
2) add in hook_uninstall to delete the echo_key variable.

jacob.embree’s picture

The issue is not that the cache is cleared in between but rather that any changes to the database during a transaction, either in a cache table or in the variables table, do not take effect until the transaction is committed.

Need a way to authenticate access to /echo that does not depend on the state of the database.

letapjar’s picture

Re #37 - Right I get it - db write during a transaction won't be saved until the transaction closes.

The approach in #36 totally avoids that. You can still read from the db during a transaction.

The echo_key I propose is bascially a secret key much like the app_secret you get when creating an app for facebook or twitter. It is set once when the module is installed and does not need to be changed later.

During the echo_themed_page() and _echo_access() callbacks - there is no need for a db write - instead we use a db_read of the secret key and on each side separately calculate the digest (passed in the internal http_request as the "token").

In _echo_access() - the only way that $calculated_token will equal the token passed in with the http_request is if they both used the same echo_key. Since the echo_key is never publicly exposed this serves the purpose of making sure requests came from the site itself and not from the outside.

There's still the separate issue that the module is not using rawurldecode() in the _echo_access() function. This should be fixed and I've filed a separate issue for that.

These changes are working nicely for my sites.

jacob.embree’s picture

Status: Postponed (maintainer needs more info) » Reviewed & tested by the community

My apologies. echo-access_denied_in_email_sent-1448278-36.patch in #36 works. Next time I'll test before making a silly comment.

alauddin’s picture

can confirm patch from #36 works...D7.34 & Echo 7.x-1.9

joran lafleuriel’s picture

#36 saved my life ! Thanks a lot.

All html emailing process was corrupted before I patched Echo 7.x.19 with this.

I succeed to send html emails to gmail with this conf :

Drupal 	7.34
Omega 3 subtheme...
Chaos tools 7.x.1.7
Display Suite 7.x.2.7
Boost	7.x.1.0
Htmlmail 7.x-2.65
Mailsystem 7.x-2.34
Simplenews 7.x-1.1
Echo 7.x.19 with patch #36
Emogrifier module  7.x-1.18 [ does not work if earlier ]
Librairie emogrifier : old version [ only emogrifier.php, not in 'Classes' folder ] [ does not work if earlier ]

  • Nafes committed e51610c on 7.x-1.x authored by letapjar
    Issue #1448278 by pillarsdotnet, tomgf, damien_vancouver, letapjar,...
Nafes’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks for the patch, letapjar!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

timb’s picture

Was this really committed? I see that &x-1.9 hasn't been updated since March 2012.

markusd1984’s picture

Is this the same error as "401 Unauthorized"?

This server could not verify that you are authorized to access the document requested. Either you supplied the wrong credentials (e.g., bad password), or your browser doesn't understand how to supply the credentials required.

I tried patch #36 but unfortunately it did not resolve this.

Html mails without an Email Theme work fine. Any advise how to debug / troubleshoot please?

FYI - I am using SMTP module with HTML MAIL (& SSO with LDAP over HTTPS).