Derby
  1. Derby
  2. DERBY-5492

Restrictive file permissions: permissions removed also for owner on NTFS if Acl does not contain explicit entry for owner

    Details

    • Bug behavior facts:
      Security

      Description

      It turns out that the file owner does not necessarily get an explicit AclEntry; this depends on whether the created file has sufficient permissions already through, say, a permission for everybody to write. The present logic removes all AclEntries except those granted to the file's owner, erroneously presuming there would be such an entry always. This led to all AclEntries being removed.

      This error is seen in Oracle's nightly regressions for Windows, but did not reproduced when running manually on Windows. This was due to different default inherited permissions on the directories in which the regression tests were run.

      1. derby-5492-1.diff
        9 kB
        Dag H. Wanvik
      2. derby-5492-1.stat
        0.2 kB
        Dag H. Wanvik
      3. derby-5492-2.diff
        9 kB
        Dag H. Wanvik
      4. derby-5492-2.stat
        0.2 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a patch which makes two changes:

          • Construct a new AclEntry for the owner with all rights, and removed existing ones (NTFS). This should handle the error seen in Oracle's regressions.
          • For Solaris/ZFS and similar file systems which support both Posix file attributes view and ACLs, don't touch the ACLs but stick to the Posix flags.

          For the latter my rationale is as follows: Principle of least surprise: most users never touch the ACLs but use the more familiar Posix file masks. It turned out the existing Derby implementation, although protecting the file adequately, showed a "+" in the ls(1) listing indicating that the settings could not be directly mapped onto the Posix model. The reason was that we removed more permissions than the plain read,write, and execute. Since ZFS internally builds on ACLs, the ls(1) listing would should that Derby had been tinkering with the non-mappable ACL permissions. I think it is better to stick to the Posix permissions by default. If people are using ACL functionality they are likely more than average concerned with security and can run with default file permissions and take full responsibility of the permissions fo created filed. Rationale end.

          Rerunning regressions of NTFS, Solaris/ZFS and Linux.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a patch which makes two changes: Construct a new AclEntry for the owner with all rights, and removed existing ones (NTFS). This should handle the error seen in Oracle's regressions. For Solaris/ZFS and similar file systems which support both Posix file attributes view and ACLs, don't touch the ACLs but stick to the Posix flags. For the latter my rationale is as follows: Principle of least surprise: most users never touch the ACLs but use the more familiar Posix file masks. It turned out the existing Derby implementation, although protecting the file adequately, showed a "+" in the ls(1) listing indicating that the settings could not be directly mapped onto the Posix model. The reason was that we removed more permissions than the plain read,write, and execute. Since ZFS internally builds on ACLs, the ls(1) listing would should that Derby had been tinkering with the non-mappable ACL permissions. I think it is better to stick to the Posix permissions by default. If people are using ACL functionality they are likely more than average concerned with security and can run with default file permissions and take full responsibility of the permissions fo created filed. Rationale end. Rerunning regressions of NTFS, Solaris/ZFS and Linux.
          Hide
          Knut Anders Hatlen added a comment -

          I ran a quick test on the Windows machine where we saw the failures in the nightly tests. I ran the derbynet test suite and verified that it failed with permission problems without the patch and passed with the patch.

          Tiny nit: The patch adds an import for the Monitor class, but it doesn't use it. The rest of the changes look good to me.

          Show
          Knut Anders Hatlen added a comment - I ran a quick test on the Windows machine where we saw the failures in the nightly tests. I ran the derbynet test suite and verified that it failed with permission problems without the patch and passed with the patch. Tiny nit: The patch adds an import for the Monitor class, but it doesn't use it. The rest of the changes look good to me.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at the patch, Knut. Uploading derby-5492-2 which removes two unused imports. Committed at svn 1199673 and closing.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at the patch, Knut. Uploading derby-5492-2 which removes two unused imports. Committed at svn 1199673 and closing.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development