fix(git-lfs): fixed security problem with allowing off-chain attacks to replace OID data without validating hash for already present oids.

- Fixes  RCCE-141

vcsserver/git_lfs/app.py

@@ -14,7 +14,7 b''
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software Foundation,
 # Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301  USA
-
+import hashlib
 import re
 import logging
 
@@ -160,8 +160,14 b' def lfs_objects_oid_upload(request):'
     engine = store.get_engine(mode='wb')
     log.debug('LFS: starting chunked write of LFS oid: %s to storage', oid)
 
+    # validate if OID is not by any chance already in the store
+    if store.has_oid():
+        log.debug('LFS: oid %s exists in store', oid)
+        return {'upload': 'ok', 'state': 'in-store'}
+
     body = request.environ['wsgi.input']
 
+    digest = hashlib.sha256()
     with engine as f:
         blksize = 64 * 1024  # 64kb
         while True:
@@ -175,10 +181,17 b' def lfs_objects_oid_upload(request):'
 
             if not chunk:
                 break
+            f.write(chunk)
+            digest.update(chunk)
 
-            f.write(chunk)
+        hex_digest = digest.hexdigest()
+        digest_check = hex_digest == oid
+        if not digest_check:
+            engine.cleanup() # trigger cleanup so we don't save mismatch OID into the store
+            return write_response_error(
+                HTTPBadRequest, f'oid {oid} does not match expected sha {hex_digest}')
 
-    return {'upload': 'ok'}
+    return {'upload': 'ok', 'state': 'written'}
 
 
 def lfs_objects_oid_download(request):
@@ -223,12 +236,10 b' def lfs_objects_verify(request):'
 
     store_size = store.size_oid()
     if store_size != size:
-        msg = 'requested file size mismatch store size:{} requested:{}'.format(
-            store_size, size)
-        return write_response_error(
-            HTTPUnprocessableEntity, msg)
+        msg = f'requested file size mismatch store size:{store_size} requested:{size}'
+        return write_response_error(HTTPUnprocessableEntity, msg)
 
-    return {'message': {'size': 'ok', 'in_store': 'ok'}}
+    return {'message': {'size': store_size, 'oid': oid}}
 
 
 def lfs_objects_lock(request):

vcsserver/git_lfs/lib.py

