Hello,

Currently the `svn patch` command changes the permissions on any file
it patches to 600. This occurs because it creates a file under the
system temporary directory for applying the patch, and then copies
this file to the final destination. `apr_file_mktemp` sensibly assigns
mode 600 to files created under the system temporary directory to
avoid them being exposed to the entire system. So the result is that
any file that `svn patch` patches ends up with 600 permissions rather
than whatever it had before.

Here is an example of the current behaviour (before my patch):

[Cathy@localhost subversion]$ ls -la INSTALL
-rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:34 INSTALL
[Cathy@localhost subversion]$ ./svn patch patch.diff
U         INSTALL
[Cathy@localhost subversion]$ ls -la INSTALL
-rw-------. 1 Cathy Cathy 63070 Nov 20 17:58 INSTALL

Notice that applying the patch has changed the permissions of INSTALL.

After my patch, we instead get the following:

[Cathy@localhost subversion]$ ls -la INSTALL
-rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:59 INSTALL
[Cathy@localhost subversion]$ ./svn patch patch.diff
U         INSTALL
[Cathy@localhost subversion]$ ls -la INSTALL
-rw-rw-r--. 1 Cathy Cathy 63070 Nov 20 17:59 INSTALL

As expected, patching the file no longer alters its permissions.


I have attached my patch to this email and it includes a log message.

Regards,

Cathy
[[[
svn patch: Don't change permissions of patched files

* subversion/libsvn_client/patch.c
  (init_patch_target): Create temporary file for result of patch
    under the working directory, rather than under the system
    temporary directory. This is already how EOL translation
    works (see `svn_subst_copy_and_translate4`) and has the
    advantage that we can safely change permissions on the file
    without potentially exposing the file to the entire system.
  (install_patched_target): Make sure the final patched file has
    the same permissions as the original file before patching.

Patch by: Cathy J. Fitzpatrick <ca...@cathyjf.com>
]]]
Index: subversion/libsvn_client/patch.c
===================================================================
--- subversion/libsvn_client/patch.c	(revision 1543974)
+++ subversion/libsvn_client/patch.c	(working copy)
@@ -1076,23 +1076,33 @@
        * ### we should have kept the patch field in patch_target_t to be
        * ### able to distinguish between 'what the patch says we should do'
        * ### and 'what we can do with the given state of our WC'. */
       if (patch->operation == svn_diff_op_added)
         target->added = TRUE;
       else if (patch->operation == svn_diff_op_deleted)
         target->deleted = TRUE;
 
       if (! target->is_symlink)
         {
-          /* Open a temporary file to write the patched result to. */
+          /* Open a temporary file to write the patched result to.
+           *
+           * Create the temporary file under the working directory (the same
+           * as EOL translation uses) so that it is safe to change its file
+           * permissions without exposing the file to the entire system, which
+           * could happen if the file were under the system temporary
+           * directory, rather than the working directory.
+           */
           SVN_ERR(svn_io_open_unique_file3(&target->patched_file,
-                                           &target->patched_path, NULL,
+                                           &target->patched_path,
+                                           svn_dirent_dirname(
+                                             wcroot_abspath,
+                                             scratch_pool),
                                            remove_tempfiles ?
                                              svn_io_file_del_on_pool_cleanup :
                                              svn_io_file_del_none,
                                            result_pool, scratch_pool));
 
           /* Put the write callback in place. */
           content->write = write_file;
           content->write_baton = target->patched_file;
         }
       else
@@ -2564,20 +2574,27 @@
               svn_boolean_t repair_eol;
 
               /* Copy the patched file on top of the target file.
                * Always expand keywords in the patched file, but repair EOL
                * only if svn:eol-style dictates a particular style. */
               repair_eol = (target->content->eol_style ==
                               svn_subst_eol_style_fixed ||
                             target->content->eol_style ==
                               svn_subst_eol_style_native);
 
+              /* Make sure the patched file has the same permissions the
+               * original file. By design, `patched_path` will be under
+               * the working directory, so this does introduce any
+               * security issues with temp files. */
+              SVN_ERR(svn_io_copy_perms(
+                        target->local_abspath, target->patched_path, pool));
+
               SVN_ERR(svn_subst_copy_and_translate4(
                         target->patched_path, target->local_abspath,
                         target->content->eol_str, repair_eol,
                         target->content->keywords,
                         TRUE /* expand */, FALSE /* special */,
                         ctx->cancel_func, ctx->cancel_baton, pool));
             }
 
           if (target->added || target->replaced)
             {

Reply via email to