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.

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

Create private .ssh and .gnupg directories prior to merge (during clone) #74

Closed
TheLocehiliosan opened this issue Jul 14, 2017 · 17 comments

Comments

Projects
None yet
3 participants
@TheLocehiliosan
Copy link
Owner

commented Jul 14, 2017

As referenced in Debian #868300, permissions on .ssh and .gnupg directories are restricted after an initial clone is complete. Git employs the user's umask to set permissions for any files/directories created.

If .ssh or .gnupg directories already exist with restricted permissions prior to cloning, the permissions will not be changed. Git only affects the permissions of directories that do not already exist. In practice this race condition does not pose a problem assuming the encryption feature is used for confidential data.

For example, suppose a user wants to manage a .ssh/config and .ssh/id_rsa. Also suppose the .ssh/config is limited configuration data only, and the .ssh/id_rsa is their private key. If .ssh/id_rsa is included using the encryption function the following scenarios are possible:

  • If .ssh already exists and is secured

    • During the clone .ssh permissions are not changed (remain secure)
  • If .ssh does not yet exist

    • During the clone .ssh and .ssh/config will be created with the default umask
    • (at this point no private data is exposed)
    • After the clone permissions are restricted
    • yadm decrypt is run (either by the user or via bootstrap) and the .ssh/id_rsa is put into the .ssh. .ssh's permissions will already be restricted at this point

Regardless, I plan to update clone operations. If there are files tracked in the repo under locations .ssh or .gnupg and those paths do not yet exist (an initial clone), I will create empty directories and secure them first. This should mitigate any concerns about a race condition. However, I don't think there is any effective problem as it stands today.

@TheLocehiliosan TheLocehiliosan added the bug label Jul 14, 2017

@TheLocehiliosan TheLocehiliosan added this to the 1.11.1 milestone Jul 14, 2017

@TheLocehiliosan TheLocehiliosan self-assigned this Jul 14, 2017

@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 14, 2017

@danielshahaf - I think you reported this Debian bug report. If you want to take a look at my comments above, I'm not sure if grave is the most appropriate severity level. I would be interested if you had any feedback on my comments, and planned changes. Thanks!

@danielshahaf

This comment has been minimized.

Copy link

commented Jul 14, 2017

@TheLocehiliosan Yes, it was I.

Regarding the fix, the important thing is to have no window in which the directory exists and is world readable. I suggest mkdir -m700 or (umask 077 && mkdir).

I agree that the severity isn't clear cut here. A successful exploit requires several planets to align. (And for that matter, the exploitability of "dir is created 0755, chmod'd 0700, and then a secret file is created in it" may differ between linux and kFreeBSD...) To cut a long story short, feel free to adjust the severity as you see fit. :-)

Thanks for the timely response.

@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 14, 2017

Thanks for the feedback. I'll push ahead with these planned changes in about a week when I'll have time to focus on yadm again.

@danielshahaf

This comment has been minimized.

Copy link

commented Jul 17, 2017

As noted downstream, this issue has been assigned the identifier CVE-2017-11353.

TheLocehiliosan added a commit that referenced this issue Jul 18, 2017

