Repository navigation
netrc module allows read of non-secured .netrc file #59189
Description
| BPO | 14984 |
|---|---|
| Nosy | @warsaw, @birkenfeld, @larryhastings, @giampaolo, @benjaminp, @bitdancer |
| Files |
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']
Activity
brunoPiguet commentedon Jun 2, 2012
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.
[-]netrc module alows read of non-secured .netrc file[/-][+]netrc module allows read of non-secured .netrc file[/+]bitdancer commentedon Jun 2, 2012
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 commentedon Jun 9, 2012
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 commentedon Jun 9, 2012
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 commentedon Sep 8, 2013
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 commentedon Sep 15, 2013
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 commentedon Sep 15, 2013
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 commentedon Sep 15, 2013
On Sep 15, 2013, at 06:05 PM, R. David Murray wrote:
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 commentedon Sep 15, 2013
Here is a 2.6 specific patch. I've hand tested this.
bitdancer commentedon Sep 15, 2013
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 commentedon Sep 15, 2013
On Sep 15, 2013, at 06:51 PM, R. David Murray wrote:
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__().
8 remaining items
python-dev commentedon Sep 16, 2013
New changeset e5c4eb6b8e05 by R David Murray in branch '2.6':
bpo-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 bpo-14984: On POSIX, enforce permissions when reading default .netrc.
http://hg.python.org/cpython/rev/2e19c65d6688
bitdancer commentedon Sep 16, 2013
Removing 2.6 and 2.7 from versions since it is now fixed there. I'll work on porting it to python3.
bitdancer commentedon Sep 16, 2013
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?
birkenfeld commentedon Sep 17, 2013
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.
benjaminp commentedon Sep 17, 2013
Fine for 3.1.
python-dev commentedon Sep 18, 2013
New changeset 1b673e0fd8f3 by R David Murray in branch '2.6':
Add versionchanged for bpo-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 bpo-14984, remove extra blank from string.
http://hg.python.org/cpython/rev/48be42b94381
bitdancer commentedon Sep 18, 2013
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!
benjaminp commentedon Sep 18, 2013
You should be able to push now.
2013/9/17 R. David Murray <report@bugs.python.org>:
python-dev commentedon Sep 18, 2013
New changeset 6396d1fc72da by R David Murray in branch '3.1':
bpo-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 bpo-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 bpo-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 bpo-14984: On POSIX, enforce permissions when reading default .netrc.
http://hg.python.org/cpython/rev/bfc86caf98ae
bitdancer commentedon Sep 18, 2013
Thanks, Benjamin. And Thank you, Bruno.
python-dev commentedon Sep 18, 2013
New changeset fb3ad8a749c8 by R David Murray in branch '2.6':
bpo-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 bpo-14984: only import pwd on POSIX.
http://hg.python.org/cpython/rev/88e62c43e443
New changeset 713d71048ab9 by R David Murray in branch '3.1':
bpo-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 bpo-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 bpo-14984: only import pwd on POSIX.
http://hg.python.org/cpython/rev/b8206cb2c4ee
New changeset ad9a5ded5cf6 by R David Murray in branch 'default':
Merge bpo-14984: only import pwd on POSIX.
http://hg.python.org/cpython/rev/ad9a5ded5cf6
brunoPiguet commentedon Oct 9, 2013
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.
bitdancer commentedon Oct 9, 2013
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.