kallithea Changeset - fa3365c94064

Changeset - fa3365c94064

Parent rev.

Child rev.

[Not reviewed]

stable

0 2 0

Mads Kiilerich - 8 years ago 2018-05-07 11:38:40
mads@kiilerich.com

repos: introduce low level check of clone URIs to prevent direct file system access to local repos

This is already checked in web form validation, but also check at low level to
make sure API access enforce the same invariants.

This issue was found and reported by
Kacper Szurek
https://security.szurek.pl/

2 files changed with 29 insertions and 17 deletions:

kallithea/model/repo.py

kallithea/tests/api/api_base.py

0 comments (0 inline, 0 general)

kallithea/model/repo.py

➞

Show inline comments

@@ @@ -24,25 +24,25 @@ Original author and date, and relevant c @@
 :copyright: (c) 2013 RhodeCode GmbH, and others.
 :license: GPLv3, see LICENSE.md for more details.
 """
 import os
 import shutil
 import logging
 import traceback
 from datetime import datetime
 from sqlalchemy.orm import subqueryload
 from kallithea.lib.utils import make_ui
+from kallithea.lib.utils import make_ui, is_valid_repo_uri
 from kallithea.lib.vcs.backends import get_backend
 from kallithea.lib.compat import json
 from kallithea.lib.utils2 import LazyProperty, safe_str, safe_unicode, \
     remove_prefix, obfuscate_url_pw, get_current_authuser
 from kallithea.lib.caching_query import FromCache
 from kallithea.lib.hooks import log_delete_repository
 from kallithea.model import BaseModel
 from kallithea.model.db import Repository, UserRepoToPerm, UserGroupRepoToPerm, \
     UserRepoGroupToPerm, UserGroupRepoGroupToPerm, User, Permission, \
     Statistics, UserGroup, Ui, RepoGroup, RepositoryField
@@ @@ -326,24 +326,28 @@ class RepoModel(BaseModel): @@
             log.debug('Updating repo %s with params:%s', cur_repo, kwargs)
             for k in ['repo_enable_downloads',
                       'repo_description',
                       'repo_enable_locking',
                       'repo_landing_rev',
                       'repo_private',
                       'repo_enable_statistics',
                       ]:
                 if k in kwargs:
                     setattr(cur_repo, remove_prefix(k, 'repo_'), kwargs[k])
             clone_uri = kwargs.get('clone_uri')
             if clone_uri is not None and clone_uri != cur_repo.clone_uri_hidden:
                 # clone_uri is modified - if given a value, check it is valid
                 if clone_uri != '':
                     # will raise exception on error
                     is_valid_repo_uri(cur_repo.repo_type, clone_uri, make_ui('db', clear_session=False))
                 cur_repo.clone_uri = clone_uri
             if 'repo_name' in kwargs:
                 cur_repo.repo_name = cur_repo.get_new_name(kwargs['repo_name'])
             #if private flag is set, reset default permission to NONE
             if kwargs.get('repo_private'):
                 EMPTY_PERM = 'repository.none'
                 RepoModel().grant_user_permission(
                     repo=cur_repo, user='default', perm=EMPTY_PERM
+                )
                 #handle extra fields
@@ @@ -390,24 +394,27 @@ class RepoModel(BaseModel): @@
             repo_name_full = repo_name
             repo_name = repo_name.split(self.URL_SEPARATOR)[-1]
             new_repo = Repository()
             new_repo.repo_state = state
             new_repo.enable_statistics = False
             new_repo.repo_name = repo_name_full
             new_repo.repo_type = repo_type
             new_repo.user = owner
             new_repo.group = repo_group
             new_repo.description = description or repo_name
             new_repo.private = private
             if clone_uri:
                 # will raise exception on error
                 is_valid_repo_uri(repo_type, clone_uri, make_ui('db', clear_session=False))
             new_repo.clone_uri = clone_uri
             new_repo.landing_rev = landing_rev
             new_repo.enable_statistics = enable_statistics
             new_repo.enable_locking = enable_locking
             new_repo.enable_downloads = enable_downloads
             if repo_group:
                 new_repo.enable_locking = repo_group.enable_locking
             if fork_of:
                 parent_repo = fork_of
@@ @@ -668,29 +675,25 @@ class RepoModel(BaseModel): @@
             if obj is not None:
                 self.sa.delete(obj)
         except Exception:
             log.error(traceback.format_exc())
             raise
     def _create_filesystem_repo(self, repo_name, repo_type, repo_group,
                                 clone_uri=None, repo_store_location=None):
         """
         Makes repository on filesystem. Operation is group aware, meaning that it will create
         a repository within a group, and alter the paths accordingly to the group location.
         :param repo_name:
         :param alias:
         :param parent:
         :param clone_uri:
         :param repo_store_location:
         Note: clone_uri is low level and not validated - it might be a file system path used for validated cloning
         """
         from kallithea.lib.utils import is_valid_repo, is_valid_repo_group
         from kallithea.model.scm import ScmModel
         if '/' in repo_name:
             raise ValueError('repo_name must not contain groups got `%s`' % repo_name)
         if isinstance(repo_group, RepoGroup):
             new_parent_path = os.sep.join(repo_group.full_path_splitted)
         else:
             new_parent_path = repo_group or ''

kallithea/tests/api/api_base.py

➞

Show inline comments

@@ @@ -269,26 +269,30 @@ class _BaseTestApi(object): @@
         expected = ret
         self._compare_ok(id_, expected, given=response.body)
     def test_api_get_user_with_giving_userid_non_admin(self):
         id_, params = _build_data(self.apikey_regular, 'get_user',
                                   userid=self.TEST_USER_LOGIN)
         response = api_call(self, params)
         expected = 'userid is not the same as your user'
         self._compare_error(id_, expected, given=response.body)
     def test_api_pull(self):
         # Note: pulling from local repos is a mis-feature - it will bypass access control
         # ... but ok, if the path already has been set in the database
         repo_name = 'test_pull'
         r = fixture.create_repo(repo_name, repo_type=self.REPO_TYPE)
         # hack around that clone_uri can't be set to to a local path
         # (as shown by test_api_create_repo_clone_uri_local)
         r.clone_uri = os.path.join(TESTS_TMP_PATH, self.REPO)
         Session.add(r)
         Session.commit()
         id_, params = _build_data(self.apikey, 'pull',
                                   repoid=repo_name,)
         response = api_call(self, params)
         expected = {'msg': 'Pulled from `%s`' % repo_name,
                     'repository': repo_name}
         self._compare_ok(id_, expected, given=response.body)
@@ @@ -986,40 +990,37 @@ class _BaseTestApi(object): @@
         repo = RepoModel().get_by_repo_name(repo_name)
         self.assertNotEqual(repo, None)
         ret = {
             'msg': 'Created new repository `%s`' % repo_name,
             'success': True,
             'task': None,
+        }
         expected = ret
         self._compare_ok(id_, expected, given=response.body)
         fixture.destroy_repo(repo_name)
     def test_api_create_repo_clone_uri_local(self):
         # FIXME: cloning from local repo is a mis-feature - it will bypass access control
         # cloning from local repo was a mis-feature - it would bypass access control
         # TODO: introduce other test coverage of actual remote cloning
         clone_uri = os.path.join(TESTS_TMP_PATH, self.REPO)
         repo_name = u'api-repo'
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,
                                   clone_uri=clone_uri,
+        )
         response = api_call(self, params)
         expected = {
             'msg': 'Created new repository `%s`' % repo_name,
             'success': True,
             'task': None,
+        }
         self._compare_ok(id_, expected, given=response.body)
         expected = "failed to create repository `%s`" % repo_name
         self._compare_error(id_, expected, given=response.body)
         fixture.destroy_repo(repo_name)
     def test_api_create_repo_and_repo_group(self):
         repo_name = 'my_gr/api-repo'
         id_, params = _build_data(self.apikey, 'create_repo',
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         print params
         repo = RepoModel().get_by_repo_name(repo_name)
         self.assertNotEqual(repo, None)
@@ @@ -1148,89 +1149,97 @@ class _BaseTestApi(object): @@
                                   repo_name=repo_name,
                                   owner=TEST_USER_ADMIN_LOGIN,
                                   repo_type=self.REPO_TYPE,)
         response = api_call(self, params)
         expected = 'failed to create repository `%s`' % repo_name
         self._compare_error(id_, expected, given=response.body)
     @parameterized.expand([
         ('owner', {'owner': TEST_USER_REGULAR_LOGIN}),
         ('description', {'description': 'new description'}),
         ('active', {'active': True}),
         ('active', {'active': False}),
         ('clone_uri', {'clone_uri': 'http://example.com/repo'}),
         ('clone_uri', {'clone_uri': '/repo'}), # FIXME: pulling from local repo is a mis-feature - it will bypass access control
         ('clone_uri', {'clone_uri': 'http://example.com/repo'}), # will fail - pulling from non-existing repo should fail
         ('clone_uri', {'clone_uri': '/repo'}), # will fail - pulling from local repo was a mis-feature - it would bypass access control
         ('clone_uri', {'clone_uri': None}),
         ('landing_rev', {'landing_rev': 'branch:master'}),
         ('enable_statistics', {'enable_statistics': True}),
         ('enable_locking', {'enable_locking': True}),
         ('enable_downloads', {'enable_downloads': True}),
         ('name', {'name': 'new_repo_name'}),
         ('repo_group', {'group': 'test_group_for_update'}),
     ])
     def test_api_update_repo(self, changing_attr, updates):
         repo_name = 'api_update_me'
         repo = fixture.create_repo(repo_name, repo_type=self.REPO_TYPE)
         if changing_attr == 'repo_group':
             fixture.create_repo_group(updates['group'])
         id_, params = _build_data(self.apikey, 'update_repo',
                                   repoid=repo_name, **updates)
         response = api_call(self, params)
         if changing_attr == 'name':
             repo_name = updates['name']
         if changing_attr == 'repo_group':
             repo_name = '/'.join([updates['group'], repo_name])
         try:
             if changing_attr == 'clone_uri' and updates['clone_uri']:
                 expected = u'failed to update repo `%s`' % repo_name
                 self._compare_error(id_, expected, given=response.body)
             else:
             expected = {
                 'msg': 'updated repo ID:%s %s' % (repo.repo_id, repo_name),
                 'repository': repo.get_api_data()
+            }
             self._compare_ok(id_, expected, given=response.body)
         finally:
             fixture.destroy_repo(repo_name)
             if changing_attr == 'repo_group':
                 fixture.destroy_repo_group(updates['group'])
     @parameterized.expand([
         ('owner', {'owner': TEST_USER_REGULAR_LOGIN}),
         ('description', {'description': u'new description'}),
         ('active', {'active': True}),
         ('active', {'active': False}),
         ('clone_uri', {'clone_uri': 'http://example.com/repo'}),
         ('clone_uri', {'clone_uri': '/repo'}), # FIXME: pulling from local repo is a mis-feature - it will bypass access control
         ('clone_uri', {'clone_uri': 'http://example.com/repo'}), # will fail - pulling from non-existing repo should fail
         ('clone_uri', {'clone_uri': '/repo'}), # will fail - pulling from local repo was a mis-feature - it would bypass access control
         ('clone_uri', {'clone_uri': None}),
         ('landing_rev', {'landing_rev': 'branch:master'}),
         ('enable_statistics', {'enable_statistics': True}),
         ('enable_locking', {'enable_locking': True}),
         ('enable_downloads', {'enable_downloads': True}),
         ('name', {'name': u'new_repo_name'}),
         ('repo_group', {'group': u'test_group_for_update'}),
     ])
     def test_api_update_group_repo(self, changing_attr, updates):
         group_name = u'lololo'
         fixture.create_repo_group(group_name)
         repo_name = u'%s/api_update_me' % group_name
         repo = fixture.create_repo(repo_name, repo_group=group_name, repo_type=self.REPO_TYPE)
         if changing_attr == 'repo_group':
             fixture.create_repo_group(updates['group'])
         id_, params = _build_data(self.apikey, 'update_repo',
                                   repoid=repo_name, **updates)
         response = api_call(self, params)
         if changing_attr == 'name':
             repo_name = u'%s/%s' % (group_name, updates['name'])
         if changing_attr == 'repo_group':
             repo_name = u'/'.join([updates['group'], repo_name.rsplit('/', 1)[-1]])
         try:
             if changing_attr == 'clone_uri' and updates['clone_uri']:
                 expected = u'failed to update repo `%s`' % repo_name
                 self._compare_error(id_, expected, given=response.body)
             else:
             expected = {
                 'msg': 'updated repo ID:%s %s' % (repo.repo_id, repo_name),
                 'repository': repo.get_api_data()
+            }
             self._compare_ok(id_, expected, given=response.body)
         finally:
             fixture.destroy_repo(repo_name)
             if changing_attr == 'repo_group':
                 fixture.destroy_repo_group(updates['group'])
         fixture.destroy_repo_group(group_name)
     def test_api_update_repo_repo_group_does_not_exist(self):

0 comments (0 inline, 0 general)