authentication.conf should not be created "chmod go+r"

Bug #475501 reported by Joke de Buhr
This bug affects 2 people
Affects Status Importance Assigned to Milestone
In Progress
Joke de Buhr
Fix Released
Jelmer Vernooij

Bug Description

lauchpad-login and other plugins may use set_credential() to create the authentication.conf file. Since it's very likely someone might at passwords later on the file should never be created world readable initially.
It's safer to create the file with a umask(0177) rather the default umask. This way it's impossible to gain access to user-stored password if the user forget to run "chmod u=rw,go= ~/.bazaar/authenfication.conf" after adding ftp passwords or whatever.
Even set_credential() has the ability to store passwords so umasking the file would be safer if plugins start to set passwords with set_credential().

Related branches

Joke de Buhr (joke)
visibility: private → public
Revision history for this message
Vincent Ladeuil (vila) wrote :

Thanks for working on this !

Would you mind doing a merge proposal so we can better track your patch ?
A couple of comments in the mean time:

The _save() docstring is out of date: obviously this is used outside of tests now,

Can you add tests to:
- check that the file is created with the right umask,
- check that we issue the warning if not (and mention the mode bits in the warning)

I'm not sure about how to address that concern on windows, but at least to start we may not want to
do a check there if there is no way to make it succeeds...

Don't hesitate to bring the topic on the mailing list where we'll get more feedback.

Changed in bzr:
assignee: nobody → Joke de Buhr (joke)
importance: Undecided → Medium
status: New → In Progress
Revision history for this message
Joke de Buhr (joke) wrote :

No problem. I'm going to propose a merge after adding some checks.

I've got no experience in python windows programming but as far as I know functions like os.umask() should work in a compatible workaround way or be ignored on windows.

so far the patch has this functions:
 - bzr's ~/.bazaar/authentication.conf is initially created with permissions 600
 - bzr emits a warning upon accessing the file if it's permissions are insecure (g!=0, o!= 0)
 - warning can be disabled in ~/.bazaar/bazaar.conf

Jelmer Vernooij (jelmer)
tags: added: authentication
Revision history for this message
John A Meinel (jameinel) wrote :

Joke did do some work on this, but never proposed it for merging. Presumably because he wanted to add some test cases. For now, I'm just going to propose the branch to get this unstuck a bit.

Jelmer Vernooij (jelmer)
Changed in brz:
milestone: none → 3.0.0
Jelmer Vernooij (jelmer)
Changed in brz:
status: New → Triaged
importance: Undecided → High
importance: High → Medium
Jelmer Vernooij (jelmer)
Changed in brz:
status: Triaged → In Progress
assignee: nobody → Jelmer Vernooij (jelmer)
Jelmer Vernooij (jelmer)
Changed in brz:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.