Repository navigation
fix: stop following a link out of the root that a file service serves #127
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
base: master
Are you sure you want to change the base?
Changes from all commits
a885317
79266ee
8162674
afd0f8c
126d05c
b3692cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,6 +200,7 @@ | |
| random_integer, | ||
| host, | ||
| hostname, | ||
| is_sub_path, | ||
| size_round_unit, | ||
| verify, | ||
| verify_equal, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,7 @@ def __init__( | |
| list_engine="base", | ||
| cors=False, | ||
| cache=0, | ||
| follow_links=False, | ||
| *args, | ||
| **kwargs | ||
| ): | ||
|
|
@@ -100,6 +101,7 @@ def __init__( | |
| self.list_engine = list_engine | ||
| self.cors = cors | ||
| self.cache = cache | ||
| self.follow_links = follow_links | ||
|
|
||
| @classmethod | ||
| def _sorter_build(cls, name=None): | ||
|
|
@@ -497,6 +499,10 @@ def on_serve(self): | |
| self.cors = self.get_env("CORS", self.cors, cast=bool) | ||
| if self.env: | ||
| self.cache = self.get_env("CACHE", self.cache, cast=int) | ||
| if self.env: | ||
| self.follow_links = self.get_env( | ||
| "FOLLOW_LINKS", self.follow_links, cast=bool | ||
| ) | ||
| self._build_regex() | ||
| self.base_path = os.path.abspath(self.base_path) | ||
| self.cache_d = datetime.timedelta(seconds=self.cache) | ||
|
|
@@ -547,7 +553,7 @@ def on_data_http(self, connection, parser): | |
| # verifies if the provided path starts with the contents of the | ||
| # base path in case it does not it's a security issue and a proper | ||
| # exception must be raised indicating the issue | ||
| is_sub = path_f.startswith(self.base_path) | ||
| is_sub = self._is_sub(path_f) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target implementation ---'
sed -n '500,610p' src/netius/extra/file.py
printf '%s\n' '--- related path/open calls ---'
rg -n -C 4 '_is_sub|on_normal_file|open\\(|getmtime|getsize|index' src/netius/extra/file.pyRepository: hivesolutions/netius Length of output: 5429 🏁 Script executed: #!/bin/bash
set -eu
sed -n '500,610p' src/netius/extra/file.py
rg -n -C 4 '_is_sub|on_normal_file|open\(|getmtime|getsize|index' src/netius/extra/file.pyRepository: hivesolutions/netius Length of output: 10653 🏁 Script executed: #!/bin/bash
set -eu
sed -n '630,890p' src/netius/extra/file.pyRepository: hivesolutions/netius Length of output: 10152 Path Traversal (CWE-59) Reachability: External · Exploitability: Difficult Make containment and file opening atomic.
🤖 Prompt for AI AgentsSource: Learnings |
||
| if not is_sub: | ||
| raise netius.SecurityError("Invalid path") | ||
|
Comment on lines
+556
to
558
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
|
|
||
|
|
@@ -595,6 +601,13 @@ def on_dir_file(self, connection, parser, path, style=True): | |
| index_path = os.path.join(path, index_file) | ||
| if not os.path.exists(index_path): | ||
| continue | ||
|
|
||
| # verifies that the index file is itself under the base path, as | ||
| # it's the one that is going to be opened and it may be a link | ||
| # whose target lies outside of it (security issue) | ||
| is_sub = self._is_sub(index_path) | ||
| if not is_sub: | ||
| raise netius.SecurityError("Invalid path") | ||
| return self.on_normal_file(connection, parser, index_path) | ||
|
|
||
| if not self.list_dirs: | ||
|
|
@@ -837,6 +850,11 @@ def _resolve_regex(self, path): | |
| return (value, True) | ||
| return (path, False) | ||
|
|
||
| def _is_sub(self, path): | ||
| return netius.common.is_sub_path( | ||
| self.base_path, path, follow_links=self.follow_links | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| import logging | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,12 +96,19 @@ | |
| class FTPConnection(netius.Connection): | ||
|
|
||
| def __init__( | ||
| self, base_path="", host="ftp.localhost", mode="ascii", *args, **kwargs | ||
| self, | ||
| base_path="", | ||
| host="ftp.localhost", | ||
| mode="ascii", | ||
| follow_links=False, | ||
|
joamag marked this conversation as resolved.
|
||
| *args, | ||
| **kwargs | ||
| ): | ||
| netius.Connection.__init__(self, *args, **kwargs) | ||
| self.parser = None | ||
| self.base_path = os.path.abspath(base_path) | ||
| self.host = host | ||
| self.follow_links = follow_links | ||
| self.mode = mode | ||
| self.data_server = None | ||
| self.remaining = None | ||
|
|
@@ -214,16 +221,13 @@ def flush_list(self): | |
|
|
||
| def flush_retr(self): | ||
| self.send_ftp(150, "file sending") | ||
| full_path = self._get_path(self.file_name) | ||
| self.bytes_p = os.path.getsize(full_path) | ||
| self.file = open(full_path, "rb") | ||
| self.bytes_p = os.path.getsize(self.file_path) | ||
| self.file = open(self.file_path, "rb") | ||
| self._file_send(self) | ||
|
|
||
| def flush_stor(self): | ||
| self.send_ftp(150, "file receiving") | ||
|
|
||
| full_path = self._get_path(self.file_name) | ||
| self.file = open(full_path, "wb") | ||
| self.file = open(self.file_path, "wb") | ||
|
|
||
| def on_flush_list(self, connection): | ||
| self._data_close() | ||
|
|
@@ -258,9 +262,14 @@ def on_line(self, code, message, is_final=True): | |
|
|
||
| # retrieves the reference to the method that is going to be called | ||
| # for the handling of the current line from the current instance and | ||
| # then calls it with the provided message | ||
|
joamag marked this conversation as resolved.
|
||
| # then calls it with the provided message, note that a path that is | ||
| # refused is reported back as a failed command instead of being | ||
| # raised, as dropping the connection for it would be too much | ||
| method = getattr(self, method_n) | ||
| method(message) | ||
| try: | ||
| method(message) | ||
| except netius.SecurityError: | ||
|
joamag marked this conversation as resolved.
|
||
| self.not_ok() | ||
|
Comment on lines
+269
to
+272
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a session has already accepted Useful? React with 👍 / 👎. |
||
|
|
||
| def on_user(self, message): | ||
| self.username = message | ||
|
|
@@ -387,13 +396,18 @@ def on_list(self, message): | |
| self.data_server.flush_ftp() | ||
|
|
||
| def on_retr(self, message): | ||
| # resolves the path of the file at the command itself, so that a | ||
| # path that is refused is answered as a failed command before the | ||
| # transfer is announced, note that the flushing of the transfer may | ||
| # only run once the data connection is accepted, at which point the | ||
| # refusal would have no command left to be reported against | ||
| self.file_path = self._get_path(extra=message) | ||
| self.remaining = "retr" | ||
| self.file_name = message | ||
| self.data_server.flush_ftp() | ||
|
|
||
| def on_stor(self, message): | ||
| self.file_path = self._get_path(extra=message) | ||
| self.remaining = "stor" | ||
| self.file_name = message | ||
| self.data_server.flush_ftp() | ||
|
|
||
| def _file_send(self, connection): | ||
|
|
@@ -497,8 +511,8 @@ def _get_path(self, extra=None): | |
| # verifies if the resolved path starts with the contents of the | ||
| # base path in case it does not it's a security issue and a proper | ||
| # exception must be raised indicating the issue | ||
| is_sub = relative_path == self.base_path or relative_path.startswith( | ||
| os.path.join(self.base_path, "") | ||
| is_sub = netius.common.is_sub_path( | ||
| self.base_path, relative_path, follow_links=self.follow_links | ||
| ) | ||
| if not is_sub: | ||
| raise netius.SecurityError("Invalid path") | ||
|
joamag marked this conversation as resolved.
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
@@ -559,10 +573,13 @@ class FTPServer(netius.ContainerServer): | |
| :see: http://tools.ietf.org/html/rfc959 | ||
| """ | ||
|
|
||
| def __init__(self, base_path="", auth_s="dummy", *args, **kwargs): | ||
| def __init__( | ||
| self, base_path="", auth_s="dummy", follow_links=False, *args, **kwargs | ||
| ): | ||
| netius.ContainerServer.__init__(self, *args, **kwargs) | ||
| self.base_path = base_path | ||
| self.auth_s = auth_s | ||
| self.follow_links = follow_links | ||
|
|
||
| def serve(self, host="ftp.localhost", port=21, *args, **kwargs): | ||
| netius.ContainerServer.serve(self, port=port, *args, **kwargs) | ||
|
|
@@ -584,6 +601,10 @@ def on_serve(self): | |
| self.host = self.get_env("FTP_HOST", self.host) | ||
| if self.env: | ||
| self.auth_s = self.get_env("FTP_AUTH", self.auth_s) | ||
| if self.env: | ||
| self.follow_links = self.get_env( | ||
| "FOLLOW_LINKS", self.follow_links, cast=bool | ||
| ) | ||
| self.auth = self.get_auth(self.auth_s) | ||
| self.info( | ||
| "Starting FTP server on '%s' using '%s' ..." % (self.host, self.auth_s) | ||
|
|
@@ -600,6 +621,7 @@ def build_connection(self, socket, address, ssl=False): | |
| ssl=ssl, | ||
| base_path=self.base_path, | ||
| host=self.host, | ||
| follow_links=self.follow_links, | ||
| ) | ||
|
|
||
| def on_line_ftp(self, connection, code, message): | ||
|
|
||
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: hivesolutions/netius
Length of output: 11362
🤖 get_repo_knowledge executed:
get_repo_knowledge hivesolutions/netius /tmp/coderabbit-repo-knowledge/hivesolutions-netius-a8ceff5e/conventionsLength of output: 1567
🏁 Script executed:
Repository: hivesolutions/netius
Length of output: 50377
🏁 Script executed:
Repository: hivesolutions/netius
Length of output: 7642
🏁 Script executed:
Repository: hivesolutions/netius
Length of output: 8923
🏁 Script executed:
Repository: hivesolutions/netius
Length of output: 158
Preserve the positional constructor contract.
FileServer.__init__captures the first positional value aftercacheasfollow_links, instead of forwarding it toHTTP2Server.__init__aslegacy. This can change existing positional configurations. Handlefollow_linksas a keyword without shifting*argsinsrc/netius/extra/file.pyandsrc/netius/extra/file.pyi.🧰 Tools
🪛 ast-grep (0.45.2)
[warning] 80-103: Do not use an empty list as a default parameter
Context: def init(
self,
base_path="",
style_urls=[],
index_files=[],
path_regex=[],
list_dirs=True,
list_engine="base",
cors=False,
cache=0,
follow_links=False,
*args,
**kwargs
):
netius.servers.HTTP2Server.init(self, *args, **kwargs)
self.base_path = base_path
self.style_urls = style_urls
self.index_files = index_files
self.path_regex = path_regex
self.list_dirs = list_dirs
self.list_engine = list_engine
self.cors = cors
self.cache = cache
self.follow_links = follow_links
Note: [CWE-710] Improper Adherence to Coding Standards (mutable default argument).
(no-empty-list-as-parameter)
📍 Affects 2 files
src/netius/extra/file.py#L91-L91(this comment)src/netius/extra/file.pyi#L34-L34🤖 Prompt for AI Agents