PR#5482: fix: generate_archive() follows symbolic links in temporary clones - pagure

pagure

#5482 fix: generate_archive() follows symbolic links in temporary clones

Merged 2 years ago by wombelix. Opened 2 years ago by wombelix.

wombelix/pagure fix_security_issue_rhbz2280030_generate_archive_follows_symlink into master

fix: generate_archive() follows symbolic links in temporary clones

Dominik Wombacher • 2 years ago

c6ced5d

pagure/lib/git.py

file modified

+12 -1

		`@@ -14,6 +14,7 @@`
		`import logging`
		`import os`
		`import shutil`
		`+ import stat`
		`import subprocess`
		`import tarfile`
		`import tempfile`
		`@@ -2842,7 +2843,17 @@`
		`def addToZip(zf, path, zippath):`
		`if _exclude_git(path):`
		`return`
		`- if os.path.isfile(path):`
		`+ # if path is symlink, add actual link and not target to zip`
		`+ if os.path.islink(path):`
		`+ zi = zipfile.ZipInfo.from_file(path, zippath)`
		`+ zi.compress_type = zipfile.ZIP_DEFLATED`
		`+ # System which created zip, 3 = Unix; 0 = Windows`
		`+ zi.create_system = 3`
		`+ # mark as a symlink`
		`+ zi.external_attr \|= stat.S_IFLNK << 16`
		`+ zf.writestr(zi, path)`
		`+ return`
		`+ elif os.path.isfile(path):`
		`zf.write(path, zippath, zipfile.ZIP_DEFLATED)`
		`elif os.path.isdir(path):`
		`if zippath:`

tests/test_pagure_flask_ui_archives.py

file modified

+73 -1

		`@@ -13,7 +13,7 @@`
		`import unittest`
		`import sys`
		`import os`
		`- import time`
		`+ from zipfile import ZipFile`

		`import mock`
		`import pygit2`
		`@@ -123,6 +123,78 @@`

		`self.assertEqual(os.listdir(self.archive_path), [])`

		`+ # All tests against test3 repo complete, re-use repo for symlink test`
		`+ def test_project_with_symlink_zip(self):`
		`+ """Test that symlink rather than target file gets added to zip"""`
		`+ filename = "os-release"`
		`+ symlinkfile_target = "/etc/os-release"`
		`+ directory = "etc"`
		`+ symlinkdir_target = "/etc"`
		`+ readme = "README.rst"`
		`+ archivename = "test3.zip"`
		`+ namespace = "somenamespace"`
		`+ reponame = "test3"`
		`+ repopath = os.path.join(`
		`+ self.path, "repos", namespace, "%s.git" % reponame`
		`+ )`
		`+ repo = pygit2.Repository(repopath)`
		`+ archivepath = os.path.join(self.archive_path, namespace, reponame)`
		`+ tests.add_commit_git_repo(`
		`+ repopath,`
		`+ ncommits=1,`
		`+ filename=filename,`
		`+ symlink_to=symlinkfile_target,`
		`+ )`
		`+ tests.add_commit_git_repo(`
		`+ repopath,`
		`+ ncommits=1,`
		`+ filename=directory,`
		`+ symlink_to=symlinkdir_target,`
		`+ )`
		`+ tests.add_readme_git_repo(repopath)`
		`+ commit = repo.head.target.hex`
		`+`
		`+ with mock.patch.dict(`
		`+ "pagure.config.config",`
		`+ {"ARCHIVE_FOLDER": os.path.join(self.path, "archives")},`
		`+ ):`
		`+ output = self.app.get(`
		`+ "/%s/%s/archive/%s/%s"`
		`+ % (namespace, reponame, commit, archivename),`
		`+ follow_redirects=True,`
		`+ )`
		`+`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ # Was a subfolder for the correct commit created?`
		`+ self.assertEqual(os.listdir(archivepath), [commit])`
		`+`
		`+ archivepath_commit = os.path.join(archivepath, commit)`
		`+ # Does the subfolder contain the expected zip file?`
		`+ self.assertEqual(os.listdir(archivepath_commit), [archivename])`
		`+`
		`+ archivepath_zip = os.path.join(archivepath_commit, archivename)`
		`+ # Is the test file in the zip archive a symlink?`
		`+ with ZipFile(os.path.join(archivepath_zip)) as testzip:`
		`+ self.assertIn(`
		`+ "lrw-r--r--",`
		`+ str(testzip.getinfo("%s/%s" % (reponame, filename))),`
		`+ )`
		`+`
		`+ # Is the test directory in the zip archive a symlink?`
		`+ with ZipFile(os.path.join(archivepath_zip)) as testzip:`
		`+ self.assertIn(`
		`+ "?rwxr-xr-x",`
		`+ str(testzip.getinfo("%s/%s/" % (reponame, directory))),`
		`+ )`
		`+`
		`+ # Is the readme still an actual file in the zip archive?`
		`+ with ZipFile(os.path.join(archivepath_zip)) as testzip:`
		`+ self.assertIn(`
		`+ "-rw-r--r--",`
		`+ str(testzip.getinfo("%s/%s" % (reponame, readme))),`
		`+ )`
		`+`
		`def test_project_no_tag(self):`
		`"""Test getting the archive of a non-empty project but without`
		`tags."""`

wombelix commented 2 years ago

If the repo contains symlinks, add actual link rather than the target content to the zip archive.
Avoids following symlinks and inclusion of data from outside the repository.

Vulnerability discovered by Thomas Chauchefoin thomas@chauchefoin.fr

Fixes: rhbz#2280030

Signed-off-by: Dominik Wombacher dominik@wombacher.cc

Metadata Update from @wombelix:
- Request assigned

2 years ago

wombelix commented 2 years ago

Patch reviewed and validated in Bugzilla. Tests performed locally, all passed.

rebased onto c6ced5d

2 years ago

Pull-Request has been merged by wombelix

2 years ago