CVE-2022-4724: Improper Access Control (CWE-284)
value: Improper Access Control in GitHub repository ikus060/rdiffweb prior to 2.5.5.
CWE description: The product does not restrict or incorrectly restricts access to a resource from an unauthorized actor.
CVSS v3 base score: 9.8
Published: 2022-12-27T15:15Z
Repository: https://github.com/ikus060/rdiffweb
Language: Python
Files changed: README.md, rdiffweb/core/model/__init__.py, rdiffweb/core/model/_sshkey.py, rdiffweb/core/model/_user.py, rdiffweb/core/model/tests/test_user.py
Commit date: 2022-12-23 14:03:15 -0500
Commit message: Make sure that all ssh keys are unique, regardless of the user

Diff: diff --git a/README.md b/README.md
index 8d357d640..a4294793e 100644
--- a/README.md
+++ b/README.md
@@ -113,10 +113,11 @@ Professional support for Rdiffweb is available by contacting [IKUS Soft](https:/
 * Fix loading of Charts in Status page
 * Ensure Gmail and other mail client doesn't create hyperlink automatically for any nodification sent by Rdiffweb to avoid phishing - credit to [Nehal Pillai](https://www.linkedin.com/in/nehal-pillai-02a854172)
 * Sent email notification to user when a new SSH Key get added - credit to [Nehal Pillai](https://www.linkedin.com/in/nehal-pillai-02a854172)
 * Ratelimit "Resend code to my email" in Two-Factor Authentication view - credit to [Nehal Pillai](https://www.linkedin.com/in/nehal-pillai-02a854172)
 * Username are not case-insensitive - credits to [raiders0786](https://www.linkedin.com/in/chirag-agrawal-770488144/)
+* Make sure that all ssh keys are unique, regardless of the user - credit to [Nehal Pillai](https://www.linkedin.com/in/nehal-pillai-02a854172)
 
 Breaking changes:
 
 * Username with different cases (e.g.: admin vs Ammin) are not supported. If your database contains such username make sure to remove them before upgrading otherwise Rdiffweb will not start.
 
diff --git a/rdiffweb/core/model/__init__.py b/rdiffweb/core/model/__init__.py
index df13f84c1..1c7e3636c 100644
--- a/rdiffweb/core/model/__init__.py
+++ b/rdiffweb/core/model/__init__.py
@@ -17,16 +17,16 @@
 
 import logging
 import sys
 
 import cherrypy
-from sqlalchemy import event
+from sqlalchemy import event, func
 from sqlalchemy.exc import IntegrityError
 
 from ._repo import RepoObject  # noqa
 from ._session import DbSession, SessionObject  # noqa
-from ._sshkey import SshKey  # noqa
+from ._sshkey import SshKey, sshkey_fingerprint_index  # noqa
 from ._token import Token  # noqa
 from ._user import DuplicateSSHKeyError, UserObject, user_username_index  # noqa
 
 Base = cherrypy.tools.db.get_base()
 
@@ -135,5 +135,24 @@ def db_after_create(target, connection, **kw):
                 '%s' % '\n'.join([str(k) for k in duplicate_users]),
             )
             logger.error(msg)
             print(msg, file=sys.stderr)
             raise SystemExit(12)
+
+    # Fix SSH Key uniqueness - since 2.5.4
+    if not _index_exists(connection, 'sshkey_fingerprint_index'):
+        duplicate_sshkeys = (
+            SshKey.query.with_entities(SshKey.fingerprint)
+            .group_by(SshKey.fingerprint)
+            .having(func.count(SshKey.fingerprint) > 1)
+        ).all()
+        try:
+            sshkey_fingerprint_index.create()
+        except IntegrityError:
+            msg = (
+                'Failure to upgrade your database to make SSH Keys unique. '
+                'You must downgrade and deleted duplicate SSH Keys. '
+                '%s' % '\n'.join([str(k) for k in duplicate_sshkeys]),
+            )
+            logger.error(msg)
+            print(msg, file=sys.stderr)
+            raise SystemExit(12)
diff --git a/rdiffweb/core/model/_sshkey.py b/rdiffweb/core/model/_sshkey.py
index 384219433..48b671b8c 100644
--- a/rdiffweb/core/model/_sshkey.py
+++ b/rdiffweb/core/model/_sshkey.py
@@ -14,16 +14,20 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import cherrypy
-from sqlalchemy import Column, Integer, Text
+from sqlalchemy import Column, Index, Integer, Text
 
 Base = cherrypy.tools.db.get_base()
 
 
 class SshKey(Base):
     __tablename__ = 'sshkeys'
     __table_args__ = {'sqlite_autoincrement': True}
     fingerprint = Column('Fingerprint', Text)
     key = Column('Key', Text, unique=True, primary_key=True)
     userid = Column('UserID', Integer, nullable=False)
+
+
+# Make finger print unique
+sshkey_fingerprint_index = Index('sshkey_fingerprint_index', SshKey.fingerprint, unique=True)
diff --git a/rdiffweb/core/model/_user.py b/rdiffweb/core/model/_user.py
index 9b06363c6..a448aac37 100644
--- a/rdiffweb/core/model/_user.py
+++ b/rdiffweb/core/model/_user.py
@@ -157,11 +157,11 @@ class UserObject(Base):
         """
         # Parse and validate ssh key
         assert key
         key = authorizedkeys.check_publickey(key)
 
-        # Remove option, replace comments.
+        # Remove option & Remove comment for SQL storage
         key = authorizedkeys.AuthorizedKey(
             options=None, keytype=key.keytype, key=key.key, comment=comment or key.comment
         )
 
         # If a filename exists, use it by default.
@@ -174,11 +174,12 @@ class UserObject(Base):
                 authorizedkeys.add(fh, key)
         else:
             # Also look in database.
             logger.info("add key [%s] to [%s] database", key, self.username)
             try:
-                SshKey(userid=self.userid, fingerprint=key.fingerprint, key=key.getvalue()).add().flush()
+                sshkey = SshKey(userid=self.userid, fingerprint=key.fingerprint, key=key.getvalue())
+                sshkey.add().flush()
             except IntegrityError:
                 raise DuplicateSSHKeyError(
                     _("Duplicate key. This key already exists or is associated to another user.")
                 )
         cherrypy.engine.publish('user_attr_changed', self, {'authorizedkeys': True})
diff --git a/rdiffweb/core/model/tests/test_user.py b/rdiffweb/core/model/tests/test_user.py
index 569ac52a0..401d5de32 100644
--- a/rdiffweb/core/model/tests/test_user.py
+++ b/rdiffweb/core/model/tests/test_user.py
@@ -323,19 +323,51 @@ class UserObjectTest(rdiffweb.test.WebCase):
         self.assertEqual("3c:99:ed:a7:82:a8:71:09:2c:15:3d:78:4a:8c:11:99", keys[0].fingerprint)
 
     def test_add_authorizedkey_duplicate(self):
         # Read the pub key
         key = self._read_ssh_key()
-        # Add the key to the user
+        # Given a user with a SSH Key
         userobj = UserObject.get_user(self.USERNAME)
         userobj.add_authorizedkey(key)
         userobj.commit()
-        # Add the same key
+
+        # When adding the same identical key.
+        # Then an error is raised
         with self.assertRaises(DuplicateSSHKeyError):
             userobj.add_authorizedkey(key)
             userobj.commit()
 
+    def test_add_authorizedkey_duplicate_new_comment(self):
+        # Read the pub key
+        key = self._read_ssh_key()
+        # Given a user with a SSH Key
+        userobj = UserObject.get_user(self.USERNAME)
+        userobj.add_authorizedkey(key)
+        userobj.commit()
+
+        # When adding the same key with a different comment
+        # Then an error is raised
+        with self.assertRaises(DuplicateSSHKeyError):
+            userobj.add_authorizedkey(key, comment="new comment")
+            userobj.commit()
+
+    def test_add_authorizedkey_duplicate_new_user(self):
+        # Read the pub key
+        key = self._read_ssh_key()
+        # Given a user with a SSH Key
+        userobj = UserObject.get_user(self.USERNAME)
+        userobj.add_authorizedkey(key)
+        userobj.commit()
+
+        # When adding the same key to a different user
+        # Then an error is raised
+        newuser = UserObject.add_user("newuser")
+        newuser.commit()
+        with self.assertRaises(DuplicateSSHKeyError):
+            newuser.add_authorizedkey(key, comment="new comment")
+            newuser.commit()
+
     def test_add_authorizedkey_with_file(self):
         """
         Add an ssh key for a user with an authorizedkey file.
         """
         userobj = UserObject.get_user(self.USERNAME)

Vulnerable code: from sqlalchemy import event
from ._sshkey import SshKey  # noqa
from sqlalchemy import Column, Integer, Text
        # Remove option, replace comments.
                SshKey(userid=self.userid, fingerprint=key.fingerprint, key=key.getvalue()).add().flush()
        # Add the key to the user
        # Add the same key
Fixed code: * Make sure that all ssh keys are unique, regardless of the user - credit to [Nehal Pillai](https://www.linkedin.com/in/nehal-pillai-02a854172)
from sqlalchemy import event, func
from ._sshkey import SshKey, sshkey_fingerprint_index  # noqa

    # Fix SSH Key uniqueness - since 2.5.4
    if not _index_exists(connection, 'sshkey_fingerprint_index'):
        duplicate_sshkeys = (
            SshKey.query.with_entities(SshKey.fingerprint)
            .group_by(SshKey.fingerprint)
            .having(func.count(SshKey.fingerprint) > 1)
        ).all()
        try:
            sshkey_fingerprint_index.create()
        except IntegrityError:
            msg = (
                'Failure to upgrade your database to make SSH Keys unique. '
                'You must downgrade and deleted duplicate SSH Keys. '
                '%s' % '\n'.join([str(k) for k in duplicate_sshkeys]),
            )
            logger.error(msg)
            print(msg, file=sys.stderr)
            raise SystemExit(12)
from sqlalchemy import Column, Index, Integer, Text


# Make finger print unique
sshkey_fingerprint_index = Index('sshkey_fingerprint_index', SshKey.fingerprint, unique=True)
        # Remove option & Remove comment for SQL storage
                sshkey = SshKey(userid=self.userid, fingerprint=key.fingerprint, key=key.getvalue())
                sshkey.add().flush()
        # Given a user with a SSH Key

        # When adding the same identical key.
        # Then an error is raised
    def test_add_authorizedkey_duplicate_new_comment(self):
        # Read the pub key
        key = self._read_ssh_key()
        # Given a user with a SSH Key
        userobj = UserObject.get_user(self.USERNAME)
        userobj.add_authorizedkey(key)
        userobj.commit()

        # When adding the same key with a different comment
        # Then an error is raised
        with self.assertRaises(DuplicateSSHKeyError):
            userobj.add_authorizedkey(key, comment="new comment")
            userobj.commit()

    def test_add_authorizedkey_duplicate_new_user(self):
        # Read the pub key
        key = self._read_ssh_key()
        # Given a user with a SSH Key
        userobj = UserObject.get_user(self.USERNAME)
        userobj.add_authorizedkey(key)
        userobj.commit()

        # When adding the same key to a different user
        # Then an error is raised
        newuser = UserObject.add_user("newuser")
        newuser.commit()
        with self.assertRaises(DuplicateSSHKeyError):
            newuser.add_authorizedkey(key, comment="new comment")
            newuser.commit()