The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #12463

introduce better, and more secure, file handling abstractions, then use them in our code

Added by Daniel Pittman about 2 years ago. Updated about 2 years ago.

Status:ClosedStart date:02/06/2012
Priority:NormalDue date:
Assignee:Dominic Maraglia% Done:

100%

Category:-
Target version:2.7.11
Affected Puppet version: Branch:
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

We have a bunch of places that implement “secure” file handling, and a bunch of old ways to do that safely. For example, secure_open is rarely used and has some … very strange behaviours, but implements one pattern for securely writing to a file.

Other places, such as the file type, the ssh_authorized_keys type, and the k5login type (and their providers), have other patterns for securely handling this: * change to the target user before operating, potentially writing through a symlink anyway. * use secure_open * write your own “secure temporary file” operation with a predictable filename and bad symlink behaviour

We also have some methods over in the settings, and other parts of the code, that work to write files out safely.

We should unify these into a single, sane file handling system. This should be useful in all the places we write files, and replace them.

My work on replace_file is a start to this, but we should talk about the details.

Jeff references this:

Basically what’s described in the safeopen paper [1] which mainly revolves around validating the path is only modifiable by trusted ID’s (EUID and root to start). [1] http://research.cs.wisc.edu/mist/safefile/safeopen_ares2008.pdf


Subtasks

Bug #12460: Insecure handling of file writes in k5login typeClosedDominic Maraglia

Bug #12462: possible data loss, unlikely (requires write access to /e...ClosedDominic Maraglia

History

#1 Updated by Jason McKerr about 2 years ago

  • Assignee changed from Jason McKerr to Deepak Giridharagopal

#3 Updated by Daniel Pittman about 2 years ago

I helpfully reviewed both branches, and left a bunch of comments. Pretty much everything on the 2.6.x branch also applies to the 2.7.x branch; the comments there are deliberately just the extra stuff that, eg, Windows added.

Apparently I had more things on my mental todo list for this code than was obvious to me when I wrote the tickets. Sorry that wasn’t super well communicated.

#4 Updated by Matthaus Owens about 2 years ago

  • Status changed from Accepted to Closed
  • Target version set to 2.7.11
  • Private changed from Yes to No

released in 2.6.14 and 2.7.11

Also available in: Atom PDF