Create secured private dirs prior to merge (#74)
When cloning a repo which includes data in a .ssh or .gnupg directory:

* If those directories exist at the time of cloning, yadm will assert
  user only privileges for those directories.

* If those directories do not exist at the time of cloning, yadm will
  create the directories and assert user only privileges for those
  directories.

All of these actions are prior to merging the fetch data into the
work-tree.
@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 18, 2017

@danielshahaf @czchen - I've published a change to the dev branch. Can you confirm this resolves your concerns?

@czchen

This comment has been minimized.

Copy link

commented Jul 19, 2017

looks good for me

@danielshahaf

This comment has been minimized.

Copy link

commented Jul 19, 2017

The code does mkdir foo; chmod foo. Suppose an attacker does opendir("foo") before the chmod, will he be able to use openat(3) after the chmod?

I don't know the answer, but using mkdir -m or umask instead would resolve this concern.

Also I suggest to mention the CVE number in the log message.

TheLocehiliosan added a commit that referenced this issue Jul 19, 2017

Create secured private dirs prior to merge (#74)
This directly addresses CVE-2017-11353.

When cloning a repo which includes data in a .ssh or .gnupg directory:

* If those directories exist at the time of cloning, yadm will assert
  user only privileges for those directories.

* If those directories do not exist at the time of cloning, yadm will
  create the directories and assert user only privileges for those
  directories.

All of these actions are prior to merging the fetch data into the
work-tree.
@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 19, 2017

@danielshahaf Yes, you are absolutely right with the mkdir -m. That thought got lost when I was handling the case where a private directory already exists. I've fixed that and pushed an update to the dev branch, and the commit messages also mentions the CVE number. How does it look to you now?

TheLocehiliosan added a commit that referenced this issue Jul 19, 2017

Create secured private dirs prior to merge (#74)
This directly addresses CVE-2017-11353.

When cloning a repo which includes data in a .ssh or .gnupg directory:

* If those directories exist at the time of cloning, yadm will assert
  user only privileges for those directories.

* If those directories do not exist at the time of cloning, yadm will
  create the directories and assert user only privileges for those
  directories.

All of these actions are prior to merging the fetch data into the
work-tree.

TheLocehiliosan added a commit that referenced this issue Jul 19, 2017

Create secured private dirs prior to merge (#74)
This directly addresses CVE-2017-11353.

When cloning a repo which includes data in a .ssh or .gnupg directory:

* If those directories exist at the time of cloning, yadm will assert
  user only privileges for those directories.

* If those directories do not exist at the time of cloning, yadm will
  create the directories and assert user only privileges for those
  directories.

All of these actions are prior to merging the fetch data into the
work-tree.
@danielshahaf

This comment has been minimized.

Copy link

commented Jul 19, 2017

I wouldn't chmod the directory if it already exists. I'd either just use it and trust the user to have created it correctly, or alternatively check its permissions and bail out if they are too open to my liking.

TheLocehiliosan added a commit that referenced this issue Jul 20, 2017

Create secured private dirs prior to merge (#74)
This directly addresses CVE-2017-11353.

When cloning a repo which includes data in a .ssh or .gnupg directory,
if those directories do not exist at the time of cloning, yadm will
create the directories and assert user only privileges for those
directories. This is done prior to merging the fetched data into the
work-tree.
@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 20, 2017

@danielshahaf Sure. I actually want to do the least amount of changes that address the concerns you've brought up. I've pushed another change to the dev branch. Would you be so kind as to confirm this addresses your concerns? I want to be sure before doing a release. Thanks.

@danielshahaf

This comment has been minimized.

Copy link

commented Jul 20, 2017

@TheLocehiliosan Sure, that makes sense. The mkdir looks secure now.

What I have not reviewed is 1) the new test, 2) whether any other codepaths need patching (there is a git pull codepath other than clone, isn't there?), 3) whether $YADM_WORK exists at the time of the mkdir (if it doesn't, it'd be created with mode 0700, instead of with the umask-default permissions in current master).

@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 20, 2017

Regarding other code paths, aside from clone, all work-tree operations are done by Git directly.

Regarding $YADM_WORK being created as 0700, I'm fairly sure that the standard operation of mkdir with -p and -m combined, is to apply the specified mask for the last directory only, and parent directories will get their access mode from umask (if they need to be created).

Thanks

@danielshahaf

This comment has been minimized.

Copy link

commented Jul 20, 2017

Regarding other code paths, aside from clone, all work-tree operations are done by Git directly.

So if a git pull creates a .ssh dir, that dir would be created with the default permissions, wouldn't it?

@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 20, 2017

@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 21, 2017

After thinking a bit more about it, perhaps it is easiest to always create the directories prior to Git operations. To remove the heavy-handedness I'll have that only apply to Git commands that change the work-tree, and I'll create an option that allows users to disable it if they don't want that feature. However the directory creation during clone (if the repo tracks data in these directories) will continue to operate as it does in the dev branch now.

@danielshahaf does that plan sound satisfactory to you?

TheLocehiliosan added a commit that referenced this issue Jul 22, 2017

Create secured private dirs (#74)
Directories are created prior to merge during clone, and prior to any
Git command run.

This directly addresses CVE-2017-11353.

When cloning a repo which includes data in a .ssh or .gnupg directory,
if those directories do not exist at the time of cloning, yadm will
create the directories with mask 0700 prior to merging the fetched data
into the work-tree.

When running a Git command and .ssh or .gnupg directories do not exist,
create those directories with mask 0700 prior to running the Git
command. However, do not create those directories if
yadm.auto-private-dirs is false.
@TheLocehiliosan

This comment has been minimized.

Copy link
Owner Author

commented Jul 28, 2017

@danielshahaf - Any further thoughts on the latest version of the dev branch? I just want to be sure I'm fully addressing your concerns before I publish a release.

@danielshahaf

This comment has been minimized.

Copy link

commented Jul 29, 2017

@TheLocehiliosan Sorry, no time to review the new diff right now :(

TheLocehiliosan added a commit that referenced this issue Aug 23, 2017

Create secured private dirs (#74)
Directories are created prior to merge during clone, and prior to any
Git command run.

This directly addresses CVE-2017-11353.

When cloning a repo which includes data in a .ssh or .gnupg directory,
if those directories do not exist at the time of cloning, yadm will
create the directories with mask 0700 prior to merging the fetched data
into the work-tree.

When running a Git command and .ssh or .gnupg directories do not exist,
create those directories with mask 0700 prior to running the Git
command. However, do not create those directories if
yadm.auto-private-dirs is false.

TheLocehiliosan added a commit that referenced this issue Aug 23, 2017

Release 1.11.1
Update version number and update documentation

* Create private dirs prior to merge (#74)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.