Create private .ssh and .gnupg directories prior to merge (during clone) #74
Comments
|
@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 |
|
@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 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. |
|
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. |
|
As noted downstream, this issue has been assigned the identifier 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 @czchen - I've published a change to the dev branch. Can you confirm this resolves your concerns? |
|
looks good for me |
|
The code does I don't know the answer, but using Also I suggest to mention the CVE number in the log message. |
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 Yes, you are absolutely right with the |
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.
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.
|
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. |
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.
|
@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 |
|
@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 |
|
Regarding other code paths, aside from Regarding Thanks |
So if a |
|
Yes that is correct. However, as soon as the Git operation is complete, the permissions are asserted by yadm.
It is probably possible to test if the repo tracks data in .ssh prior to a merge (as is done during the `yadm clone` now, but in the case of a pull, Git does a fetch and merge in a single operation. I'm not sure of a reasonable method to determine prior to a pull if the commits present in the remote repo contain data from .ssh when there previously wasn't. Even if I were to query the remote repo prior to allowing a pull, there is the possibility that a new commit shows up between the query and the pull. I'm very open to some suggestion though.
It's possible to ALWAYS create these directories prior to any commands (on the chance that data in those directories is tracked), but that seems rather heavy-handed to me. But perhaps that is a way to allay concerns.
Again, no one should maintain private data in .ssh within the repo itself. If anyone does that, their data is also exposed in their repo. Any private data should be handled using the encrypt/decrypt features. I think providing individuals use tools sensibly, that it is possible that configuration data is momentarily readable prior being secured, but that private data is not exposed.
|
|
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? |
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.
|
@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. |
|
@TheLocehiliosan Sorry, no time to review the new diff right now :( |
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.
Update version number and update documentation * Create private dirs prior to merge (#74)
As referenced in Debian #868300, permissions on
.sshand.gnupgdirectories are restricted after an initial clone is complete. Git employs the user's umask to set permissions for any files/directories created.If
.sshor.gnupgdirectories 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/configand.ssh/id_rsa. Also suppose the.ssh/configis limited configuration data only, and the.ssh/id_rsais their private key. If.ssh/id_rsais included using the encryption function the following scenarios are possible:If
.sshalready exists and is secured.sshpermissions are not changed (remain secure)If
.sshdoes not yet exist.sshand.ssh/configwill be created with the default umaskyadm decryptis run (either by the user or via bootstrap) and the.ssh/id_rsais put into the.ssh..ssh's permissions will already be restricted at this pointRegardless, I plan to update clone operations. If there are files tracked in the repo under locations
.sshor.gnupgand 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.The text was updated successfully, but these errors were encountered: