This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: strips execute permission on Unix
Type: Stage:
Components: Demos and Tools Versions: Python 2.4
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: facundobatista Nosy List: dripton, facundobatista, jlgijsbers, sjoerd, tim.peters
Priority: normal Keywords:

Created on 2004-10-20 15:48 by dripton, last changed 2022-04-11 14:56 by admin. This issue is now closed.

File name Uploaded Description Edit
diff_reindent.txt facundobatista, 2004-10-26 02:57 diff file
Messages (17)
msg22789 - (view) Author: David Ripton (dripton) Date: 2004-10-20 15:48
Tools/scripts/ strips execute permission
from its target file.

If the tool detects that changes need to be made, then
the target file is renamed to its original name +
'.bak', then a new file is created with the original
file's name and its whitespace-modified contents.  No
manipulation of the new file's permissions is done, so
it ends up with the default for the user's umask.

This makes annoying to run in an automated
fashion, if you rely on execute permission.

The fix is trivial, except for portability.  Here's the
Posix-only version:

+            if == "posix":
+                mode = os.stat(bak).st_mode
+                os.chmod(file, mode)

Patch available upon request.
msg22790 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-10-24 21:20
Logged In: YES 

Instead of renaming the file and creating a new one, just
copied the original and that's all (the target file is open
with write mode, the content replaced, but the permissions

A patch is attached.

Tim, I'm assigning this to you to doublecheck (not found
unit tests for this script).

.    Facundo
msg22791 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-10-26 02:57
Logged In: YES 

Attaching the file.
msg22792 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2004-10-26 03:19
Logged In: YES 

Sorry, I don't normally run on Unix, so I have no way to tell 
whether this fixes whatever the complaint is.  Offhand it 
doesn't seem right to create a .bak file with default 
permissions either.  For example, if only the user had 
permission to get at the original, letting the .bak file have 
broader permissions seems wrong too.
msg22793 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-10-27 00:44
Logged In: YES 

This fixes the problem: the original file metadata
(permisions, etc) stays unchanged.

The issue with the metadata of the bak file is none: the new
file has the user defaults permissions, group and owner. In
the previous version, the original file left with those
defaults permissions, so security is not getting worse.

The only left issue is, if the user wants to recover the
data from the bak file, he would need to change its metadata
(this is minor, if something goes wrong and the bak is
needed, it's likely that the user will need a manual approach).

My primary concern is that I don't want to commit the
changes now that we're beta without testing the changes in
other platforms (however, I'll ask this on python-dev).
msg22794 - (view) Author: Johannes Gijsbers (jlgijsbers) * (Python triager) Date: 2004-10-27 12:38
Logged In: YES 

The patch works for me (Linux 2.6.8-1-686 i686 GNU/Linux)
and a coworker (Mac OS X 10.3).
msg22795 - (view) Author: David Ripton (dripton) Date: 2004-10-27 14:45
Logged In: YES 

The patch is an improvement (better to have the wrong
permissions on the target file than on the backup file), but
I think it would be even better if the .bak file had the
same permissions as the target file.

How about using shutil.copy2 (which preserves permissions,
among other things) instead of shutil.copy?

msg22796 - (view) Author: David Ripton (dripton) Date: 2004-10-27 14:47
Logged In: YES 

s/wrong/right/ in my last comment.  The target is more
important than the backup, but we should go for the win/win
and give both of them the correct permissions, since it's easy.
msg22797 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-10-27 14:56
Logged In: YES 

I thought about it. The problem with this approach is that
it also copies the permissions, but not the owner/group ids
(you get the defaults there).

So, I think is better to leave the user defaults than mix
the old permissions with new owner/group (which may lead to
a security risk in some combinations).

There's a better approach: to copy *all* the file metadata.
But don't know how to do it in a portable way...
msg22798 - (view) Author: Sjoerd Mullender (sjoerd) * (Python committer) Date: 2004-10-27 15:29
Logged In: YES 

Then perhaps it should do in-place replacement, so first
*copy* the original to the backup, and then overwrite the
msg22799 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-10-27 16:07
Logged In: YES 

That is *exactly* what the patch does, but David complained
about the metadata of the copy.
msg22800 - (view) Author: Sjoerd Mullender (sjoerd) * (Python committer) Date: 2004-10-27 17:34
Logged In: YES 

Now that I looked at the patch, I have one concern.

I don't mind if the backup copy doesn't have the same
permissions as the original, especially not if you can't
guarantee that the owner/group doesn't change.  However, I
do mind if the backup copy has *more* permissions than the
original.  Consider a file that was only readable by the
owner (probably for some good reason), and because of a
permissive umask, the backup copy all of a sudden is
msg22801 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-10-29 00:49
Logged In: YES 

I could touch the permissions of the copy to be only
readable, but with os.chmod() it'll be portable only to
Windows and Unix (according to the docs). And in Mac?

Take note that in this very moment, the copy is created via
the open(..., "w") which leaves the copy with the same
permissions that with the patch. We're not creating a
problem... maybe just don't solving it!
msg22802 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2004-11-05 01:36
Logged In: YES 

I'll just let this bug live, don't want to commit it for 2.4
without bigger consensus.
msg22803 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2004-11-05 02:29
Logged In: YES 

That's a good plan, and for another reason too:  this 
shouldn't be confined to  There *should* be a 
standard module/package in the library to handle:

1. Searching a directory subtree for files with names 
matching a set of patterns, dealing intelligently with symlinks 
and whatever other OS-specific crud gets in the way.

2. Feeding the files that match to a filter.

3. If the filter decides to replace the file, dealing with making 
a backup "correctly", whatever that means across OSes.

None of that code should be *in* to begin with.  
Filter programs keep reinventing this crud because there's 
nothing standard to build on.  There are many filter programs 
under the Tools directory.  For example, is 
going to become popular soon, as people lose interest in 
ancient Pythons and want to get rid of their old __future__ 
msg22804 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2007-03-25 22:10
Lot of time passed, I want to finish this. I propose to make the following:

- Copy the file to .bak, change the file in place: this preserves the file metadata, actually fixing the bug. It does not solve the security problem, but it does not make things worse (actually, this moves the security issue from the original file to the .bak one).

- Add an option -n (--nobackup), to the program not to make the .bak copy, and explicity saying in the docs that this option is useful if you have security concerns.

- Not move the functionality to another library: it will be something nice to have, but these security issues will need more thoughts. Anyway, this goes far beyond this bug.

What do you think?
msg62494 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2008-02-17 16:24
Fixed in r60877, as of the description in my last comment.
Date User Action Args
2022-04-11 14:56:07adminsetgithub: 41053
2008-02-17 16:24:20facundobatistasetstatus: open -> closed
resolution: fixed
messages: + msg62494
2004-10-20 15:48:19driptoncreate