Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-15787

clients built without encryption support can corrupt encrypted directories

Details

    • Bug
    • Resolution: Fixed
    • Blocker
    • Lustre 2.15.0
    • None
    • None
    • 3
    • 9223372036854775807

    Description

      Clients build without encryption support can create files in encrypted directories. The names of the created files may collide with the names of existing decrypted files.

      One way to see this is to build a CentOS 7 (3.10.0) based client and then to mount an encryption enabled FS.

      On the old client:

      # grep ENCRYPT ex/lustre-release/config.h
      /* #undef CONFIG_LDISKFS_FS_ENCRYPTION */
      /* #undef CONFIG_LL_ENCRYPTION */
      
      new-server# fscrypt setup /mnt/lustre
      Metadata directories created at "/mnt/lustre/.fscrypt".
      new-server# mkdir /mnt/lustre/private
      new-server# fscrypt encrypt --user=root /mnt/lustre/private
      ...
      "/mnt/lustre/private" is now encrypted, unlocked, and ready for use.
      new-server# echo XXX > /mnt/lustre/private/f0
      new-server# ls /mnt/lustre/private/
      f0
      
      old-client# mount new-server@tcp:/lustre /mnt/lustre -t lustre
      old-client# mount -t lustre
      192.168.122.63@tcp:/lustre on /mnt/lustre type lustre (rw,flock,lazystatfs,noencrypt)
      old-client# ls /mnt/lustre/private/
      j?"n5?<=??7?Jj?"=@????x??]|??lqdX?
      old-client# echo YYY > /mnt/lustre/private/f0
      old-client# ls /mnt/lustre/private/
      f0  j?"n5?<=??7?Jj?"=@????x??]|??lqdX?
      
      new-server# ls /mnt/lustre/private/
      ls: reading directory '/mnt/lustre/private/': Structure needs cleaning
      f0
      new-server# rm -rf /mnt/lustre/private
      rm: traversal failed: /mnt/lustre/private: Structure needs cleaning
      new-server# ls /mnt/lustre/private
      ls: reading directory '/mnt/lustre/private': Structure needs cleaning
      new-server# rmdir /mnt/lustre/private
      rmdir: failed to remove '/mnt/lustre/private': Directory not empty
      

      Another thing I notice here is that when the old client tries to read the encrypted file, read() just returns 0.

      This seems like a serious issue to me. If a site is running a mix of old and new client kernels then it seems relatively easy to wander into this situation unawares.

      As discussed on chat, the MDT should protect against this situation. If the directory is encrypted (has LUSTRE_ENCRYPT_FL set) and the client does not have the encryption connect flag (OBD_CONNECT2_ENCRYPT) then the MDT could disallow creates in the directory. But this needs investigation.

      Attachments

        Issue Links

          Activity

            [LU-15787] clients built without encryption support can corrupt encrypted directories
            pjones Peter Jones added a comment -

            Landed for 2.15

            pjones Peter Jones added a comment - Landed for 2.15

            "Oleg Drokin <green@whamcloud.com>" merged in patch https://review.whamcloud.com/47182/
            Subject: LU-15787 sec: document enc-unaware clients on enc files
            Project: fs/lustre-release
            Branch: master
            Current Patch Set:
            Commit: 751a8114ef3afe9abe7692b3974b070db6a705a2

            gerrit Gerrit Updater added a comment - "Oleg Drokin <green@whamcloud.com>" merged in patch https://review.whamcloud.com/47182/ Subject: LU-15787 sec: document enc-unaware clients on enc files Project: fs/lustre-release Branch: master Current Patch Set: Commit: 751a8114ef3afe9abe7692b3974b070db6a705a2

            "Oleg Drokin <green@whamcloud.com>" merged in patch https://review.whamcloud.com/47156/
            Subject: LU-15787 sec: block enc unaware clients on enc files
            Project: fs/lustre-release
            Branch: master
            Current Patch Set:
            Commit: a31db2ec062ccc995527d37f0330edbca9d486a9

            gerrit Gerrit Updater added a comment - "Oleg Drokin <green@whamcloud.com>" merged in patch https://review.whamcloud.com/47156/ Subject: LU-15787 sec: block enc unaware clients on enc files Project: fs/lustre-release Branch: master Current Patch Set: Commit: a31db2ec062ccc995527d37f0330edbca9d486a9

            "Sebastien Buisson <sbuisson@ddn.com>" uploaded a new patch: https://review.whamcloud.com/47182
            Subject: LU-15787 sec: document enc-unaware clients on enc files
            Project: fs/lustre-release
            Branch: master
            Current Patch Set: 1
            Commit: cabd3955c47543d2193f237918a8204d5532d1f6

            gerrit Gerrit Updater added a comment - "Sebastien Buisson <sbuisson@ddn.com>" uploaded a new patch: https://review.whamcloud.com/47182 Subject: LU-15787 sec: document enc-unaware clients on enc files Project: fs/lustre-release Branch: master Current Patch Set: 1 Commit: cabd3955c47543d2193f237918a8204d5532d1f6

            Colin, definitely without Sebastien's 47156 patch there is a real risk of a user/application without a key or fscrypt inadvertently or maliciously corrupting an encrypted directory. That is a DOS on the legitimate user of the file. Even worse, the corruption is in a way that even the user with the key can't fix the problem anymore == service call where we need to do low-level debugfs hacking to delete the bad files.

            As for why we would want to add MDS support for base64 encoding filenames for no-key/no-fscrypt clients, that is mainly for manageability and consistency. It is bad to return "nasty" filenames to old clients that they can't do anything with (or may choke on), acceptable would be to block them outright from accessing these files/directories on the assumption that they must have some newer client with fscrypt support that could at least access the directory and delete the files, even if they don't have the key.

            Preferably, the no-key filenames should be consistent for all clients (no-key or without fscrypt support) so that tools that monitor space usage, migrate, etc. can still handle them. I'm sure users would love a mechanism to lock out admins from their directory trees to prevent purge from cleaning up their old files. This needs to work for basic directory operations (e.g. "ls -l", "rm -r", etc.) and potentially also Lustre-specific interfaces like "lfs fid2path", "lfs path2fid". This will need some support on the MDS side to encode the filenames in a consistent way with no-key fscrypt-capable clients.

            Separately, it is currently not possible to do backup/restore for fscrypt files (neither at the Lustre level nor the MDT with tar, but dd or e2image would still work). This is a limitation present in the upstream fscrypt implementation that we've partly worked around for Lustre to allow mirror/migrate for OST management, but it is something that needs to be addressed in the future in order for admins to manage filesystems that have fscrypted directory trees in them.

            adilger Andreas Dilger added a comment - Colin, definitely without Sebastien's 47156 patch there is a real risk of a user/application without a key or fscrypt inadvertently or maliciously corrupting an encrypted directory. That is a DOS on the legitimate user of the file. Even worse, the corruption is in a way that even the user with the key can't fix the problem anymore == service call where we need to do low-level debugfs hacking to delete the bad files. As for why we would want to add MDS support for base64 encoding filenames for no-key/no-fscrypt clients, that is mainly for manageability and consistency. It is bad to return "nasty" filenames to old clients that they can't do anything with (or may choke on), acceptable would be to block them outright from accessing these files/directories on the assumption that they must have some newer client with fscrypt support that could at least access the directory and delete the files, even if they don't have the key. Preferably, the no-key filenames should be consistent for all clients (no-key or without fscrypt support) so that tools that monitor space usage, migrate, etc. can still handle them. I'm sure users would love a mechanism to lock out admins from their directory trees to prevent purge from cleaning up their old files. This needs to work for basic directory operations (e.g. " ls -l ", " rm -r ", etc.) and potentially also Lustre-specific interfaces like " lfs fid2path ", " lfs path2fid ". This will need some support on the MDS side to encode the filenames in a consistent way with no-key fscrypt-capable clients. Separately, it is currently not possible to do backup/restore for fscrypt files (neither at the Lustre level nor the MDT with tar , but dd or e2image would still work). This is a limitation present in the upstream fscrypt implementation that we've partly worked around for Lustre to allow mirror/migrate for OST management, but it is something that needs to be addressed in the future in order for admins to manage filesystems that have fscrypted directory trees in them.
            cfaber Colin Faber added a comment - - edited

            Hm.. why though? If the filenames are encrypted, and the client lacks both key and encryption support, what's the problem?

            The original issue seems to be addressed by sebastien's patch, wouldn't extending this further be feature enhancements so that older non-supported clients have a "more correct" view of a bunch of files that they can't access anyways?

            cfaber Colin Faber added a comment - - edited Hm.. why though? If the filenames are encrypted, and the client lacks both key and encryption support, what's the problem? The original issue seems to be addressed by sebastien 's patch, wouldn't extending this further be feature enhancements so that older non-supported clients have a "more correct" view of a bunch of files that they can't access anyways?

            That would require to implement on server side, for the specific encryption unaware client case, a filename processing similar to what is done in ll_setup_filename() and ll_fname_disk_to_usr on client side, in order to handle the digested form. This digested form is imposed by the NAME_MAX limitation (i.e. base64 encoding NAME_MAX long encrypted file names would produce too long names).

            sebastien Sebastien Buisson added a comment - That would require to implement on server side, for the specific encryption unaware client case, a filename processing similar to what is done in ll_setup_filename() and ll_fname_disk_to_usr on client side, in order to handle the digested form. This digested form is imposed by the NAME_MAX limitation (i.e. base64 encoding NAME_MAX long encrypted file names would produce too long names).

            Hmm, you want the MDS to base64 encode the encrypted filenames before returning them to the client, in all cases, or just to encryption unaware clients?

            Ideally the same no-key filenames would be visible userspace for clients w/o encryption support, clients w/o the key, as well as fid2path. That allows userspace to work consistently with these filenames.

            adilger Andreas Dilger added a comment - Hmm, you want the MDS to base64 encode the encrypted filenames before returning them to the client, in all cases, or just to encryption unaware clients? Ideally the same no-key filenames would be visible userspace for clients w/o encryption support, clients w/o the key, as well as fid2path. That allows userspace to work consistently with these filenames.

            "Sebastien Buisson <sbuisson@ddn.com>" uploaded a new patch: https://review.whamcloud.com/47156
            Subject: LU-15787 sec: block enc unaware clients on enc files
            Project: fs/lustre-release
            Branch: master
            Current Patch Set: 1
            Commit: 2c5d4a99365cea42e6f98be7cafc089fd95283c6

            gerrit Gerrit Updater added a comment - "Sebastien Buisson <sbuisson@ddn.com>" uploaded a new patch: https://review.whamcloud.com/47156 Subject: LU-15787 sec: block enc unaware clients on enc files Project: fs/lustre-release Branch: master Current Patch Set: 1 Commit: 2c5d4a99365cea42e6f98be7cafc089fd95283c6

            Hmm, you want the MDS to base64 encode the encrypted filenames before returning them to the client, in all cases, or just to encryption unaware clients?

            For encryption aware clients, this is not necessary, the client code properly handles this.
            For encryption unaware clients, I am not sure it is a good idea. Keeping non printable characters at least has the advantage to show that these files should not be manipulated.
            And in all cases, the MDS already escapes a few critical characters (NULL, LF, CR, /, DEL and =) in encrypted file names before sending them to clients, by calling critical_encode(). So the encrypted names received by an encryption unaware client are not that badly formed to break ls for instance.

            sebastien Sebastien Buisson added a comment - Hmm, you want the MDS to base64 encode the encrypted filenames before returning them to the client, in all cases, or just to encryption unaware clients? For encryption aware clients, this is not necessary, the client code properly handles this. For encryption unaware clients, I am not sure it is a good idea. Keeping non printable characters at least has the advantage to show that these files should not be manipulated. And in all cases, the MDS already escapes a few critical characters (NULL, LF, CR, /, DEL and =) in encrypted file names before sending them to clients, by calling critical_encode(). So the encrypted names received by an encryption unaware client are not that badly formed to break ls for instance.

            People

              sebastien Sebastien Buisson
              jhammond John Hammond
              Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: