Project

General

Profile

Actions

Feature #2551

closed

mod_magnet lighty.stat is_symlink

Added by ef about 10 years ago. Updated over 7 years ago.

Status:
Missing Feedback
Priority:
Normal
Category:
mod_magnet
Target version:
-
ASK QUESTIONS IN Forums:

Description

I think the is_link field in the array returned by lighty.stat() is pretty useless. It is derived from a value obtained by stat(), not fstat().
I guess its value should either be replaced by the is_symlink field of the stat_cache_entry structure or an is_symlink array member added (what the attached patch does).
Of course this only works with server.follow-symlink disabled.


Files

is_symlink (414 Bytes) is_symlink ef, 2014-02-12 16:54
Actions #1

Updated by stbuehler about 10 years ago

  • Tracker changed from Bug to Feature
  • Subject changed from mod_magnet lighty.stat is_link to mod_magnet lighty.stat is_symlink
  • Target version set to 1.4.x
Actions #2

Updated by gstrauss about 8 years ago

  • Status changed from New to Need Feedback

@stbuehler, is this something we want to consider adding at all?

The value may or may not have any semblance of validity depending on other configuration, and there is always a race condition between time-of-check (lstat()) and time-of-use later in lua.

If you need to manipulate the filesystem, you're probably better off using the LuaFileSystem module.

Actions #3

Updated by ef almost 8 years ago

gstrauss wrote:

The value may or may not have any semblance of validity depending on other configuration

Why that?
The present situation is that the value is always false.

and there is always a race condition between time-of-check (lstat()) and time-of-use later in lua.

Yes, like all the other lighty.stat values (or even lighttpd's internal values).

If you need to manipulate the filesystem, you're probably better off using the LuaFileSystem module.

It's a file system attribute cache after all, isn't it?

Actions #4

Updated by gstrauss almost 8 years ago

It's a file system attribute cache after all, isn't it?

Are you aware of just how expensive it is to use server.follow-symlink = "disable"? I suggest strace -p <lighttpd_pid> and watching what happens when you make a request. If not critical to your security model, then you might set server.follow-symlink = "enable" and use the LuaFileSystem module to check for symlinks where you need to do so. If server.follow-symlink = "disable" is critical to your security model, then you ought to consider how the stat cache introduces large ToC-ToU race conditions. Disabling the stat cache would reduce the ToC-ToU race conditions with the stat and then later open, but the race conditions would still exist.

Of course this only works with server.follow-symlink disabled.

I am not inclined to champion adding a feature which is valid only for one specific configuration, when that specific configuration is a security-related configuration option (server.follow-symlink), and when that security-related configuration option already has issues with ToC-ToU.

Your patch exposes the information "this was a symlink at some time in the past" if and only if server.follow-symlink = "disable".

How are you using this patch? Please help me to understand why is this important to you, considering you have set server.follow-symlink = "disabled", presumably for a reason.

Without a really good reason from you, I hope you'll agree that this feature request should be withdrawn, as it is not likely to be applicable (or safe) for general usage.

Actions #5

Updated by gstrauss almost 8 years ago

  • Status changed from Need Feedback to Missing Feedback

Withdrawing. The request is not a good match with the security-oriented server.follow-symlink directive, which already has a few serious limitations and caveats in its current implementation.

Actions #6

Updated by ef almost 8 years ago

Are you aware of just how expensive it is to use
server.follow-symlink = "disable"?

I do all the resolving in the Lua script; It may be expensive, but I see no other way of allowing a restrained set of symlinks.

I am not inclined to champion adding a feature which is valid only for one specific configuration, when that specific configuration is a security-related configuration option (server.follow-symlink), and when that security-related configuration option already has issues with ToC-ToU.

Your patch exposes the information "this was a symlink at some time in the past" if and only if server.follow-symlink = "disable".

I think we are talking past each other.

I have a lua script which expands paths into a canonical form not supposed to contain any symlinks. It is that expanded form that lighttpd then tries to deliver.
Now of course, by the time lighttpd tries to deliver, the converted path may contain symlinks although it's not supposed to. However, that problem is independent of me using the stat cache to determine if a component is a symlink. Also, think of me using a modified file system with a mount option causing read() et al (but not readlink()) to return ELOOP on the first symlink encountered; then any left-over symlink would cause the request to fail.

Now, my script walks through the path components, using the stat cache to determine if a component is a symlink. That information may be out-dated, but as far as I can tell, it doesn't impose any (additional) security risk if it is:
-- If the cache says it's a symlink while in fact it's not, readlink(2) is guaranteed to fail with EINVAL.
-- If the cache says it's not a symlink, while in fact it is, there's no /additional/ security risk by using the cache. Also think of my mount option.
If the link target changes after my readlink(), it doesn't matter because I use whatever I get to both base my access checks on and replace the symlink with the target.

How are you using this patch? Please help me to understand why is this important to you, considering you have set server.follow-symlink = "disabled", presumably for a reason.

Basically, I want to allow some uses of symlinks while rejecting others.
I have that script that expands each symlink, either rejecting it or replacing the path component with the link target.
Allowed uses of symlinks (in the physical path) include vhost entries under server.docuument-root, links inside people's www directories.
Forbidden uses of symliks include linking from a personal www directory to another person's or outside the www tree.
The exact rules are somewhat involved, I would have to read my own script in order to state them.

Actions #7

Updated by gstrauss almost 8 years ago

Thanks for the response.

I won't attempt to convince you that I do understand what you're trying to do. It doesn't matter. I understand the security implications even better. In any case, as I mentioned 2 months ago, your request is not a good match with the security-oriented server.follow-symlink directive, which already has a few serious limitations and caveats in its current implementation.

I maintain that you should avoid the stat_cache in order to reduce the race conditions inherent in your usage.

It sounds like what you really should be doing is to realpath() ('man -s 3 realpath') on the target path, and then check that the resulting path begins with the document root, or the userdir path for the user specified in the URL. There is still a race condition between time of check and time of use, but doing this check will prevent the average user from symlinking to, and then causing the web server to deliver files outside the document root or across userdirs.

I have not used luaposix, but a quick search turned up a realpath implementation for lua: https://github.com/luaposix/luaposix
https://luaposix.github.io/luaposix/modules/posix.stdlib.html

Actions #8

Updated by stbuehler over 7 years ago

  • Target version deleted (1.4.x)
Actions

Also available in: Atom