@@ -133,12 +133,16 b' class LFSOidStore:'
         """
 
         class StoreEngine:
+            _cleanup = None
             def __init__(self, mode, store_path, oid_path, tmp_oid_path):
                 self.mode = mode
                 self.store_path = store_path
                 self.oid_path = oid_path
                 self.tmp_oid_path = tmp_oid_path
 
+            def cleanup(self):
+                self._cleanup = True
+
             def __enter__(self):
                 if not os.path.isdir(self.store_path):
                     os.makedirs(self.store_path)
@@ -149,9 +153,13 b' class LFSOidStore:'
                 return fd
 
             def __exit__(self, exc_type, exc_value, traceback):
-                # close tmp file, and rename to final destination
                 self.fd.close()
-                shutil.move(self.tmp_oid_path, self.oid_path)
+
+                if self._cleanup is None:
+                    # close tmp file, and rename to final destination
+                    shutil.move(self.tmp_oid_path, self.oid_path)
+                else:
+                    os.remove(self.tmp_oid_path)
 
         return StoreEngine(
             mode, self.store_path, self.oid_path, self.tmp_oid_path)

vcsserver/git_lfs/tests/test_lfs_app.py

@@ -100,22 +100,27 b' class TestLFSApplication:'
 
     def test_app_batch_api_download_missing_object(
             self, git_lfs_app, http_auth):
-        params = {'operation': 'download',
-                  'objects': [{'oid': '123', 'size': '1024'}]}
+        params = {
+            'operation': 'download',
+            'objects': [{'oid': '123', 'size': '1024'}]
+        }
         response = git_lfs_app.post_json(
             '/repo/info/lfs/objects/batch', params=params,
             extra_environ=http_auth)
 
         expected_objects = [
-            {'authenticated': True,
-             'errors': {'error': {
-                 'code': 404,
-                 'message': 'object: 123 does not exist in store'}},
-             'oid': '123',
-             'size': '1024'}
+            {
+                'oid': '123',
+                'size': '1024',
+                'authenticated': True,
+                'errors': {'error': {'code': 404, 'message': 'object: 123 does not exist in store'}},
+            }
         ]
+
         assert json.loads(response.text) == {
-            'objects': expected_objects, 'transfer': 'basic'}
+            'objects': expected_objects,
+            'transfer': 'basic'
+        }
 
     def test_app_batch_api_download(self, git_lfs_app, http_auth):
         oid = '456'
@@ -142,7 +147,9 b' class TestLFSApplication:'
              'size': '1024'}
         ]
         assert json.loads(response.text) == {
-            'objects': expected_objects, 'transfer': 'basic'}
+            'objects': expected_objects,
+            'transfer': 'basic'
+        }
 
     def test_app_batch_api_upload(self, git_lfs_app, http_auth):
         params = {'operation': 'upload',
@@ -151,21 +158,31 b' class TestLFSApplication:'
             '/repo/info/lfs/objects/batch', params=params,
             extra_environ=http_auth)
         expected_objects = [
-            {'authenticated': True,
-             'actions': {
-                 'upload': {
-                     'header': {'Authorization': 'Basic XXXXX',
-                                 'Transfer-Encoding': 'chunked'},
-                     'href': 'http://localhost/repo/info/lfs/objects/123'},
-                 'verify': {
-                     'header': {'Authorization': 'Basic XXXXX'},
-                     'href': 'http://localhost/repo/info/lfs/verify'}
-             },
-             'oid': '123',
-             'size': '1024'}
+            {
+                'authenticated': True,
+                'actions': {
+                    'upload': {
+                        'header': {
+                            'Authorization': 'Basic XXXXX',
+                            'Transfer-Encoding': 'chunked'
+                        },
+                        'href': 'http://localhost/repo/info/lfs/objects/123'
+                    },
+                    'verify': {
+                        'header': {
+                            'Authorization': 'Basic XXXXX'
+                        },
+                        'href': 'http://localhost/repo/info/lfs/verify'
+                    }
+                },
+                'oid': '123',
+                'size': '1024'
+            }
         ]
         assert json.loads(response.text) == {
-            'objects': expected_objects, 'transfer': 'basic'}
+            'objects': expected_objects,
+            'transfer': 'basic'
+        }
 
     def test_app_batch_api_upload_for_https(self, git_lfs_https_app, http_auth):
         params = {'operation': 'upload',
@@ -178,7 +195,7 b' class TestLFSApplication:'
              'actions': {
                  'upload': {
                      'header': {'Authorization': 'Basic XXXXX',
-                                 'Transfer-Encoding': 'chunked'},
+                                'Transfer-Encoding': 'chunked'},
                      'href': 'https://localhost/repo/info/lfs/objects/123'},
                  'verify': {
                      'header': {'Authorization': 'Basic XXXXX'},
@@ -206,7 +223,8 b' class TestLFSApplication:'
             status=404)
 
         assert json.loads(response.text) == {
-            'message': 'oid `missing` does not exists in store'}
+            'message': 'oid `missing` does not exists in store'
+        }
 
     def test_app_verify_api_size_mismatch(self, git_lfs_app):
         oid = 'existing'
@@ -221,8 +239,8 b' class TestLFSApplication:'
             '/repo/info/lfs/verify', params=params, status=422)
 
         assert json.loads(response.text) == {
-            'message': 'requested file size mismatch '
-                        'store size:11 requested:1024'}
+            'message': 'requested file size mismatch store size:11 requested:1024'
+        }
 
     def test_app_verify_api(self, git_lfs_app):
         oid = 'existing'
@@ -237,13 +255,13 b' class TestLFSApplication:'
             '/repo/info/lfs/verify', params=params)
 
         assert json.loads(response.text) == {
-            'message': {'size': 'ok', 'in_store': 'ok'}}
+            'message': {'size': 11, 'oid': oid}
+        }
 
     def test_app_download_api_oid_not_existing(self, git_lfs_app):
         oid = 'missing'
 
-        response = git_lfs_app.get(
-            '/repo/info/lfs/objects/{oid}'.format(oid=oid), status=404)
+        response = git_lfs_app.get(f'/repo/info/lfs/objects/{oid}', status=404)
 
         assert json.loads(response.text) == {
             'message': 'requested file with oid `missing` not found in store'}
@@ -256,19 +274,37 b' class TestLFSApplication:'
         with open(oid_path, 'wb') as f:
             f.write(safe_bytes('OID_CONTENT'))
 
-        response = git_lfs_app.get(
-            '/repo/info/lfs/objects/{oid}'.format(oid=oid))
+        response = git_lfs_app.get(f'/repo/info/lfs/objects/{oid}')
         assert response
 
     def test_app_upload(self, git_lfs_app):
-        oid = 'uploaded'
+        oid = '65f23e22a9bfedda96929b3cfcb8b6d2fdd34a2e877ddb81f45d79ab05710e12'
 
         response = git_lfs_app.put(
-            '/repo/info/lfs/objects/{oid}'.format(oid=oid), params='CONTENT')
+            f'/repo/info/lfs/objects/{oid}', params='CONTENT')
 
-        assert json.loads(response.text) == {'upload': 'ok'}
+        assert json.loads(response.text) == {'upload': 'ok', 'state': 'written'}
 
         # verify that we actually wrote that OID
         oid_path = LFSOidStore(oid=oid, repo=None, store_location=git_lfs_app._store).oid_path
         assert os.path.isfile(oid_path)
         assert 'CONTENT' == open(oid_path).read()
+
+        response = git_lfs_app.put(
+            f'/repo/info/lfs/objects/{oid}', params='CONTENT')
+
+        assert json.loads(response.text) == {'upload': 'ok', 'state': 'in-store'}
+
+
+    def test_app_upload_wrong_sha(self, git_lfs_app):
+        oid = 'i-am-a-wrong-sha'
+
+        response = git_lfs_app.put(f'/repo/info/lfs/objects/{oid}', params='CONTENT', status=400)
+
+        assert json.loads(response.text) == {
+            'message': 'oid i-am-a-wrong-sha does not match expected sha '
+                       '65f23e22a9bfedda96929b3cfcb8b6d2fdd34a2e877ddb81f45d79ab05710e12'}
+
+        # check this OID wasn't written to store
+        response = git_lfs_app.get(f'/repo/info/lfs/objects/{oid}', status=404)
+        assert json.loads(response.text) == {'message': 'requested file with oid `i-am-a-wrong-sha` not found in store'}

vcsserver/git_lfs/tests/test_lib.py

@@ -24,7 +24,7 b' from vcsserver.git_lfs.lib import OidHan'
 @pytest.fixture()
 def lfs_store(tmpdir):
     repo = 'test'
-    oid = '123456789'
+    oid = '65f23e22a9bfedda96929b3cfcb8b6d2fdd34a2e877ddb81f45d79ab05710e12'
     store = LFSOidStore(oid=oid, repo=repo, store_location=str(tmpdir))
     return store
 
@@ -63,7 +63,8 b' class TestOidHandler:'
         assert response is None
         assert has_errors['error'] == {
             'code': 404,
-            'message': 'object: 123456789 does not exist in store'}
+            'message': 'object: 65f23e22a9bfedda96929b3cfcb8b6d2fdd34a2e877ddb81f45d79ab05710e12 does not exist in store'
+        }
 
     def test_download_oid(self, oid_handler):
         store = oid_handler.get_store()