Option for wrap-file middleware to serve symlinks #68

Merged
merged 1 commit into from Apr 10, 2012

Conversation

Projects
None yet
2 participants
Contributor

lynaghk commented Apr 7, 2012

I discussed this change with @weavejester on IRC earlier this week.
It's just like what it sounds like.

Unfortunately, I couldn't think of a nice way to write a test for this.

Collaborator

weavejester commented Apr 7, 2012

This looks like it doesn't filter out ".."s in the path. Could you add a test to make sure it does?

Contributor

lynaghk commented Apr 7, 2012

My motivating use case was a symlink to ../vendor.
If your concern is security, maybe we can rename the option to :allow-unsafe-paths? or some such so that the user knows what he or she is getting into?

Collaborator

weavejester commented Apr 7, 2012

Having a symlink to ../vendor is fine. The problem is having a path with .. in it.

Contributor

lynaghk commented Apr 7, 2012

Ah, I see what you mean. I'll dig into it and update the pull this morning.

Contributor

lynaghk commented Apr 7, 2012

Is this what you meant? I just amended the commit so that safe-path? checks directly for ... No need for extra options. Test included.

Collaborator

weavejester commented Apr 7, 2012

That's not quite what I meant.

The purpose of safe-path? is to prevent directory transversal attacks. The safest way to do this is to check the canonical path directly, therefore the existing safe-path? should be kept.

This also prevents symlinks, but under most circumstances this is desirable behaviour. If we want to allow symlinks, we need an option that lessens the security a little to allow it.

Let's do this by adding in a new function, like:

(defn- directory-transversal? [path]
  (-> (str/split path #"/|\\")
      (set)
      (contains? "..")))

And then checking both safe-path? and (not) directory-transversal? under normal circumstances, and only directory-transversal? when symlinks are allowed.

Contributor

lynaghk commented Apr 7, 2012

So it's possible to do directory traversal attacks beyond adding ".." to a path?

Your solution sounds good, and I'll update the commit---I'm just curious as to what I'm missing that makes the ".." check insufficient.

Collaborator

weavejester commented Apr 7, 2012

Yes, via symlinks. If you deny symlinks, you're guaranteed to stay within the directory structure, barring something exotic like directory mounts. With symlinks, you need to be a little more careful, making sure that there are no symlinks that lead to directories you don't want the user to have access to, and that there's no way for a malicious user to sneak in a symlink somehow. Most of the time you don't need symlinks, so it makes sense to have them off by default for that extra bit of security.

Contributor

lynaghk commented Apr 7, 2012

The no-directory-traversal tests are here: lynaghk/ring@d7f869e#L3R116

Collaborator

weavejester commented Apr 7, 2012

This looks okay. Let me run a few tests and I'll try and get it merged in today or tomorrow.

Contributor

lynaghk commented Apr 7, 2012

Sounds good, thanks!

On Sat, Apr 7, 2012 at 12:48 PM, James Reeves <
reply@reply.github.com

wrote:

This looks okay. Let me run a few tests and I'll try and get it merged in
today or tomorrow.


Reply to this email directly or view it on GitHub:
#68 (comment)

Collaborator

weavejester commented Apr 10, 2012

There are a couple of small problems. I might fix them myself if you don't have time:

  1. You've replaced if-let with when-let for some reason. when is typically only used when there are side-effects. Please prefer if and if-let when you don't need side-effects.

  2. The logic for the safe path checking is a little wrong. You have:

    (or (safe-path? root path) 
        (and (:allow-symlinks? opts) (not (directory-transversal? path)))

    But it should be:

    (and (not (directory-transversal? path))
         (or (not (:allow-symlinks? opts))
             (safe-path? path)))
Contributor

lynaghk commented Apr 10, 2012

  1. I use when when there isn't an else-clause, but I'll keep that note in mind on future patches.

  2. Since safe-path? is staying the same (resolving the canonical paths), it will disallow any directory traversals outside of the public dir already. If you bump the (not (directory-traversal? path)) to the toplevel condition, won't that potentially break paths like "/public/folder/../file"?

Collaborator

weavejester commented Apr 10, 2012

It's true that in theory there's no difference, but when it comes to security I prefer a defence-in-depth approach. If there's a problem with safe-path? then an exploit might be caught by directory-transversal?.

It's true that it'll mean "/public/folder/../file" won't work, but that will only affect people explicitly adding in a ".." in their path, and why would they do that other than to exploit the system?

Collaborator

weavejester commented Apr 10, 2012

Also, in general, when shouldn't be used to replace if, because (when x y) expands to (if x (do y)). Effectively all you're doing is adding in a redundant do that's only needed for handling side effects.

Contributor

lynaghk commented Apr 10, 2012

Websites have ".." in resource paths all the time.
I wrote the logic as I did because I didn't want to introduce any breaking changes.
Can we break it into two different commits?
One introducing symlinks, and another adding the "defence-in-depth" with the directory-traversal? check applied to all paths.

Collaborator

weavejester commented Apr 10, 2012

URIs in HTML are normalized before being sent to the web server, so this change wouldn't affect that...

But on the other hand, this pull request has been going on for too long. If you make the changes to "when" I'll merge.

Defence in depth strategies can be added later, as you point out. I think I might normalize the URI being passed to the middleware, but I might save that for 1.2.0.

Contributor

lynaghk commented Apr 10, 2012

Replaced the whens.
Thanks for your help and discussion on this weavejester, I appreciate you taking the time.

weavejester merged commit 6eacec1 into mmcgrana:master Apr 10, 2012

Collaborator

weavejester commented Apr 10, 2012

Thanks for putting up with my requests and alterations :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment