Project

General

Profile

Bug #4648

CVE-2014-0135 - Kafo does not handle default_values.yaml securely

Added by Marek Hulán about 9 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Difficulty:
Triaged:
Bugzilla link:
Pull request:
Fixed in Releases:
Found in Releases:
Red Hat JIRA:

Description

/tmp/default_values.yaml file has world readable permissions and does not check for existence when it's being created. Therefore it's prone to race-condition attacks. This file contains default values for all parameters (usually autogenerated passwords)

Proposed fix steps:
  1. we'll use mktmpdir which will be passed to kafo_configure puppet module as a parameter
  2. kafo_configure puppet module will safely create file (check for non-existence, create file with 0600, then dumps data)
  3. packages (rpm, deb, gem) will remove any existing /tmp/default_values.yaml
0001-Fix-4648-store-default-values-securely.patch 0001-Fix-4648-store-default-values-securely.patch 3.62 KB fix for review Marek Hulán, 03/13/2014 03:46 PM
0001-Fix-4648-store-default-values-securely.patch 0001-Fix-4648-store-default-values-securely.patch 3.96 KB fixed file creation race condition Marek Hulán, 03/14/2014 07:34 AM

Associated revisions

Revision f5f2d6d1 (diff)
Added by Marek Hulán almost 9 years ago

Fixes #4648 - make sure default password are not exposed

Revision 9b67a5a2
Added by Marek Hulán almost 9 years ago

Merge pull request #80 from ares/master

Fixes #4648 - make sure default password are not exposed

Revision 417f551e (diff)
Added by Marek Hulán almost 9 years ago

Fixes #4648 - make sure default password are not exposed

History

#1 Updated by Marek Hulán about 9 years ago

  • Description updated (diff)

#3 Updated by Kurt Seifried about 9 years ago

From http://kurt.seifried.org/2012/03/14/creating-temporary-files-securely/

Ruby
use Tempfile for files:

http://www.ruby-doc.org/stdlib-1.9.3/libdoc/tempfile/rdoc/Tempfile.html#method-c-new

use tmpdir for directories (require ‘tmpdir’ and then you can “Dir.mktmpdir”):

http://www.ruby-doc.org/stdlib-1.9.3/libdoc/tmpdir/rdoc/index.html

I would suggest using Tempfile as well. Belt and suspenders.

#4 Updated by Dominic Cleal about 9 years ago

Using mktmpdir should be sufficient to make this safe, so the child Puppet process is told to only write into that shared temporary space. The bit that concerns me in the check for File.exist? in the Puppet function, which isn't safe against races.

Better is to use an exclusive open on the file which will fail to open the file at the kernel level if it already exists, i.e. http://stackoverflow.com/questions/5226166/how-do-open-a-file-for-writing-only-if-it-doesnt-already-exist-in-ruby

#5 Updated by Marek Hulán about 9 years ago

We could create a tempfile in puppet process but after the process finishes it would be removed. So we could create it rather from kafo but then in puppet process we can't be sure about the origin, therefore I think we should be happy with tmp dir only.

Good catch with File.open Dominic, I'm attaching fixed version for re-review.

#6 Updated by Marek Hulán about 9 years ago

  • Subject changed from Kafo does not handle default_values.yaml securely to CVE-2014-0135 - Kafo does not handle default_values.yaml securely

#7 Updated by Dominic Cleal about 9 years ago

  • Status changed from Ready For Testing to Pending

Yep, the predictable filename is required too, but since it's in a safely created temporary directory, that's cool.

ACK, patch looks good.

#8 Updated by Dominic Cleal almost 9 years ago

  • Target version changed from 1.9.0 to 1.8.4

#9 Updated by Marek Hulán almost 9 years ago

  • Private changed from Yes to No

#10 Updated by Marek Hulán almost 9 years ago

  • Status changed from Pending to Closed
  • % Done changed from 0 to 100

#11 Updated by Dominic Cleal almost 9 years ago

Fixes released in Kafo 0.3.17 and 0.5.2.

Also available in: Atom PDF