STORM-269: prevent logviewer from serving files outside the log directory

[ptgoetz]

This is a quick fix to prevent the log viewer from serving files outside of the log directory. It simply checks that the requested file has the root log directory as a parent. If this is not the case, it will not serve the file.

[d2r]

The string check will not prevent log-dir/../../../../etc/passwd I don't think.

Can we try something like this?

(let [file (.getCanonicalFile (File. LOG-DIR fname))
      path (.getCanonicalPath file)]
    (if (= (File. LOG-DIR) (.getParentFile file))

Indentation looks a little weird too, after the if was added. Not sure if that's a tabs vs. spaces issue.

[ptgoetz]

The path to the requested file (String) comes from File.getCanonicalPath() further down in the code. so all the relative portions of the path will have been resolved.

I'll check the indentation. I'm open to either approach.

[d2r]

Yeah, you're right, it is resolved. But then I also noticed root-dir is given by the request.

Would a request like this also succeed?

/log?tail=99999999&file=passwd&log-root=/etc

[ptgoetz]

Yeah, good catch. This thing is an open http server for the entire file system. Thankfully it doesn't support POST or PUT.

There's no reason we should allow the root-dir to be specified as a request parameter. That's insane. I pity the fool running this as root.

Whatever the fix, I think it needs to be a high priority. I would also consider back porting the fix to earlier releases.

[ptgoetz]

@d2r I think we're okay with this patch. I tried a couple of things and it looks like you can't override root-dir in the request since the conf-middleware function associates it with the request, so it is ignored if supplied as a request parameter.

I tried the following url:

http://supervisor:8000/log?file=hosts&log-root=/etc/

And nothing was returned. I got the following in logviewer.log:

2014-04-27 14:07:23 o.m.log [WARN] /log?file=hosts&log-root=/etc/
java.io.FileNotFoundException: /var/log/storm/hosts (No such file or directory)

(Note that in this installation, storm is configured to log to /var/log/storm/)

[lucky]

Root or not, any file readable by storm is accessible, including configuration files which means this has the potential for exposing credentials.

[ptgoetz]

@lucky Agreed. Until this is fixed, I think we need to recommend that the logviewer daemon not be run unless appropriate safeguards are in place.

[harshach]

I don't think log-root can be sent as http param its being read from storm config .
https://github.com/apache/incubator-storm/blob/master/storm-core/src/clj/backtype/storm/daemon/logviewer.clj#L108
with @ptgoetz fix logviewer not able to read any other files apart from the ones in storm.log.dir and log-root is not accepted via http param. I tested the fix in centos 6.3 and windows 7, os x it works in plugging the security hole.

[ptgoetz]

I updated the patch to fix the formatting. I also added an extra conversion of the log-root to a canonical path to ensure we are comparing full paths.

[d2r]

@harshach, @ptgoetz OK, good to know it is not as open as I feared.

With the String compare instead of File compare, do we know whether this will give a string with a trailing slash?

(.getCanonicalPath (File. root-dir))

If it does, we should be OK. If not, then a user could get to certain sibling directories. (e.g., if root-dir was a directory named "foo", then a user could read from a sibling directory "foobar").

[ptgoetz]

@d2r You're right. I switched over to using File to do the comparison. This will also prevent users from accessing files in subdirectories log directory.

[d2r]

Should this be (.getCanonicalFile (File. root-dir))?
I am not sure whether FileAppender#getFile will give the canonical path. If it does not, and if the Appender's path has a symlink, then the new condition could fail even for legitimate requests.

[ptgoetz]

@d2r -- updated per your comments.

[d2r]

• Tested with ../ -> File not found
• Tested with /etc/passwd -> File not found
• Tested above with logback pointing to a file with symlink in the path -> File not found
• Tested with file under sibling directory (logsfoo/foobar) -> File not found
• Tested normal case (logviewer.log) -> OK

Looks good.

+1

[lucky]

@d2r Did you test with the example I gave in the bug report?

../../../../../../../../../../etc/passwd

[d2r]

Just did -> File not found.

[lucky]

Awesome, thanks!

[revans2]

looks good to me too +1.

[ptgoetz]

Awesome. Thanks for the review guys.

[xiaokang]

It's really a serious security hole. Thanks all guys to find and fix it. I'm really sorry to have introduced the problem when contributing the logviewer code.

[nothinking]

To someone like me.

If you want to change log dir, add to storm.yaml this line.

storm.log.dir=/some/real/dir/path

Remember! No Symbolic Path!!