fix: FileProvider.validate() leaked files outside root #4474
Conversation
Fixes two issues and adds two improvements: * FileProvider would read files outside the root if the root were a prefix of the file path string-wise. Consider a root path `/tmp/sample/root` and a relative path `../root.txt`, resulting in reading a file `/tmp/sample/root.txt`. * The "inside root" check was performed after checking if file existed. This would expose information about files outside the root through the exception. While an exploit would be difficult (if possible at all), it is better to fix this weakness anyway. * The exception type is changed from the most generic `Exception` to `ValueError` to make it easier to distinguish it from `ContentException` that is raised in other cases. * The `"/"` literal in path operations is changed to `os.sep` to improve portability. Signed-off-by: Jan Holeček <71874510+jholecek-rh@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4474 +/- ##
=======================================
Coverage 77.67% 77.67%
=======================================
Files 745 745
Lines 41737 41742 +5
Branches 6703 6704 +1
=======================================
+ Hits 32418 32423 +5
Misses 8290 8290
Partials 1029 1029
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| self.validate() | ||
|
|
||
| def _is_inside_root(self): |
There was a problem hiding this comment.
As this method is not used anywhere out the validate, better make it an inner funciton of the validate, Otherwise, we may need to add particular tests it (although it's a private one)
There was a problem hiding this comment.
I cannot agree with this. In my opinion, nested function and class definitions are a niche feature for specific cases (e.g. wrappers). Nested functions cannot be accessed, reused, or called manually when troubleshooting.
I intentionally chose a private name for _is_inside_root so that it remains an implementation detail. It is not part of the public interface and thus should not need a dedicated test. The implementation is properly covered by tests that I did add (in this PR and in #4458) that use the public validate method.
In fact, I would argue that validate should not be public either. We touched the issue of interfaces of provider classes in our private conversations and I do intend to continue those. However, clarifying the public interface of the FileProvider class was beyond the scope of this PR.
There was a problem hiding this comment.
P.S. The FileProvider.validate() method is not used only in FileProvider.__init__. Should it be defined as a nested function in the __init__ method?
There was a problem hiding this comment.
Sorry, I didn't get your point
- Although the
validate()is called only in the__ini__, but for different child classes, it has different implementation. It should not be designed as inner function. inner function, only works for function which is very simple (just like this one) and sometimes a bit common (can be called times) for its parent function.
If its function is common enough, and is useful not only for its parent caller. We can upgrade it as you did or might need to consider moving it to the insights.util as a utility function.
Because I didn't get from the code and feel hard to imagine the possibility that it will be used by other cases in this class, I asked you move it to a inner function or just expand it in the caller.
There was a problem hiding this comment.
Although the validate() is called only in the ini, but for different child classes, it has different implementation. It should not be designed as inner function.
I do agree that the validate() method should not be defined as an inner function. I used this example to demonstrate why I do not agree with the reasons that you provided for defining _is_inside_root() as an inner function.
As for the "different implemenations", no subclass of FileProvider overrides the validate() method. The CommandOutputProvider class happens to define a similar method, but the implementation is completely independent. I have brought up issues in the insight-core hierarchy of provider classes with you in a separate discussion. I think the hierarchy deserves a close review of the interfaces and separation of responsibilities. I would prefer to keep this topic separate from this PR that aims to fix a vulnerability.
As for the rest, it seems that our views of what a well-structured, readable code looks like differ significantly. The structure and readability of insights-core code is actually one of the broader topics that we wanted to discuss with you, because my team continuously struggles when dealing with the insights-core code base.
I cannot continue this PR without a better understanding of/alignment on desired code style and its justification. I will reach out to you in private to agree next steps and timelines.
xiangce
added
the
data processing
All Pull Requests:
Check all that apply:
Complete Description of Additions/Changes:
Fixes two issues and adds two improvements:
FileProvider would read files outside the root if the root were a prefix of the file path string-wise. Consider a root path
/tmp/sample/rootand a relative path../root.txt, resulting in reading a file/tmp/sample/root.txt.The "inside root" check was performed after checking if file existed. This would expose information about files outside the root through the exception. While an exploit would be difficult (if possible at all), it is better to fix this weakness anyway.
The exception type is changed from the most generic
ExceptiontoValueErrorto make it easier to distinguish it fromContentExceptionthat is raised in other cases.The
"/"literal in path operations is changed toos.septo improve portability.