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.

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.

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.

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.

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.

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.)

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).

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.

Here is a 2.6 specific patch.  I've hand tested this.

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?

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__().

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.

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".)

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...

@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?

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?

@RDM: Please commit to 2.6 and null merge to 2.7.  Thanks!

Well, I was planning to merge it, since 2.7 needs the fix as well.

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!

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

Removing 2.6 and 2.7 from versions since it is now fixed there.  I'll work on porting it to python3.

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?

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.

Fine for 3.1.

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

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!

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>
> _______________________________________

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

Thanks, Benjamin.  And Thank you, Bruno.

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

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.

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.