Timeout,proxy and rce fix #43

srozb · 2024-02-29T11:18:15Z

3 commits stacked here:

Timeout was not applied to the first request and therefore causing script to stuck if target was not responding.
As mentioned in Security risk - RCE in .git/config #42, potential dangerous variables in .git/config file may cause RCE. This commit tries to comment out lines that may be unsafe - feel free to add more patterns of such lines. I'm aware this is not a perfect solution but at least some kind of protection.
I'm not sure if proxy configuration was applied to git command issued as a subprocess. This commit adds environment variable ALL_PROXY to ensure git communication uses configured proxy.

arthaud

Thanks for doing this. I wrote down some comments.

arthaud · 2024-03-01T09:30:05Z

git_dumper.py


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


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()

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.

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.

arthaud · 2024-03-01T09:35:50Z

git_dumper.py

-        subprocess.check_call(["git", "checkout", "."])
+        ENV = os.environ
+        configured_proxy = socks.getdefaultproxy()
+        if configured_proxy[0] > 0:


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

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.

arthaud · 2024-03-01T09:37:16Z

git_dumper.py

+        ["git", "checkout", "."], 
+        stderr=open(os.devnull, "wb"),
+        env=ENV
+    )


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.

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 .

arthaud

(wrongly clicked on Approve above)

arthaud · 2024-03-02T15:55:07Z

git_dumper.py

+        ["git", "checkout", "."], 
+        stderr=open(os.devnull, "wb"),
+        env=ENV
+    )


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 .

srozb · 2024-03-04T09:44:54Z

Please take another look at changed source, as I'm not 100% sure if I picked the correct line to move the environment creation to.

I think eventually it'd be better to move the ALL_PROXY env variable creation just after the pysocks proxy is set up and ensure every subprocess call is taken with env=environment argument. It'd also be nice to bound sanitization with checkout putting it together in separate function.

(thinking out loud)

arthaud · 2024-03-05T08:28:29Z

Great, thanks for contributing!

ZanyMonk · 2024-04-16T00:13:49Z

git_dumper.py

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


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

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

One can bypass the regex with this syntax:

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

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.

srozb added 3 commits February 29, 2024 10:36

fix timeout broken for main request

470e229

fix potential RCE in .git/config

8293c00

add 'ALL_PROXY' env variable to ensure git proxied

7807a8c

srozb changed the title ~~Socks fix~~ Timeout,proxy and rce fix Feb 29, 2024

arthaud approved these changes Mar 1, 2024

View reviewed changes

arthaud requested changes Mar 1, 2024

View reviewed changes

srozb added 2 commits March 1, 2024 12:51

proxy check fix & avoid modifying the environment

cc420b3

change variable name in the second subprocess.call

5b6ea35

arthaud requested changes Mar 2, 2024

View reviewed changes

environment creation move, add missing sanitization

e9d5dfe

arthaud merged commit 1df41e9 into arthaud:master Mar 5, 2024

ZanyMonk reviewed Apr 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeout,proxy and rce fix #43

Timeout,proxy and rce fix #43

srozb commented Feb 29, 2024

arthaud left a comment

arthaud Mar 1, 2024 •

edited

Loading

srozb Mar 1, 2024

arthaud Mar 1, 2024 •

edited

Loading

arthaud Mar 1, 2024

srozb Mar 1, 2024

arthaud Mar 1, 2024

arthaud Mar 2, 2024

arthaud left a comment

arthaud Mar 2, 2024

srozb commented Mar 4, 2024 •

edited

Loading

arthaud commented Mar 5, 2024

ZanyMonk Apr 16, 2024 •

edited

Loading

Timeout,proxy and rce fix #43

Timeout,proxy and rce fix #43

Conversation

srozb commented Feb 29, 2024

arthaud left a comment

Choose a reason for hiding this comment

arthaud Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

srozb Mar 1, 2024

Choose a reason for hiding this comment

arthaud Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

arthaud Mar 1, 2024

Choose a reason for hiding this comment

srozb Mar 1, 2024

Choose a reason for hiding this comment

arthaud Mar 1, 2024

Choose a reason for hiding this comment

arthaud Mar 2, 2024

Choose a reason for hiding this comment

arthaud left a comment

Choose a reason for hiding this comment

arthaud Mar 2, 2024

Choose a reason for hiding this comment

srozb commented Mar 4, 2024 • edited Loading

arthaud commented Mar 5, 2024

ZanyMonk Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

arthaud Mar 1, 2024 •

edited

Loading

arthaud Mar 1, 2024 •

edited

Loading

srozb commented Mar 4, 2024 •

edited

Loading

ZanyMonk Apr 16, 2024 •

edited

Loading