CVE-2024-4981: Pagure: _update_file_in_git() follows symbolic links in temporary clones

Description of problem: In pagure/lib/git.py, the method updatefileingit() allows updating files on Pagure repositories directly from the web interface. Under the hood, it clones the repository to a temporary folder, performs the write operation, commits the changes and pushes it back to either the default branch or a new one.

def updatefileingit( repo, branch, branchto, filename, content, message, user, email ): # [...] with TemporaryClone(repo, "main", "editfile") as tempclone: # [...] filepath = os.path.join(newpath, filename) # [...] with open(filepath, "wb") as stream: stream.write(content.replace("\r", "").encode("utf-8")) # [...] newrepo.createcommit( # [...] ) # [...] tempclone.push( user.username, nbranchref.name if nbranchref else branchref.name, branchto, )

This code doesn't take enough precautions when dealing with symbolic links: if filepath points to one, open(filepath) will follow it. This link can point outside of the temporary clone folder.

Version-Release number of selected component (if applicable): Likely introduced in commit 54335c2 in release 0.1.11, and verified on latest commit as of today (1b36cb8).

How reproducible: This bug can be reliably exploited on the latest development version of Pagure; see steps below.

Steps to Reproduce: 1. Create a new repository on a test Pagure instance; 2. Clone it locally; 3. From the local clone, run: ln -s /tmp/foo foo; 4. Commit this file, and push the commit back to Pagure; 5. From the web interface, in the "Files" tab, click on the one named foo, and then on the button "Edit"; 6. Put anything in the textarea, and then click on "Commit changes"; 7. Notice that the file /tmp/foo was created on the Pagure server.

Actual results: Calls to updatefileingit() on symbolic links allow attackers to write fully controlled data to arbitrary paths (as long as the system user git has the right permissions on the destination).

I could demonstrate the exploitation of this vulnerability and gain arbitrary code execution on stg.pagure.io by overriding /srv/git/.bashrc. As a proof, here's the output of name -a: Linux pagure-stg01.fedoraproject.org 4.18.0-513.11.1.el89.x8664 #1 SMP Thu Dec 7 03:06:13 EST 2023 x8664 x8664 x8664 GNU/Linux. I've since removed my changes to this file.

Expected results: Calls to updatefileingit() on symbolic links should only be performed if the destination of link stays "within" the temporary clone folder. At first glance, I would not use os.readlink() here, as it would not catch cases where several links are chained; os.path.realpath() seems more appropriate.

Additional info: I haven't had the time to work on a patch for this one, I'll try to submit it in the coming days.

— Red Hat