Skip to content
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.

Sign up for GitHub

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

Timeout,proxy and rce fix #43

Merged
merged 6 commits into from
Mar 5, 2024
Merged

Timeout,proxy and rce fix #43

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
470e229
fix timeout broken for main request
srozb Feb 29, 2024
8293c00
fix potential RCE in .git/config
srozb Feb 29, 2024
7807a8c
add 'ALL_PROXY' env variable to ensure git proxied
srozb Feb 29, 2024
cc420b3
proxy check fix & avoid modifying the environment
srozb Mar 1, 2024
5b6ea35
change variable name in the second subprocess.call
srozb Mar 1, 2024
e9d5dfe
environment creation move, add missing sanitization
srozb Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions git_dumper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,21 @@ def do_task(self, obj, url, directory, retry, timeout, http_headers, client_cert
return get_referenced_sha1(obj_file)


def sanitize_file(filepath):
""" Inplace comment out possibly unsafe lines based on regex """
assert os.path.isfile(filepath), "%s is not a file" % filepath

UNSAFE=r"^\s*fsmonitor|sshcommand|askpass|editor|pager"

with open(filepath, 'r+') as f:
content = f.read()
modified_content = re.sub(UNSAFE, '# \g<0>', content, flags=re.IGNORECASE)
Copy link
Contributor

@ZanyMonk ZanyMonk Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @srozb, thanks for taking the time to fix this. However there is a few issues with your current patch.

  1. This line lacks a | re.MULTILINE. In the current state it only works if the payload is the first line (which never happens).

  2. One can bypass the regex with this syntax:

[core]
       ...
[core] fsmonitor = "sh -c '...'"

  1. As stated by the owner in the issue I opened, it would be much simpler and safer to (re)move .git/config, since we don't need it to run git checkout . IIRC.

    That is because it's possible to use many other git features to leverage RCE, for example one could [include] another file from the $GIT_DIR. This would defeat your patch.

if content != modified_content:
printf("Warning: '%s' file was altered\n" % filepath)
f.seek(0)
f.write(modified_content)


def fetch_git(url, directory, jobs, retry, timeout, http_headers, client_cert_p12=None, client_cert_p12_password=None):
""" Dump a git repository into the output directory """

Expand Down Expand Up @@ -427,7 +442,11 @@ def fetch_git(url, directory, jobs, retry, timeout, http_headers, client_cert_p1

# check for /.git/HEAD
printf("[-] Testing %s/.git/HEAD ", url)
response = session.get("%s/.git/HEAD" % url, allow_redirects=False)
response = session.get(
"%s/.git/HEAD" % url,
timeout=timeout,
allow_redirects=False
)
printf("[%d]\n", response.status_code)

valid, error_message = verify_response(response)
Expand Down Expand Up @@ -459,10 +478,18 @@ def fetch_git(url, directory, jobs, retry, timeout, http_headers, client_cert_p1
jobs,
args=(url, directory, retry, timeout, http_headers),
)

os.chdir(directory)

printf("[-] Sanitizing .git/config\n")
sanitize_file(".git/config")

printf("[-] Running git checkout .\n")
os.chdir(directory)
subprocess.check_call(["git", "checkout", "."])
ENV = os.environ
Copy link
Owner

@arthaud arthaud Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't modify the current environment in case someone uses git-dumper as a library (although unlikely). We can simply make a copy.
Also, nitpicking but I would just call it "environment". ENV makes it seem like it's a global variable, which it isn't.

Suggested change
ENV = os.environ
environment = os.environ.copy()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if so, git checkout should not be called at all as someone might assume that the whole traffic is transported over configured proxy and I think subprocess will not use monkeypatched sockets. Perhaps adding optional flag --git-checkout with a proper explanation would suffice.

Copy link
Owner

@arthaud arthaud Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think there is a misunderstanding.
We should definitely call git checkout . with the modified environment. I'm just saying we shouldn't modify os.environ in case it's used somewhere else. TL, DR: Just add a .copy() to os.environ.

configured_proxy = socks.getdefaultproxy()
if configured_proxy[0] > 0:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there are no proxy configured? It looks like configured_proxy would be None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it's indeed a None type, so changing 490 line to:

if configured_proxy != None:

should not raise exception in case of no proxy is set. I thought I tested a no proxy usage but looks like I didn't.

ENV["ALL_PROXY"] = f"http.proxy={["http", "socks4h", "socks5h"][configured_proxy[0]]}" + f"://{configured_proxy[1]}:{configured_proxy[2]}"
subprocess.check_call(["git", "checkout", "."], env=ENV)
return 0

# no directory listing
Expand Down Expand Up @@ -644,7 +671,11 @@ def fetch_git(url, directory, jobs, retry, timeout, http_headers, client_cert_p1
os.chdir(directory)

# ignore errors
subprocess.call(["git", "checkout", "."], stderr=open(os.devnull, "wb"))
subprocess.call(
["git", "checkout", "."],
stderr=open(os.devnull, "wb"),
env=ENV
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ENV is only created within that if line 469 so it wouldn't exist here (since that if returns).
We also need to sanitize the .git/config here.
We could move the whole git checkout step in a function to avoid code duplication.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment wasn't addressed.
Notice how the if line 450 implements a different stategy where we just download recursively using directory indexing, but if that if is not taken, we use a different strategy. In that case, you are not sanitizing the .git/config and you are not setting up the environment variable either.
We need two changes:

  • Move the environment variable creation and setup before the if line 450
  • Add a Sanitizing .git/config step here (line 647) before calling git checkout .


return 0

Expand Down