Limit include access

[robhrt7]

Hi,

Recently we run into security issue with EJS include being able to access any file on the system. Which means, that any application running on common server could include files from home, root and and other personal data directories.

It's possible to do something like this <%- include('/path/to/home/.ssh/id_rsa' %>, stealing private content from the server. This is quite a big breach, potentially leading to many problems.

I was investigating the possibilities of limiting EJS include for some specific fs scope, and seems like there's no workaround for this, except special node environment set-up in isolated sandbox or forking EJS. I also tried to override include helper function, which is possible, but I couldn't managed to implement scoping for nested includes. Also, legacy support for older include syntax is not possible to override from outside.

Potential solution for the problem is based on providing an additional option with defined sandbox directory path. This option could be then used to limit fs lookups, limiting access outside defined space.

[robhrt7]

Here's the example sandbox option implementation robhrt7@3bb676f. If it's acceptable, I can make a PR.

[mde]

This could potentially be very useful for everyone, yes. Could you give some examples of usage, and include tests, so I can be sure I understand exactly how you're using path.relative to enforce this?

[robhrt7]

I'll make a PR with all proper tests as soon as we will agree that EJS needs this feature. path.relative is used here to check which filepath was requested, relatively to defined boundaries.

For example, if I set sandbox to /some/path/to/app then if app will try to include templates from /some/path the error will occur. Here's an example from my app sourcejs/Source@6a2ef4a.

[mde]

Given how deeply the filesystem access is baked into EJS, I don't see a better way of ensuring reasonable security for systems that use it. The patch is pretty minimal, and adds very little to the complexity of the API or to the download size for browser-side use. I'll definitely merge a PR that implements this.

[RyanZim]

@operatino, are you working on this?

[robhrt7]

@RyanZim I have my fork done (robhrt7@3bb676f), will do a PR as soon as I'll have time.