marcantondahmen / automad

A flat-file content management system and template engine
https://automad.org
MIT License
617 stars 37 forks source link

With the default Apache configuration Automad is exposing files it shouldn't #52

Open michaellenaghan opened 2 years ago

michaellenaghan commented 2 years ago

Here is Automad's demo site. And here, for example, is the automad/package-lock.json file for that site.

Those are just text files, but .php files are also exposed. For example, if you constructed a URL to, say, automad/tests/Core/FileUtilsTest.php, the tests would (try to) run. (I didn't go looking for particularly problematic .php files.)

On Apache these issues can be resolved by tightening up the .htaccess file. For inspiration, here is the .htaccess file for Gravity, and here is the .htaccess file for Kirby — two flat-file CMSes. (Here is the .htaccess for ProcessWire. ProcessWire is a traditional CMS rather than flat-file, but the .htaccess file is carefully thought out and well commented.)

Tightening up the .htaccess file would be something of a quick fix. Even better — this is a "someday" thing! — would be to change the directory structure so that static files are clearly separated from non-static files.

My Apache skills are rusty, but if you like I can take a first crack at the required changes.

P.S. Automad is an incredibly impressive piece of work. So many good ideas, so much attention to detail.

michaellenaghan commented 2 years ago

Here's my first pass at a solution:

RewriteEngine On 

RewriteBase /

RewriteRule "^$" "index.php" [L]

RewriteCond "%{REQUEST_FILENAME}" !-d
RewriteCond "%{REQUEST_FILENAME}" !-f
RewriteRule "^" "index.php" [L]

RewriteCond "%{REQUEST_FILENAME}" -d
RewriteRule "^" "index.php/_" [L]

RewriteCond "%{REQUEST_FILENAME}" -f
RewriteCond "%{REQUEST_FILENAME}" "!(index\.php|robots\.txt|\.(css|gif|ico|jpeg|jpg|js|otf|png|svg|ttf|woff|woff2))$"
RewriteRule "^" "index.php/_" [L]
michaellenaghan commented 2 years ago

Two other thoughts.

First, another option would be to send everything to Automad, whether there's a corresponding physical file or not, and let Automad decide whether or not to present something or 404. On the one hand, that would mean a greater load on Automad. On the other, that would allow Automad to control caching headers and such. But then Automad would also need some way to control which physical files it should serve and which it shouldn't. (It currently has something like that for uploads, but not for "downloads.")

Second, not sending everything to Automad means that generated files shadow physical files — and thus there can't be a generated file that matches a physical file (or directory) name. Or, to be more precise, a generated file whose slug matches a physical file (or directory) name. Automad seems to already be aware of that; it appends numbers to slugs whenever there's a collision.

michaellenaghan commented 2 years ago

Here's my second pass:

RewriteEngine On 

RewriteBase /

RewriteCond %{REQUEST_FILENAME} -d
RewriteRule . index.php/_ [L]

RewriteCond %{REQUEST_FILENAME} -f
RewriteCond %{REQUEST_FILENAME} !(^|/)\.well-known/
RewriteCond %{REQUEST_FILENAME} !(index\.php|robots\.txt|\.(css|gif|ico|jpeg|jpg|js|otf|png|svg|ttf|woff|woff2))$
RewriteRule . index.php/_ [L]

RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule . index.php [L]
marcantondahmen commented 2 years ago

Hi there and thank you for the kind and detailed feedback. I will carefully review it. Just some quick comments:

  1. Unfortunately the current file structure can not be changed due to limitations of some cheaper hosting providers. The problem is that some companies don't allow the user to change the document root in their web space in order to move the PHP files out of the public directory. In such cases, it is only possible to install apps in the public directory.
  2. I fully understand the approach to tune the .htaccess file in order to restrict access to source files. However, this only applies to Apache servers and in some extend to LiteSpeed. It doesn't help on Nginx servers. In order to restrict direct access to PHP files on all kind of servers, all PHP source files will throw a permission denied error independent from the server config in case the entry point is not the index.php or the file is accessed directly in the browser. The unit tests are the only exception here. I don't think that is problematic since that files are simple class files that don't do anything else that containing tests. PHPUnit itself is not part of the installation, but would be required to actually do something.

I hope this helps you a bit and aside from the above mentioned, I like your input to improve the .htaccess. It just should be as compatible as possible for everyone.

Cheers!

michaellenaghan commented 2 years ago

I'll respond to your comments separately; here's my third pass, which is now in production:

<IfModule mod_rewrite.c>

RewriteEngine On 

RewriteBase /

RewriteCond %{REQUEST_FILENAME} -d
RewriteRule . index.php/_ [L]

RewriteCond %{REQUEST_FILENAME} -f
RewriteCond %{REQUEST_FILENAME} !(^|/)\.well-known/
RewriteCond %{REQUEST_FILENAME} !(^|/)index\.php$
RewriteCond %{REQUEST_FILENAME} !(^|/)robots\.txt$
RewriteCond %{REQUEST_FILENAME} !(^|/)sitemap\.xml$
RewriteCond %{REQUEST_FILENAME} !\.(css|gif|ico|jpe?g|js|otf|png|svg|ttf|woff2?)$
RewriteRule . index.php/_ [L]

RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule . index.php [L]

</IfModule>

I plan to make further changes — e.g., add support for caching. But I think this represents the core of what's needed to serve files safely. That's all I wanted to do here.

michaellenaghan commented 2 years ago

Hello, Marc.

I've come much further along in my Automad journey. Very impressed.

On your comments:

    /automad
        /system
        /user
        /temp

I'm not proposing those actual names, or that specific structure. :-) But the intention would be making the system easier to protect and maintain — e.g., there's never any web access to /system, /user is what you back up, etc. I can explain more, but I'm not sure it's worth spending time on this. At least, not right now.

I should add that the ability to support a more secure directory structure is not the same as shipping with a more secure directory structure. Bolt, for example, ships with a less secure structure, but allows those who care to make it more secure — e.g., by moving /system out of /public.

Let me know if you'd like to have a broader conversation about this. (By "this" I mean security, not directory structure; related but different.) I don't want to drag you into a conversation you weren't looking for.

poetaster commented 1 year ago

I'll respond to your comments separately; here's my third pass, which is now in production:

I'm running tests with this which works well. I expanded the list of static files to include ogg / mp3 /mp4 files.

HanM23 commented 1 year ago

That's completely crazy to consider that exposing files is not so important, even json files or unit tests.

marcantondahmen commented 1 year ago

I expect professional and respectful wording here.

If you find it crazy, just don't use it. Or even better, use your valuable time to create something on your own.

Regarding the actual issue, an average developer can see that:

  1. Tests are excluded from distributions
  2. All frontend source files are excluded from distributions
  3. Other source files are not directly accessible but only through the main entry point
  4. On Apache, source files are protected with .htaccess files, separate from the main one
  5. The entire structure can be found as well in many other little projects such as wordpress, that have to work out of the box also when there is no control about the location of the document root directory on shared hosting environments

Everybody knows that it is not ideal to have the application living in the document root. There is no need to state the obvious. However it is more important to understand the reason behind that in order to be able to judge it and make it work under such conditions.

I simply can not get why it is so difficult to understand that different projects target different environments and scenarios.

And again, constructive feedback or real issues are always welcome. If you just want to complain about other people's voluntary work or simply troll around, go somewhere else. You're wasting my and other people's time.

poetaster commented 1 year ago

Just to chime in with my support for us NOT exaggerating, many well known php apps are much more vulnerable (looking at you wordpress) when installed as they MOST OFTEN are. Some mitigations are trivial. There is nothing to panic about with Automad. I make additional effort to set file access rights (set root ownership, read only, where possible) but that's ordinary operating procedures. Thanks for your efforts @marcantondahmen

marcantondahmen commented 1 year ago

Thanks, I appreciate that!

And there is no doubt that moving things out of the public directory is by default a good idea. I am personally more concerned about the way those issues are communicated here by some people. Such behavior is also present in other issues. I can see that many people forget, that some open-source developers spend a huge portion of their private time, at night and on weekends, to create software that is free to use without any kind of financial compensation. Respect, a kind way of communication and constructive critics are the minimum one can expect from the community.

But back to the issue. Moving certain directories out would have two major consequences that directly come into my mind:

  1. A more complex installation process for non-professionals that simply can't rewire a shared hosting site
  2. Packages can not be shipped as they are now since also here the templates should be separate from the dist files

In fact there is not really a security issue here. However, it would be worth trying to simply moving the directories in question somewhere above the public directory and then symlinking them back. I didn't try it yet, but I can imagine with some little adjustments to the config and possibly the .htaccess the process is quite straight forward.

poetaster commented 1 year ago

I am personally more concerned about the way those issues are communicated here by some people.

Agreed. I maintain a number of Apps 'for free' and am happy to do so. Input is also important for me. But it needs to remain on point.

And 1. is clear. It's why I mentioned wordpress which, if you install it as many do by simply downloading a zip will not yeild a secure install. Packages in, for instance, debian mitigate but are beyond the means of most people to manage. So, I get it.

I didn't try it yet, but I can imagine with some little adjustments to the config and possibly the .htaccess the process is quite straight forward.

I could do some testing in this direction. Question is, the 2 branch?

michaellenaghan commented 1 year ago

This issue isn't about directory structure, it's about the fact that the .htaccess file that Automad ships with exposes files that shouldn't be exposed.

The initial comment demonstrated access to automad/package-lock.json, but pointed out that the bigger problem was that arbitrary PHP files that weren't intended to be executable were, in fact, executable.

I demonstrated that in #59 by executing a non-Automad PHP file in the vendor tree. I chose a harmless example on purpose, but my point was that ~149 other PHP files were exposed the same way.

The reason I brought up directory structure in connection with this particular issue is that Automad's current structure mixes different kinds of content — system and user, executable and non-executable — under the same tree. That makes it harder to properly secure Automad.

Here are two examples of what I mean.

First, in terms of file permissions, what you'd really like to do is say: This tree of system files shouldn't be modifiable by the web server; I'll give them read-only permissions. This tree of user files should be modifiable by the web server; I'll give them read-write permissions.

Second, in terms of Apache permissions, what you'd really like to do is say: This tree of files should never be directly exposed by the web server; I can block all URLs that point within it. This tree of files is directly exposed by the web server; I can allow URLs that point within it.

There's a connection between those two points: you really don't want read-write user content to be executable — and, worse, also directly exposed through the web server.

Take a look at part of my proposal:

RewriteCond %{REQUEST_FILENAME} -f
RewriteCond %{REQUEST_FILENAME} !(^|/)\.well-known/
RewriteCond %{REQUEST_FILENAME} !(^|/)index\.php$
RewriteCond %{REQUEST_FILENAME} !(^|/)robots\.txt$
RewriteCond %{REQUEST_FILENAME} !(^|/)sitemap\.xml$
RewriteCond %{REQUEST_FILENAME} !\.(css|gif|ico|jpe?g|js|otf|png|svg|ttf|woff2?)$
RewriteRule . index.php/_ [L]

Notice how I'm allowing access to css, etc. files anywhere within the entire directory tree. That was working around the fact that directory trees weren't segregated by purpose. Something like this would be much better:

RewriteCond %{REQUEST_FILENAME} -f
RewriteCond %{REQUEST_FILENAME} !(^|/)\.well-known/
RewriteCond %{REQUEST_FILENAME} !(^|/)index\.php$
RewriteCond %{REQUEST_FILENAME} !(^|/)robots\.txt$
RewriteCond %{REQUEST_FILENAME} !(^|/)sitemap\.xml$
RewriteCond %{REQUEST_FILENAME} !(^|/)\assets/
RewriteCond %{REQUEST_FILENAME} !(^|/)\media/
RewriteRule . index.php/_ [L]

Now I'm allowing access to a small set of files in the root, plus all files within assets/ and media/. But I'm allowing no direct access at all to anything anywhere else. Like, for example, vendor/. Or package-lock.json.

The reason that non-executable files like composer.json or package-lock.json matter is that they expose significant implementation details. Those details make it easier for people to take advantage of known vulnerabilities.

This is, without question, a security issue.

HanM23 commented 1 year ago

I expect professional and respectful wording here.

If you find it crazy, just don't use it. Or even better, use your valuable time to create something on your own.

Regarding the actual issue, an average developer can see that:

  1. Tests are excluded from distributions
  2. All frontend source files are excluded from distributions
  3. Other source files are not directly accessible but only through the main entry point
  4. On Apache, source files are protected with .htaccess files, separate from the main one
  5. The entire structure can be found as well in many other little projects such as wordpress, that have to work out of the box also when there is no control about the location of the document root directory on shared hosting environments

Everybody knows that it is not ideal to have the application living in the document root. There is no need to state the obvious. However it is more important to understand the reason behind that in order to be able to judge it and make it work under such conditions.

I simply can not get why it is so difficult to understand that different projects target different environments and scenarios.

And again, constructive feedback or real issues are always welcome. If you just want to complain about other people's voluntary work or simply troll around, go somewhere else. You're wasting my and other people's time.

I expect professional and respectful wording here.

If you find it crazy, just don't use it. Or even better, use your valuable time to create something on your own.

Regarding the actual issue, an average developer can see that:

  1. Tests are excluded from distributions
  2. All frontend source files are excluded from distributions
  3. Other source files are not directly accessible but only through the main entry point
  4. On Apache, source files are protected with .htaccess files, separate from the main one
  5. The entire structure can be found as well in many other little projects such as wordpress, that have to work out of the box also when there is no control about the location of the document root directory on shared hosting environments

Everybody knows that it is not ideal to have the application living in the document root. There is no need to state the obvious. However it is more important to understand the reason behind that in order to be able to judge it and make it work under such conditions.

I simply can not get why it is so difficult to understand that different projects target different environments and scenarios.

And again, constructive feedback or real issues are always welcome. If you just want to complain about other people's voluntary work or simply troll around, go somewhere else. You're wasting my and other people's time.

I do not use it bro...i was about to use it then i changed my mind thanks to @michaellenaghan You just do not accept his comments. But hey push back is human, we are not machines, right.

My opinion is you are committing a huge error by not accepting those comments. @michaellenaghan is spending substantial time to help you....whereas he rather would prefer fishing or dancing instead.

poetaster commented 1 year ago

First, in terms of file permissions, what you'd really like to do is say: This tree of system files shouldn't be modifiable by the web server; I'll give them read-only permissions. This tree of user files should be modifiable by the web server; I'll give them read-write permissions.

As I've mentioned, this is standard operating procedure. Deny the web server access, make certain the files are not writable. That's mitigation 1 ( supported by an inotify demon that watches the files to ensure that they are not changed).

Second, in terms of Apache permissions, what you'd really like to do is say: This tree of files should never be directly exposed by the web server; I can block all URLs that point within it. This tree of files is directly exposed by the web server; I can allow URLs that point within it.

As I mention above, I implemented your suggested .htaccess (more or less) and it works fine, so it's a reasonable suggestion to reduce access where it's not needed.

poetaster commented 1 year ago

My opinion is you are committing a huge error by not accepting those comments. @michaellenaghan is spending substantial time to help you....whereas he rather would prefer fishing or dancing instead.

I'm not sure how much experience you have with PHP apps, apache or security, but you are not helping with this discussion. The developer has demonstrated that he is willing an able to engage in a constructive dialog. You are simply dissing without contributing anything of consequence.

poetaster commented 1 year ago

This issue isn't about directory structure, it's about the fact that the .htaccess file that Automad ships with exposes files that shouldn't be exposed.

And to put your comments in context, which I not only appreciate but have even put to the test, these comments are from last summer. Active development is on a new branch and people, especially solo devs often have time constraints.

I wonder why, other than this thread there are no pull requests that we / I could merge and test?

HanM23 commented 1 year ago

My opinion is you are committing a huge error by not accepting those comments. @michaellenaghan is spending substantial time to help you....whereas he rather would prefer fishing or dancing instead.

I'm not sure how much experience you have with PHP apps, apache or security, but you are not helping with this discussion. The developer has demonstrated that he is willing an able to engage in a constructive dialog. You are simply dissing without contributing anything of consequence.

You reaction is so predictable. I say nothing about your competencies, so do not make allusion on mines and frankly, we really do not care. You mean that as long you do not have a real proof that someone is an expert ...you will not listen to him....strange reaction...this is not really constructive as you may say. My first message was not disrespectful at all, just you do not accept remarks even those from @michaellenaghan....this is crystal clear in the first replies you sent to him. What i noticed is that thanks you me, debates on security on your stuff have started again....yes dude, thanks to me!

poetaster commented 1 year ago

My opinion is you are committing a huge error by not accepting those comments. @michaellenaghan is spending substantial time to help you....whereas he rather would prefer fishing or dancing instead.

I'm not sure how much experience you have with PHP apps, apache or security, but you are not helping with this discussion. The developer has demonstrated that he is willing an able to engage in a constructive dialog. You are simply dissing without contributing anything of consequence.

You reaction is so predictable. I say nothing about your competencies, so do not make allusion on mines and frankly, we really do not care. You mean that as long you do not have a real proof that someone is an expert ...you will not listen to him....strange reaction...

Have you read my reactions to the last production ready .htaccess files from @michaellenaghan ? He made constructive comments, I tested them, confirmed they were usable and on goes the constructive part of the discussion.

this is not really constructive as you may say. My first message was not disrespectful at all, just you do not accept remarks even those from @michaellenaghan....this is crystal clear in the first replies you sent to him. What i noticed is that thanks you me, debates on security on your stuff have started again....yes dude, thanks to me!

My substantial confirmation of @michaellenaghan .htaccess changes preceded your 'comment' which is nothing more than hand waving. The first comment in this thread since June of last year is mine. There is other discussion in other tickets, some of which has led to changes in the code, some of which is not relevant.

I'd like to help work on this code base, and that requires a focus on actual code points. This discussion is diverging from the actual codebase (1 vs 2) and is not helpful. The original points have been registered, the developer is in the loop and responsible (working on the codebase).

What I'm missing here is actual commitment to working on resolving the issues in a professional manner. That means, forking, and making PRs of improvements in a responsible manner. Although, it's ok to have posted the .htaccess here, from my pov, it's much more useful for a developer to have the contribution made in a PR that can be examined and potentially merged with less friction than this back and forth.

poetaster commented 1 year ago

The reason that non-executable files like composer.json or package-lock.json matter is that they expose significant implementation details. Those details make it easier for people to take advantage of known vulnerabilities.

This is, without question, a security issue.

And, although I think your suggestions are all reasonable, these are arguments for security through obscurity.

Any real attacker will eventually find a way back to automad as the generator (you don't address THAT issue at all) and then find the source, right here. And hence find the composer files, etc.

Anyway, I'm going to move on to trying the version 2 branch which may very well sink all of these comments into the domain irrelevancy.