Issue 14984: netrc module allows read of non-secured .netrc file (original) (raw)
Created on 2012-06-02 12:53 by bruno.Piguet, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (32)
Author: bruno Piguet (bruno.Piguet) *
Date: 2012-06-02 12:53
Most FTP clients require that the .netrc file be owned by the user and readable/writable by nobody other than the user (ie. permissions set to 0400 or 0600). The netrc module doesn't do this kind of checking, allowing the use a .netrc file written or modified by somebody else.
Author: R. David Murray (r.david.murray) * 
Date: 2012-06-02 19:02
This seems like something we should fix for the default file read. There is a backward compatibility concern, but I think the security aspect overrides that.
Author: bruno Piguet (bruno.Piguet) *
Date: 2012-06-09 09:55
Do you agree that the attached patch could be a practical solution ? The patch is for the 2.6 version of the lib. Transposition to other versions should be trivial.
If we don't want to break backward compatibility, the solution is to add a optional behavior flag argument, something like def init(self, file=None, sec_mode=False): and have the default value be False for old versions and True for 3.3 and further.
Author: R. David Murray (r.david.murray) *
Date: 2012-06-09 13:58
Thanks for the patch.
I think the extra check should be done unconditionally in the case where we've looked up the default .netrc file. Adding a feature to 3.3 to provide an optional check for other files (with default False) would then be an additional enhancement, and I think a good one. That should be a separate patch, probably even a separate issue.
I don't think the parse error is the right thing to raise, but I'm not sure what is. An OSError, perhaps?
I'm adding the 2.6 release manager to see if he thinks this is of sufficient importance to go into a 2.6 release.
Author: bruno Piguet (bruno.Piguet) *
Date: 2013-09-08 21:02
I missed the 3.3 window, may I re-propose the same minimal patch against 3.4.0a1 ? I'm not sure I follow any python standard lib coding style but the general idea is quite simple and easy to get.
I chose to ignore the backward compatibility concern, since I agree that the security aspect overrides that.
I followed your suggestion : my patch raises OSError instead of NetrcParseError.
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 18:05
For the security fix, the check should only be done if the file is the the default .netrc. (Which would also make your error message correct...otherwise it is not :) Also, it would make more sense for the 'prop =' to be inside the 'if posix'.
Barry, with that detail fixed should I apply this to 2.6? (I'll tweak the error messages a bit, too.)
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 18:08
Note that I'll test it by hand before applying, and will write a test for 3.3 (where Mock is available to make testing practical).
Author: Barry A. Warsaw (barry) *
Date: 2013-09-15 18:44
On Sep 15, 2013, at 06:05 PM, R. David Murray wrote:
For the security fix, the check should only be done if the file is the the default .netrc. (Which would also make your error message correct...otherwise it is not :) Also, it would make more sense for the 'prop =' to be inside the 'if posix'.
Barry, with that detail fixed should I apply this to 2.6? (I'll tweak the error messages a bit, too.)
For the error message, I suggest including both os.getuid and prop.st_uid, e.g. something like:
".netrc file is owned by (%d); should be (%d)" % (prop.st_uid, os.getuid())
NetrcParseError seems a little odd but I suppose I could justify incorrect ownership or mode as a parse error. We definitely don't want to introduce a new exception for 2.6.9, so the only other option is an OSError I think.
RDM, can you write any tests for this issue? Also, are any documentation changes necessary? I think this should be a candidate for 2.6.9.
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 18:49
Here is a 2.6 specific patch. I've hand tested this.
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 18:51
I could write a 2.6 test for the permissions part, but not for the incorrect owner part. Do you want one without the other?
Author: Barry A. Warsaw (barry) *
Date: 2013-09-15 18:54
On Sep 15, 2013, at 06:51 PM, R. David Murray wrote:
I could write a 2.6 test for the permissions part, but not for the incorrect owner part. Do you want one without the other?
Yeah, I guess you can't mock os or stat in 2.6. ;)
Let's test the permission part, and if you can manually test the owner part, that'll have to be good enough. The former will at least test the changes to NetrcParseError.str().
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 19:09
Hmm. Answering the doc question caused me to run into something that calls the whole patch into question:
http://www.unix.com/unix-dummies-questions-answers/11326-netrc-refuses-password.html.
In that example, the ftp program only rejected reading the password from the .netrc file when the permissions were wrong, but otherwise happily read it. That would be a better backward compatibility fix. And yes, in that case I think we should probably put a note about it in the docs.
I'll update my patch and add the permissions test. I originally used OSError, but with the trigger on password only I think the parse error would actually be more appropriate, so I'll switch to that.
Author: Barry A. Warsaw (barry) *
Date: 2013-09-15 19:43
FWIW, the Ubuntu manpage netrc(5) says:
password string
Supply a password. If this token is present, the auto-login
process will supply the specified string if the remote server
requires a password as part of the login process. Note that
if this token is present in the .netrc file for any user other
than anonymous, ftp will abort the auto-login process if the
.netrc is readable by anyone besides the user.On Ubuntu, /usr/bin/ftp comes from the netkit-ftp package, which has this code in ruserpass.c:
case PASSWD:
if (*aname==NULL) {
fprintf(stderr, "Error: `password' must follow `login' in .netrc\n");
goto bad;
}
if (strcmp(*aname, "anonymous") &&
fstat(fileno(cfile), &stb) >= 0 &&
(stb.st_mode & 077) != 0) {
fprintf(stderr, "Error - .netrc file not correct permissions.\n");
fprintf(stderr, "Remove password or correct mode (should be 600).\n");
goto bad;So it looks like it's only doing a permission check too, and then only if it sees password. (FWIW, it does the same check, sans the "anonymous" check obviously, for account.)
Seems to me like only doing the permission check is sufficient, and in line with existing tools and documentation. (Though technically, I suppose if you chowned ~/.netrc to someone other than yourself, it would be "readable by anyone besides the user".)
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 20:09
Here is an updated patch, with docs and test.
Turns out it actually wasn't necessary to move the check to the password, but I'm leaving it that way anyway. The reason it wasn't necessary is that we don't actually parse the .netrc file correctly: we require that the password field be present. So I'll want to fix that bug in some version of python, in which case the test should then be in the password check where I have it now...
Author: Barry A. Warsaw (barry) *
Date: 2013-09-15 20:15
@RDM: In netrc.rst, s/posix/POSIX/
It also looks like you're keeping the ownership test. Did I misunderstand ? I thought you were only going to keep the permission test?
Author: R. David Murray (r.david.murray) *
Date: 2013-09-15 20:39
Yes, you did :) I was using "permissions check" to cover both tests, since as you say, if the file is owned by someone other than the user running the processes, a user other than the one running the process has permission to modify it.
posix->POSIX fixed in my local copy. Should I commit?
Author: Barry A. Warsaw (barry) *
Date: 2013-09-16 12:47
@RDM: Please commit to 2.6 and null merge to 2.7. Thanks!
Author: R. David Murray (r.david.murray) *
Date: 2013-09-16 15:12
Well, I was planning to merge it, since 2.7 needs the fix as well.
Author: Barry A. Warsaw (barry) *
Date: 2013-09-16 15:18
On Sep 16, 2013, at 03:12 PM, R. David Murray wrote:
Well, I was planning to merge it, since 2.7 needs the fix as well.
Oh yeah, that's fine of course. And thanks!
Author: Roundup Robot (python-dev) 
Date: 2013-09-16 18:35
New changeset e5c4eb6b8e05 by R David Murray in branch '2.6': #14984: On POSIX, enforce permissions when reading default .netrc. http://hg.python.org/cpython/rev/e5c4eb6b8e05
New changeset 2e19c65d6688 by R David Murray in branch '2.7': Merge #14984: On POSIX, enforce permissions when reading default .netrc. http://hg.python.org/cpython/rev/2e19c65d6688
Author: R. David Murray (r.david.murray) *
Date: 2013-09-16 18:36
Removing 2.6 and 2.7 from versions since it is now fixed there. I'll work on porting it to python3.
Author: R. David Murray (r.david.murray) *
Date: 2013-09-16 19:03
The patch for 3.1 is very close to the 2.7 patch, and is attached.
Benjamin and Georg, I'd like to apply this to 3.1 and merge it up through default. May I and can I?
Author: Georg Brandl (georg.brandl) *
Date: 2013-09-17 05:35
I would welcome a "versionchanged" block in the docs addition.
There seems to be a stray space in the string in the last line here:
try:user = pwd.getpwuid(os.getuid())[0]except KeyError:user = 'uid %s ' % os.getuid()
Otherwise, fine for 3.2.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2013-09-17 20:34
Fine for 3.1.
Author: Roundup Robot (python-dev)
Date: 2013-09-18 00:11
New changeset 1b673e0fd8f3 by R David Murray in branch '2.6': Add versionchanged for #14984, remove extra blank from string. http://hg.python.org/cpython/rev/1b673e0fd8f3
New changeset 48be42b94381 by R David Murray in branch '2.7': Merge: Add versionchanged for #14984, remove extra blank from string. http://hg.python.org/cpython/rev/48be42b94381
Author: R. David Murray (r.david.murray) *
Date: 2013-09-18 01:39
Well, I got the answer to the "may" question, but not the "can" question. The answer to that question is "no":
remote: - changeset 6396d1fc72da on disallowed branch '3.1'! remote: * Please strip the offending changeset(s) remote: * and re-do them, if needed, on another branch!
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2013-09-18 01:46
You should be able to push now.
2013/9/17 R. David Murray <report@bugs.python.org>:
R. David Murray added the comment:
Well, I got the answer to the "may" question, but not the "can" question. The answer to that question is "no":
remote: - changeset 6396d1fc72da on disallowed branch '3.1'! remote: * Please strip the offending changeset(s) remote: * and re-do them, if needed, on another branch!
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue14984>
Author: Roundup Robot (python-dev)
Date: 2013-09-18 11:38
New changeset 6396d1fc72da by R David Murray in branch '3.1': #14984: On POSIX, enforce permissions when reading default .netrc. http://hg.python.org/cpython/rev/6396d1fc72da
New changeset 0d9e471221da by R David Murray in branch '3.2': Merge #14984: On POSIX, enforce permissions when reading default .netrc. http://hg.python.org/cpython/rev/0d9e471221da
New changeset b657196b9fc2 by R David Murray in branch '3.3': Merge #14984: On POSIX, enforce permissions when reading default .netrc. http://hg.python.org/cpython/rev/b657196b9fc2
New changeset bfc86caf98ae by R David Murray in branch 'default': Merge #14984: On POSIX, enforce permissions when reading default .netrc. http://hg.python.org/cpython/rev/bfc86caf98ae
Author: R. David Murray (r.david.murray) *
Date: 2013-09-18 11:40
Thanks, Benjamin. And Thank you, Bruno.
Author: Roundup Robot (python-dev)
Date: 2013-09-18 13:00
New changeset fb3ad8a749c8 by R David Murray in branch '2.6': #14984: only import pwd on POSIX. http://hg.python.org/cpython/rev/fb3ad8a749c8
New changeset 88e62c43e443 by R David Murray in branch '2.7': Merge #14984: only import pwd on POSIX. http://hg.python.org/cpython/rev/88e62c43e443
New changeset 713d71048ab9 by R David Murray in branch '3.1': #14984: only import pwd on POSIX. http://hg.python.org/cpython/rev/713d71048ab9
New changeset ef90c40fe6cf by R David Murray in branch '3.2': Merge #14984: only import pwd on POSIX. http://hg.python.org/cpython/rev/ef90c40fe6cf
New changeset b8206cb2c4ee by R David Murray in branch '3.3': Merge #14984: only import pwd on POSIX. http://hg.python.org/cpython/rev/b8206cb2c4ee
New changeset ad9a5ded5cf6 by R David Murray in branch 'default': Merge #14984: only import pwd on POSIX. http://hg.python.org/cpython/rev/ad9a5ded5cf6
Author: bruno Piguet (bruno.Piguet) *
Date: 2013-10-09 21:11
I apologise for coming back to this issue lately, after its closing. I must have misconfigured something in my tracking system.
Thank-you everybody for the work done, especiallly the careful handling and documenting of the case "only if password is present in file". I recognise my proposed patch was a bit flacky. However, I don't get the rationale behind the restriction to the sole case where the file is the default .netrc ? If a clear text password is exposed in any file, it is also a security problem, isn't it ? This specific file might be more difficult to find for an attacker, but not impossible. Feel free to redirect this discussion to some other place if you want to keep this issue close and still.
Author: R. David Murray (r.david.murray) *
Date: 2013-10-09 21:55
Nothing stops us from have a post-mortem discussion on a closed issue :)
The rationale for only doing the check for .netrc is that that is backward-compatibility-wise fairly safe, because other tools will already be insisting on the same security. But for arbitrary files being parsed for arbitrary purposes by python-based tools, suddenly throwing an error if there is a password in the file could easily break things.
This doesn't necessarily prevent us from making the security even more strict in 3.4, but that is a more complex discussion (involving what purposes netrc-on-other-than-.netrc is used for in the real world), and should be a separate issue in this tracker, if you want to raise the proposal.
History
Date
User
Action
Args
2022-04-11 14:57:31
admin
set
github: 59189
2013-10-09 21:55:33
r.david.murray
set
messages: +
2013-10-09 21:11:33
bruno.Piguet
set
messages: +
2013-09-18 13:00:17
python-dev
set
messages: +
2013-09-18 11:40:47
r.david.murray
set
status: open -> closed
resolution: fixed
messages: +
stage: resolved
2013-09-18 11:38:28
python-dev
set
messages: +
2013-09-18 01:46:36
benjamin.peterson
set
messages: +
2013-09-18 01:39:08
r.david.murray
set
messages: +
2013-09-18 00:11:45
python-dev
set
messages: +
2013-09-17 20:34:51
benjamin.peterson
set
messages: +
2013-09-17 05:35:52
georg.brandl
set
messages: +
2013-09-16 19:03:55
r.david.murray
set
files: + netrc-py3.1.patch
messages: +
2013-09-16 18:36:41
r.david.murray
set
messages: +
versions: - Python 2.6, Python 2.7
2013-09-16 18:35:03
python-dev
set
nosy: + python-dev
messages: +
2013-09-16 15🔞44
barry
set
messages: +
2013-09-16 15:12:58
r.david.murray
set
messages: +
2013-09-16 12:47:11
barry
set
messages: +
2013-09-15 20:39:14
r.david.murray
set
messages: +
2013-09-15 20:15:57
r.david.murray
set
files: + netrc-2.6.patch
2013-09-15 20:15:41
r.david.murray
set
files: - netrc-2.6.patch
2013-09-15 20:15:27
barry
set
messages: +
2013-09-15 20:09:25
r.david.murray
set
files: + netrc-2.6.patch
messages: +
2013-09-15 19:43:21
barry
set
messages: +
2013-09-15 19:30:29
Arfrever
set
nosy: + Arfrever
2013-09-15 19:30:23
Arfrever
set
versions: + Python 3.1
2013-09-15 19:09:42
r.david.murray
set
messages: +
2013-09-15 18:54:05
barry
set
messages: +
2013-09-15 18:51:06
r.david.murray
set
messages: +
2013-09-15 18:49:38
r.david.murray
set
files: + netrc-2.6.patch
keywords: + patch
messages: +
2013-09-15 18:44:22
barry
set
priority: high -> release blocker
nosy: + larry, benjamin.peterson, georg.brandl
2013-09-15 18:44:00
barry
set
messages: +
2013-09-15 18:08:40
r.david.murray
set
messages: +
2013-09-15 18:05:03
r.david.murray
set
messages: +
2013-09-09 06:27:37
pitrou
set
nosy: + giampaolo.rodola
2013-09-09 06:24:59
bruno.Piguet
set
versions: + Python 3.2
2013-09-08 21:02:24
bruno.Piguet
set
files: + patch_netrc_3.4.0a1.txt
messages: +
versions: + Python 3.4, - Python 3.2
2012-06-09 13:58:40
r.david.murray
set
nosy: + barry
messages: +
2012-06-09 09:55:20
bruno.Piguet
set
files: + patch_netrc_2.6.txt
messages: +
2012-06-02 19:02:09
r.david.murray
set
priority: normal -> high
type: security
components: + Library (Lib)
versions: + Python 2.7, Python 3.2, Python 3.3
nosy: + r.david.murray
messages: +
2012-06-02 12:54:19
bruno.Piguet
set
title: netrc module alows read of non-secured .netrc file -> netrc module allows read of non-secured .netrc file
2012-06-02 12:53:41
bruno.Piguet
create