Fix Dockerfile to create Images with no git safe directory restrictions (IDFGH-11511) #12636
Conversation
|
|
👋 Welcome timoxd7, thank you for your first contribution to 📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project. Pull request review and merge process you can expectEspressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.
🔁 You can re-run automatic PR checks by retrying the DangerJS action |
|
@timoxd7 could you please give an example of a docker run command and project structure which results in this issue? |
|
@igrr I updated the comment on the pull request as i had it wrongly in my mind, sorry for the confusion. The problem happens if the user inside the docker container does not match the one in the filesystem (.git parent folder). Might happen during WSL usage or in CI. |
|
@igrr Example Output: |
|
Related to #12389 |
|
@dobairoland Thank you, haven't seen that it has been fixed a few lines below mine addition, but it only fixes the issue for the ESP-IDF repo itself, not for a project with a different owner. I will add a commit, removing the line down below, as it is obsolete if this pr would be merged. However, i don't think configuring every folder inside the docker container as safe repository implies any security risks, as it is not on a local machine with other things going on, but only an isolated container. Also, everything going into the container is explicitly mounted, e.g. every repository brought into the container is already kind of "owned", making the safe directory redundant. What do you think about this? |
|
Hello @timoxd7 , first thank you for this contribution.
IIUC the I think that with this change we would make it possible to exploit this vulnerability again. For example user starting the container can bind mount some shared drive with plenty of git projects owned by different users. IMHO with Thank you |
|
I would probably avoid setting |
|
Good idea @fhrbata. Perhaps a variable which defaults to IDF_PATH which could be overwritten to |
|
@fhrbata You are right, i mainly had a CI/CD in mind, but users could still use it in private systems and other setups this could potentially be a problem as you stated out. @dobairoland I had this kind of variable also in mind, which will not solve the problem if the official image is used. Maybe a documentation entry for the docker image would be great, informing the user to add the project's repository to the docker internal git safe directories. If you both agree, i could make a new pull request with this variable while closing this one. |
|
Hello @timoxd7 , thank you for the additional input!
This is just me thinking out loud, but maybe we can move setting the
and |
|
Hello @timoxd7 , @dobairoland do you see any problem with the following approach? I quickly tested it and it seems to be working. Thank you diff --git a/tools/docker/entrypoint.sh b/tools/docker/entrypoint.sh
index 7cf15f1917e1..513b3ba00d28 100755
--- a/tools/docker/entrypoint.sh
+++ b/tools/docker/entrypoint.sh
@@ -1,6 +1,20 @@
#!/usr/bin/env bash
set -e
+# IDF_GIT_SAFE_DIR has the same format as system PATH environment variable.
+# All path specified in IDF_GIT_SAFE_DIR will be added to user's
+# global git config as safe.directory paths. For more information
+# see git-config manual page.
+if [ -n "${IDF_GIT_SAFE_DIR+x}" ]
+then
+ echo "Adding following directories into git's safe.directory"
+ echo "$IDF_GIT_SAFE_DIR" | tr ':' '\n' | while read -r dir
+ do
+ git config --global --add safe.directory "$dir"
+ echo " $dir"
+ done
+fi
+
. $IDF_PATH/export.sh
exec "$@" usage |
|
@fhrbata LGTM |
|
@fhrbata Looks good, if it is tested and works then i think it is the best solution. If it would be documented to be needed in the official documentation would make it perfect and easier to solve the versioning problem for someone first experiencing the problem. |
|
Hello @timoxd7 , thank you very much for your feedback.
I've already created a MR for this and once it's approved and merged, it will be synced into the github. It also contains a note about this possibility in the
Hopefully it will be useful. Thank you |
With 8959555cee7e[1] ("setup_git_directory(): add an owner check for the top..")
git added an ownership check of the git directory and refuses to
run any git commands, even parsing the config file, if the git directory
is not owned by the current user. The "fatal: detected dubious ownership in repository"
is reported.
This fixes CVE-2022-24765[2], which allows to compromise user account. On a
multi-user system or e.g. on a shared file system, one user may create a "rogue"
git repository with e.g. core.fsmonitor set to an arbitrary command. Other user
may unwillingly execute this command by running e.g. git-diff or
git-status within the "rogue" git repository, which may be in one of the parent
directories. If e.g. PS1 is set to display information about a git
repository in CWD, as suggested in Git in Bash[3], the user do not need to run
any git command to trigger this, just entering some subdirectory under
this "rogue" git repository is enough, because the git command will be
started transparently through the script used in PS1. The core.fsmonitor
can be set to arbitrary command. It's purpose is to help git to identify changed files
and speed up the scanning for changed files.
rogue
├── .git # owned by user1
└── dir1 # owned by user2
├── dir2 # owned by user2
└── .git # owned by user2
user1 sets core.fsmonitor for git repository in rogue directory
$ git config --add core.fsmonitor "bash -c 'rm -rf \$HOME'"
user2 enters dir1 and runs e.g. git diff and triggers the core.fsmonitor command.
The ownership check may cause problems when running git commands in
ESP-IDF Docker container. For example user may run the container as
root, but the mounted project may be owned by a particular user.
In this case git will refuse to execute any git command within the
"/project" directory, because it's not owned by root. To overcome this,
git allows to set safe.directories, for which the ownership check is
skipped. The security check may be completely disabled by setting
safe.directories to "*". This solution was proposed in PR 12636[4], but
it would allow make it possible to exploit this vulnerability again.
This fix allows user to specify git's safe.directory in IDF_GIT_SAFE_DIR
environmental variable, which may be set during container startup.
The IDF_GIT_SAFE_DIR has same format as PATH and multiple directories can be
specified by using a ":" separator. To entirely disable this git security check
within the container, user may set IDF_GIT_SAFE_DIR='*'. This might be
heplfull in CI.
Closes #12636
[1] - git/git@8959555
[2] - https://nvd.nist.gov/vuln/detail/cve-2022-24765
[3] - https://git-scm.com/book/en/v2/Appendix-A%3A-Git-in-Other-Environments-Git-in-Bash
[4] - #12636
Signed-off-by: Frantisek Hrbata <frantisek.hrbata@espressif.com>
With 8959555cee7e[1] ("setup_git_directory(): add an owner check for the top..")
git added an ownership check of the git directory and refuses to
run any git commands, even parsing the config file, if the git directory
is not owned by the current user. The "fatal: detected dubious ownership in repository"
is reported.
This fixes CVE-2022-24765[2], which allows to compromise user account. On a
multi-user system or e.g. on a shared file system, one user may create a "rogue"
git repository with e.g. core.fsmonitor set to an arbitrary command. Other user
may unwillingly execute this command by running e.g. git-diff or
git-status within the "rogue" git repository, which may be in one of the parent
directories. If e.g. PS1 is set to display information about a git
repository in CWD, as suggested in Git in Bash[3], the user do not need to run
any git command to trigger this, just entering some subdirectory under
this "rogue" git repository is enough, because the git command will be
started transparently through the script used in PS1. The core.fsmonitor
can be set to arbitrary command. It's purpose is to help git to identify changed files
and speed up the scanning for changed files.
rogue
├── .git # owned by user1
└── dir1 # owned by user2
├── dir2 # owned by user2
└── .git # owned by user2
user1 sets core.fsmonitor for git repository in rogue directory
$ git config --add core.fsmonitor "bash -c 'rm -rf \$HOME'"
user2 enters dir1 and runs e.g. git diff and triggers the core.fsmonitor command.
The ownership check may cause problems when running git commands in
ESP-IDF Docker container. For example user may run the container as
root, but the mounted project may be owned by a particular user.
In this case git will refuse to execute any git command within the
"/project" directory, because it's not owned by root. To overcome this,
git allows to set safe.directories, for which the ownership check is
skipped. The security check may be completely disabled by setting
safe.directories to "*". This solution was proposed in PR 12636[4], but
it would allow make it possible to exploit this vulnerability again.
This fix allows user to specify git's safe.directory in IDF_GIT_SAFE_DIR
environmental variable, which may be set during container startup.
The IDF_GIT_SAFE_DIR has same format as PATH and multiple directories can be
specified by using a ":" separator. To entirely disable this git security check
within the container, user may set IDF_GIT_SAFE_DIR='*'. This might be
heplfull in CI.
Closes #12636
[1] - git/git@8959555
[2] - https://nvd.nist.gov/vuln/detail/cve-2022-24765
[3] - https://git-scm.com/book/en/v2/Appendix-A%3A-Git-in-Other-Environments-Git-in-Bash
[4] - #12636
Signed-off-by: Frantisek Hrbata <frantisek.hrbata@espressif.com>
With 8959555cee7e[1] ("setup_git_directory(): add an owner check for the top..")
git added an ownership check of the git directory and refuses to
run any git commands, even parsing the config file, if the git directory
is not owned by the current user. The "fatal: detected dubious ownership in repository"
is reported.
This fixes CVE-2022-24765[2], which allows to compromise user account. On a
multi-user system or e.g. on a shared file system, one user may create a "rogue"
git repository with e.g. core.fsmonitor set to an arbitrary command. Other user
may unwillingly execute this command by running e.g. git-diff or
git-status within the "rogue" git repository, which may be in one of the parent
directories. If e.g. PS1 is set to display information about a git
repository in CWD, as suggested in Git in Bash[3], the user do not need to run
any git command to trigger this, just entering some subdirectory under
this "rogue" git repository is enough, because the git command will be
started transparently through the script used in PS1. The core.fsmonitor
can be set to arbitrary command. It's purpose is to help git to identify changed files
and speed up the scanning for changed files.
rogue
├── .git # owned by user1
└── dir1 # owned by user2
├── dir2 # owned by user2
└── .git # owned by user2
user1 sets core.fsmonitor for git repository in rogue directory
$ git config --add core.fsmonitor "bash -c 'rm -rf \$HOME'"
user2 enters dir1 and runs e.g. git diff and triggers the core.fsmonitor command.
The ownership check may cause problems when running git commands in
ESP-IDF Docker container. For example user may run the container as
root, but the mounted project may be owned by a particular user.
In this case git will refuse to execute any git command within the
"/project" directory, because it's not owned by root. To overcome this,
git allows to set safe.directories, for which the ownership check is
skipped. The security check may be completely disabled by setting
safe.directories to "*". This solution was proposed in PR 12636[4], but
it would allow make it possible to exploit this vulnerability again.
This fix allows user to specify git's safe.directory in IDF_GIT_SAFE_DIR
environmental variable, which may be set during container startup.
The IDF_GIT_SAFE_DIR has same format as PATH and multiple directories can be
specified by using a ":" separator. To entirely disable this git security check
within the container, user may set IDF_GIT_SAFE_DIR='*'. This might be
heplfull in CI.
Closes #12636
[1] - git/git@8959555
[2] - https://nvd.nist.gov/vuln/detail/cve-2022-24765
[3] - https://git-scm.com/book/en/v2/Appendix-A%3A-Git-in-Other-Environments-Git-in-Bash
[4] - #12636
Signed-off-by: Frantisek Hrbata <frantisek.hrbata@espressif.com>
Problem
If a folder is mounted to the docker container that is not owned by the running user, git disallows the direct access without explicit set of a safe directory. This can happen for example if using wsl on Windows while using a repository on the Windows Filesystem.
Fix
Just set every folder as safe for git, as this implies no security risk in a docker container, solving the issue to not be able to access certain git repositories.
Reference
The github action for ESP-IDF Builds solved this by adding the command before running the actual action command (See espressif/esp-idf-ci-action#27)
Backport
We use ESP-IDF v5.1.x, so a backport would be great.