netrc module allows read of non-secured .netrc file

[brunoPiguet]

BPO	14984
Nosy	@warsaw, @birkenfeld, @larryhastings, @giampaolo, @benjaminp, @bitdancer
Files	patch_netrc_2.6.txt: example of what could be a patch against the 2.6 version of netrc.py patch_netrc_3.4.0a1.txt: proposed patch against 3.4.0a1 version of netrc.py netrc-2.6.patch netrc-2.6.patch netrc-py3.1.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-09-18.11:40:47.309>
created_at = <Date 2012-06-02.12:53:41.144>
labels = ['type-security', 'library', 'release-blocker']
title = 'netrc module allows read of non-secured .netrc file'
updated_at = <Date 2013-10-09.21:55:33.485>
user = 'https://bugs.python.org/brunoPiguet'

bugs.python.org fields:

activity = <Date 2013-10-09.21:55:33.485>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2013-09-18.11:40:47.309>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2012-06-02.12:53:41.144>
creator = 'bruno.Piguet'
dependencies = []
files = ['25868', '31686', '31776', '31780', '31800']
hgrepos = []
issue_num = 14984
keywords = ['patch']
message_count = 32.0
messages = ['162132', '162165', '162556', '162561', '197319', '197803', '197804', '197808', '197810', '197811', '197812', '197815', '197822', '197828', '197829', '197832', '197896', '197908', '197909', '197926', '197927', '197931', '197961', '197999', '198002', '198003', '198004', '198015', '198016', '198021', '199342', '199350']
nosy_count = 9.0
nosy_names = ['barry', 'georg.brandl', 'larry', 'giampaolo.rodola', 'benjamin.peterson', 'Arfrever', 'r.david.murray', 'python-dev', 'bruno.Piguet']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue14984'
versions = ['Python 3.1', 'Python 3.2', 'Python 3.3', 'Python 3.4']

[brunoPiguet]

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.

[bitdancer]

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.

[brunoPiguet]

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.

[bitdancer]

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.

[brunoPiguet]

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.

[bitdancer]

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

[bitdancer]

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

[warsaw]

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.

[bitdancer]

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

[bitdancer]

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?

[warsaw]

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