classification
Title: netrc module allows read of non-secured .netrc file
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3, Python 3.1, Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, barry, benjamin.peterson, bruno.Piguet, georg.brandl, giampaolo.rodola, larry, python-dev, r.david.murray
Priority: release blocker Keywords: patch

Created on 2012-06-02 12:53 by bruno.Piguet, last changed 2013-10-09 21:55 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
patch_netrc_2.6.txt bruno.Piguet, 2012-06-09 09:55 example of what could be a patch against the 2.6 version of netrc.py
patch_netrc_3.4.0a1.txt bruno.Piguet, 2013-09-08 21:02 proposed patch against 3.4.0a1 version of netrc.py
netrc-2.6.patch r.david.murray, 2013-09-15 18:49
netrc-2.6.patch r.david.murray, 2013-09-15 20:15
netrc-py3.1.patch r.david.murray, 2013-09-16 19:03
Messages (32)
msg162132 - (view) 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.
msg162165 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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.
msg162556 - (view) 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.
msg162561 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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.
msg197319 - (view) 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.
msg197803 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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.)
msg197804 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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).
msg197808 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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.
msg197810 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-15 18:49
Here is a 2.6 specific patch.  I've hand tested this.
msg197811 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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?
msg197812 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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__().
msg197815 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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.
msg197822 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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".)
msg197828 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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...
msg197829 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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 msg197815?  I thought you were only going to keep the permission test?
msg197832 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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?
msg197896 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-16 12:47
@RDM: Please commit to 2.6 and null merge to 2.7.  Thanks!
msg197908 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-16 15:12
Well, I was planning to merge it, since 2.7 needs the fix as well.
msg197909 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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!
msg197926 - (view) 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
msg197927 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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.
msg197931 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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?
msg197961 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) 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.
msg197999 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-09-17 20:34
Fine for 3.1.
msg198002 - (view) 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
msg198003 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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!
msg198004 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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>
> _______________________________________
msg198015 - (view) 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
msg198016 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-18 11:40
Thanks, Benjamin.  And Thank you, Bruno.
msg198021 - (view) 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
msg199342 - (view) 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.
msg199350 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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
2013-10-09 21:55:33r.david.murraysetmessages: + msg199350
2013-10-09 21:11:33bruno.Piguetsetmessages: + msg199342
2013-09-18 13:00:17python-devsetmessages: + msg198021
2013-09-18 11:40:47r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg198016

stage: resolved
2013-09-18 11:38:28python-devsetmessages: + msg198015
2013-09-18 01:46:36benjamin.petersonsetmessages: + msg198004
2013-09-18 01:39:08r.david.murraysetmessages: + msg198003
2013-09-18 00:11:45python-devsetmessages: + msg198002
2013-09-17 20:34:51benjamin.petersonsetmessages: + msg197999
2013-09-17 05:35:52georg.brandlsetmessages: + msg197961
2013-09-16 19:03:55r.david.murraysetfiles: + netrc-py3.1.patch

messages: + msg197931
2013-09-16 18:36:41r.david.murraysetmessages: + msg197927
versions: - Python 2.6, Python 2.7
2013-09-16 18:35:03python-devsetnosy: + python-dev
messages: + msg197926
2013-09-16 15:18:44barrysetmessages: + msg197909
2013-09-16 15:12:58r.david.murraysetmessages: + msg197908
2013-09-16 12:47:11barrysetmessages: + msg197896
2013-09-15 20:39:14r.david.murraysetmessages: + msg197832
2013-09-15 20:15:57r.david.murraysetfiles: + netrc-2.6.patch
2013-09-15 20:15:41r.david.murraysetfiles: - netrc-2.6.patch
2013-09-15 20:15:27barrysetmessages: + msg197829
2013-09-15 20:09:25r.david.murraysetfiles: + netrc-2.6.patch

messages: + msg197828
2013-09-15 19:43:21barrysetmessages: + msg197822
2013-09-15 19:30:29Arfreversetnosy: + Arfrever
2013-09-15 19:30:23Arfreversetversions: + Python 3.1
2013-09-15 19:09:42r.david.murraysetmessages: + msg197815
2013-09-15 18:54:05barrysetmessages: + msg197812
2013-09-15 18:51:06r.david.murraysetmessages: + msg197811
2013-09-15 18:49:38r.david.murraysetfiles: + netrc-2.6.patch
keywords: + patch
messages: + msg197810
2013-09-15 18:44:22barrysetpriority: high -> release blocker
nosy: + larry, benjamin.peterson, georg.brandl
2013-09-15 18:44:00barrysetmessages: + msg197808
2013-09-15 18:08:40r.david.murraysetmessages: + msg197804
2013-09-15 18:05:03r.david.murraysetmessages: + msg197803
2013-09-09 06:27:37pitrousetnosy: + giampaolo.rodola
2013-09-09 06:24:59bruno.Piguetsetversions: + Python 3.2
2013-09-08 21:02:24bruno.Piguetsetfiles: + patch_netrc_3.4.0a1.txt

messages: + msg197319
versions: + Python 3.4, - Python 3.2
2012-06-09 13:58:40r.david.murraysetnosy: + barry
messages: + msg162561
2012-06-09 09:55:20bruno.Piguetsetfiles: + patch_netrc_2.6.txt

messages: + msg162556
2012-06-02 19:02:09r.david.murraysetpriority: normal -> high

type: security
components: + Library (Lib)
versions: + Python 2.7, Python 3.2, Python 3.3
nosy: + r.david.murray

messages: + msg162165
2012-06-02 12:54:19bruno.Piguetsettitle: netrc module alows read of non-secured .netrc file -> netrc module allows read of non-secured .netrc file
2012-06-02 12:53:41bruno.Piguetcreate