11ty / eleventy

A simpler site generator. Transforms a directory of templates (of varying types) into HTML.
https://www.11ty.dev/
MIT License
17.2k stars 494 forks source link

RFC: Restrict Eleventy’s access to directories outside of its realm #368

Closed kleinfreund closed 3 years ago

kleinfreund commented 5 years ago

Currently, Eleventy does not restrict access to directories outside of its realm. This means that both input and output directory can be outside of Eleventy’s project directory (i.e. the place from where you run eleventy, a.k.a. the command directory). The following is possible:

eleventy --input=../private --output=../other-project

This is dangerous and a potential security risk for the following reasons:

I propose disallowing Eleventy from reading/writing outside of its root/project/command directory.

zachleat commented 5 years ago

yeah, I’m on board with this. --input is an easier sell than --output I think. But I’m on board with both.

Would this also require a review of any templating include functionality?

kleinfreund commented 5 years ago

Algorithm for creating safe Eleventy paths:

  1. Normalize and strip leading double-dot segments from path
  2. Use Node.js’ fs.realpath to check …

    • if the path doesn’t exist, throw an error.
    • if path is outside of Eleventy’s realm, throw an error.

    Otherwise, return the absolute path.

This algorithm allows usage of symbolic links as long as they point inside Eleventy’s realm while restricting access to any directory outside of it.

Node.js implementation (draft) for step 2:

const fs = require('fs');

function safePath(path) {
  try {
    const realPath = fs.realpathSync(path);

    if (realPath.startsWith(process.cwd())) {
      return path;
    }

    throw new Error(`Path ${realPath} is outside of Eleventy’s project directory.`);
  } catch (error) {
    throw new Error(error.path ? `Path ${error.path} does not exist.` : error);
  }
}

Heads-up:

Implementing this for EleventyFiles (i.e. the includes directory) breaks a bunch of tests because a lot of the directories inside test don’t have the default _includes includes directory (and also the default _site output directory). Currently, Eleventy doesn’t error when these directories don’t exist.

Should this be a general error? Consequently, should the test directories have these directories?

zachleat commented 5 years ago

@kleinfreund _includes should be completely optional unless you try to use it for something.

kleinfreund commented 5 years ago

@zachleat I see. This makes sense and I should’ve thought of it myself. Requiring an includes directory would break a lot of functionality and documentation (i.e. plain eleventy --input=test.md).

For optional directories, this means we require specific methods like EleventyFiles.getIncludesDir to check for these things. For mandatory directories (e.g. output), the path can be validated as suggested in https://github.com/11ty/eleventy/issues/368#issuecomment-453155629.

aral commented 4 years ago

I’d suggest rolling this change back.

A general-purpose app like Eleventy should be able to handle any input and output directory. File system access controls should be implemented at the file system level (if the Node process is running with root privileges, that’s the problem, not that Eleventy could possibly access privileged files/directories).

Not sure if I’m missing something but this seems to solve the issue at the wrong layer in the stack and limits how Eleventy can be used as part of larger pipelines by breaking with Unix input/output conventions.

At the very least, it would be great to have a flag that disables this behaviour.

In terms of a real-world use case: it is currently blocking me from including Eleventy in Site.js. The same issue did not exist in Hugo or in any other library/framework/app I’ve encountered so far.

Thoughts?

kleinfreund commented 4 years ago

@aral Work in this MR is not blocking you from what you describe as it is not part of any Eleventy release so far. This MR has not been merged and may never get merged.

Is there an active issue you’re running into or is it rather that you anticipate this possibly becoming a problem in the future?

MadeByMike commented 3 years ago

Just wanted to pop in here because I had a legitimate situation where I wanted to use 11ty to serialise some data from collections that would be consumed by another service on my web server. It was actually a life-saver that 11ty allowed me to write a file outside the site root. All I needed was:

--- 
permalink: '../other-application/serialised-data.json'
---

I get the concerns, but also maybe 11ty is a low enough level tool that it should be able to do this? An SSG is essentially a build pipeline and it makes sense for these to be less opinionated about input and output.

If this feature has to be removed I'd at least enable writing outside the root with a setting or flag. This will also ensure ongoing support for applications that make use of this feature.

Ryuno-Ki commented 3 years ago

I could live with a flag, that highlights, that this operation may be dangerous.

zachleat commented 3 years ago

Punting this out of 1.0—sorry!

zachleat commented 3 years ago

This repository is now using lodash style issue management for enhancements. This means enhancement issues will now be closed instead of leaving them open.

View the enhancement backlog here. Don’t forget to upvote the top comment with 👍!