Ticket #2278 (reopened defect)

Opened 8 years ago

Last modified 4 years ago

Problem in the Copy operation

Reported by: eshkrig Owned by: slavazanko
Priority: major Milestone: Future Releases
Component: mc-core Version: 4.7.4
Keywords: Cc: gotar@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: commited-master commited-stable

Description (last modified by andrew_b) (diff)

Hi.
Sorry for my English.
Previously, copying a file does not change access permissions of the destination file if the check box "Preserve attributes" in the Copy window is not set.
For some time it is not so: if you uncheck the "Preserve attributes" then access permissions of the destination file are set in accordance with the value of umask, which can lead to information disclosure(i.e. security problem).

Attachments

mc-copy_chmod.diff (653 bytes) - added by eshkrig 7 years ago.

Change History

comment:1 Changed 8 years ago by eshkrig

  • Version changed from 4.7.3 to 4.7.4

The problem is still present in version 4.7.4

comment:2 Changed 7 years ago by eshkrig

the problem is still there:
app-misc/mc-4.7.5.1

comment:3 Changed 7 years ago by eshkrig

the problem is still there:
app-misc/mc-4.7.5.2

comment:4 in reply to: ↑ description Changed 7 years ago by andrew_b

  • Description modified (diff)
  • Milestone changed from 4.7 to 4.8

Replying to eshkrig:

Previously, copying a file does not change access permissions of the destination file if the check box "Preserve attributes" in the Copy window is not set. For some time it is not so: if you uncheck the "Preserve attributes" then access permissions of the destination file are setted in accordance with the value of umask,

Yes, that done in #72. But how do you think should be a difference whether "Preserve attributes" is checked or not?

comment:5 follow-up: ↓ 6 Changed 7 years ago by eshkrig

IMHO: existing files should only get new content (as earlier versions do)

for example:
-rw------- /path_dst/dir1/secret_file # secret file with old contents
-rw-rw-r-- /path_dst/dir1/some_file

-rw------- /path_src/dir1/secret_file # secret file with new contents

when copying dir
/path_src/dir1 to /path_dst
we have got
-rw-r--r-- /path_dst/dir1/secret_file

That is fail becose structure may contain thousands of hundreds of thousands of catalogs.
And how about ACLs on /path_dst/dir1 ?

As I mention there is security bug for 9 months now (I am sad).

comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 7 years ago by andrew_b

Replying to eshkrig:

IMHO: existing files should only get new content (as earlier versions do)

I don't quite understand, why you check "Preserve attributes" off if you want keep permissions.

comment:7 in reply to: ↑ 6 Changed 7 years ago by eshkrig

Replying to andrew_b:

I don't quite understand, why you check "Preserve attributes" off if you want keep permissions.

Because here changes the file owner and permissions.

For example as root I often have to modify configuration files in users' home directories.
Before, I could use the MC and the copy prepared files/directories to the user's home directory.

Now the situation is as follows:
If I copy the files with the "Preserve attributes" on - owner of the files is changed to root
If I copy the files with the "Preserve attributes" off - file permissions are changed to 644(-rw-r--r--) and this is not dependent on the established permissions to the source or the destination files

Changed 7 years ago by eshkrig

comment:8 Changed 7 years ago by eshkrig

  • Branch state set to no branch

The patch, which fixes the problem

comment:9 Changed 7 years ago by bradatec

My bug was marked as a dupe of this (#2615) although it seems to me its something different.
In short, when copying files as a root, normal files get correct file/group id but it get wrong for soft links (preserve attributes are on).

comment:10 Changed 7 years ago by gotar

  • Cc gotar@… added

comment:11 Changed 6 years ago by slavazanko

  • Owner set to slavazanko
  • Status changed from new to accepted

comment:12 Changed 6 years ago by slavazanko

  • Branch state changed from no branch to on review

Created branch 2278_preserve_attrs_if_dest_exists

Review, please.

Last edited 6 years ago by angel_il (previous) (diff)

comment:13 Changed 6 years ago by slavazanko

  • Milestone changed from 4.8 to 4.8.3

comment:14 Changed 6 years ago by andrew_b

  • Votes for changeset set to andrew_b

comment:15 Changed 6 years ago by angel_il

  • Votes for changeset changed from andrew_b to andrew_b angel_il
  • Branch state changed from on review to approved

comment:16 Changed 6 years ago by slavazanko

  • Keywords stable-candidate added
  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b angel_il to commited-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master:

git log --pretty=oneline 68e1b4f..f0f39cb

comment:17 Changed 6 years ago by slavazanko

Merged to stable:

git log --pretty=oneline 4fd3dee..82dcbe2

comment:18 Changed 6 years ago by slavazanko

  • Keywords stable-candidate removed
  • Status changed from testing to closed
  • Votes for changeset changed from commited-master to commited-master commited-stable

comment:19 Changed 6 years ago by eshkrig

Thank you.

comment:20 Changed 6 years ago by onkeltem

  • Status changed from closed to reopened
  • Resolution fixed deleted

I'm not sure what exactly was fixed but I'm on 4.8.6-1 now and see that copy operation doesn't respect umask set on a remote host when using ssh.

So I connect to a remote via "Shell link...".

There I have umask set via pam_umask to 026.

If I copy a file/directory from local host to the remote and set Preserve Attributes to OFF, then the file/directory is created with 644/755 permissions.

Also I would like to have "Preserve attributes" OFF by default, any ideas how to do this?

UPDATE

After looking in code it becomes clear that MC takes local mask and use it to chmod'ing a remote object:

  else if (!dst_exists)
  {
    src_mode = umask (-1);
    umask (src_mode);
    src_mode = 0100666 & ~src_mode;
    mc_chmod (dst_vpath, (src_mode & ctx->umask_kill));
  }

https://github.com/MidnightCommander/mc/blob/master/src/filemanager/file.c#L1894

This happens with both SFTP and FISH VFSes which I tested.
The correct behavior would be to not chmod at all when we deal with a remote VFS. So instead of:

  else if (!dst_exists)

there should be something like:

  else if (!dst_exists && !<vfs_is_remote>)

Unfortunately I don't know how to check if a vfs is remote. Please fix this.

Last edited 6 years ago by onkeltem (previous) (diff)

comment:21 Changed 4 years ago by andrew_b

  • Milestone changed from 4.8.3 to Future Releases
Note: See TracTickets for help on using tickets.