New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UIFR-215: Do not allow loading arbitrary files #59
Conversation
| else { | ||
| resourceFile = new File(OpenmrsUtil.getApplicationDataDirectory(), path); | ||
| } | ||
| resourceFile = new File(OpenmrsUtil.getApplicationDataDirectory(), path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibacher and @mogoodrich perhaps still a security risk to allow access to application data directory, as this is typically where the openmrs-runtime.properties are located. Could this result in exposing sensitive data there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes should be fine for the FileResourceProvider, which was very recently added and I believe the only use case for it is to load resources from the .OpenMRS/configuration directory. Thanks for fixing this security hole that we opened up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still bypassable. if the "path" is user controlled they could enter something like ../../../../../../../../../../../../etc/passwd or similar which even when appended to the application directory will result in a bypass. You will want to do a getcanonicalpath on the path before appending the app directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@golfhackerdave The changes to this file aren't intended to resolve that issue, merely to remove the additional support for absolute paths. The actual fix is in the code for the ResourceFactory. This is because this class isn't used directly to load code, but indirectly via a call to the ResourceFactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mseaton @mogoodrich Could we just rename this to InitializerResourceProvider and restrict it to the configuration sub-directory Initializer looks for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, though I think I'd lean toward calling it "ConfigurationResourceProvider"... thoughts @mseaton ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to make the same comment. I'm fine with it, but don't tie it to Initilalizer. +1 to @mogoodrich suggestion of ConfigurationResourceProvider
| .isAbsolute()) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this fix here and am supportive of it, but can you confirm that it does not prevent behaviors that are currently relied upon from working? The main thing I am thinking of is "development mode" which allows for hot reloading of pages within UI framework when "watching" certain modules. Does this affect that? @mogoodrich do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm reasonably sure this doesn't break live reloading in development mode. Development mode seems to be support not by providing absolute paths at the point where the resource is requested so much as by scanning a fixed set of development directories in the ModuleResourceProvider class. As long as the paths provided are relative paths (which they should be) that don't involve any directory traversal, things should be fine. So things like: htmlforms:scripts/mygreatscript.js will still load.
I'm hoping that the need to request a resource by an absolute path is... non-existent, but we'll have to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think we can go with the assumption that the need to request by absolute path is non-existent and not allowed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibacher are you able to quickly test live reloading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mogoodrich Testing this really quickly and it works fine (edited the htmlForm.js file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
This prevents the load of files via absolute paths or path traversal. Ticket is here.