Re: Too-loose checks for applying safe.directory rules?

Re: Too-loose checks for applying safe.directory rules?

[Jonathan Nieder]

(+cc: git@vger, git-security -> bcc)
Hi,

Taylor Blau wrote:

> Hi all,
>
> I was skimming the Hacker News comments on my blog post covering the
> latest pair of CVEs, and this[1] comment stuck out to me.
>
> Looking at 8959555cee (setup_git_directory(): add an owner check for the
> top-level directory, 2022-03-02), I wonder why the `safe_directory_cb()`
> callback doesn't bother to check that `key` is `safe.directory`.
>
> Indeed, our checks seem too loose here. Initializing a repository as
> root:
>
>     $ su
>     # git init repo
>
> Then trying to run "git status" inside of that repo as my normal user
> gives the expected error:
>
>     $ git status
>     fatal: unsafe repository ('/home/repo' is owned by someone else)
>     To add an exception for this directory, call:
>
>       git config --global --add safe.directory /home/repo
>
> But doing the following:
>
>     $ git config --global --add foo.bar /home/repo
>
> tricks Git into thinking that _any_ value which looks like a path in the
> "early config" scope can be interpreted as if the key were
> safe.directory, even when it is not:
>
>     $ git status
>     On branch master
>
>     No commits yet
>
>     nothing to commit (create/copy files and use "git add" to track)
>
> The author of [1] sent a PR to the git/git repo on GitHub [2], so I
> don't think there's any value in doing another coordinated release here.

Thanks, Taylor.  I'm taking the liberty of moving to the main Git list.

> We should certainly fix this before 2.36 is released, but should
> probably apply those patches down to the suite of minor versions
> released today, too.
>
> It's entirely possible I'm holding it wrong and/or missing something
> here, and I'd be happy to be wrong here.
>
> Thanks,
> Taylor
>
> [1]: https://news.ycombinator.com/item?id=31010604
> [2]: https://github.com/git/git/pull/1235

Thanks,
Jonathan

Re: Too-loose checks for applying safe.directory rules?

[CB Bailey]

On 13/04/2022 06:26, Jonathan Nieder wrote:
> (+cc: git@vger, git-security -> bcc)
> Hi,
> 
> Taylor Blau wrote:
> 
>> Hi all,
>>
>> I was skimming the Hacker News comments on my blog post covering the
>> latest pair of CVEs, and this[1] comment stuck out to me.
>>
>> Looking at 8959555cee (setup_git_directory(): add an owner check for the
>> top-level directory, 2022-03-02), I wonder why the `safe_directory_cb()`
>> callback doesn't bother to check that `key` is `safe.directory`.
>>
>> Indeed, our checks seem too loose here. Initializing a repository as
>> root:
>>
>>      $ su
>>      # git init repo
>>
>> Then trying to run "git status" inside of that repo as my normal user
>> gives the expected error:
>>
>>      $ git status
>>      fatal: unsafe repository ('/home/repo' is owned by someone else)
>>      To add an exception for this directory, call:
>>
>>        git config --global --add safe.directory /home/repo
>>
>> But doing the following:
>>
>>      $ git config --global --add foo.bar /home/repo
>>
>> tricks Git into thinking that _any_ value which looks like a path in the
>> "early config" scope can be interpreted as if the key were
>> safe.directory, even when it is not:
>>
>>      $ git status
>>      On branch master
>>
>>      No commits yet
>>
>>      nothing to commit (create/copy files and use "git add" to track)

I just wanted to send a belated "thank you for fixing this" as the other 
aspect of this is that if you had any configuration variable configured 
to an empty string (in my case 'http.proxy'), then any 'safe.directory' 
which happended to be parsed before this was ignored. This bug was the 
cause of me running git in a debugger in production for the first time 
in a very long time as we tried to work out why 'safe.directory' was not 
working.

CB