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

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Feature #8120

Let user change hashing algorithm, to avoid crashing on FIPS-compliant hosts

Added by Anonymous almost 5 years ago. Updated about 2 years ago.

Status:Code InsufficientStart date:06/28/2011
Priority:NormalDue date:
Assignee:-% Done:

0%

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

We've Moved!

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

This ticket is now tracked at: https://tickets.puppetlabs.com/browse/PUP-1840


Description

I’m using Puppet in part to ensure Federal Information Processing Standard 140-2 (FIPS 140-2) compliance on my network. Part of this compliance for the system underlying Puppet is to make sure that only FIPS Approved algorithms are used. OpenSSL does this by ensuring that any attempts to run an unapproved algorithm result in either a SIGSEGV or a SIGABRT. MD5 has been broken enough that it is no longer a FIPS Approved algorithm.

The consequence for Puppet is that, if it tries to use MD5 on a FIPS-compliant system, it will crash. Here is where I have seen Puppet crash for this reason:

  1. the puppet/util/checksums.rb, used by File resources;
  2. the puppet/parser/functions/md5.rb, implementation of the md5 DSL function;
  3. certificate signature in puppet/ssl/certificate_request.rb;
  4. certificate fingerprinting in puppet/ssl/base.rb;
  5. outside Puppet, in the session ID code in openssl/ssl-internal.rb, class OpenSSL::SSLServer, due to using WEBrick.

It was easy enough to replace MD5 with SHA256 in all those places – and, in case 4, it appears I may not have needed to change the code; but the DSL function is still called md5, and MD5 is still named in some of the messages. My changes lack the refinement necessary to be useful to others.

What I think I need is to be able to say, in one place like puppet.conf, “use SHA256, not MD5,” and algorithms and messages alike will change. I think the md5 DSL function would need to be replaced with a digest function which uses the configured algorithm, and there should also be a way in the DSL to find out which digest is being used, like a digestname function.

Then, in some years when SHA2 is decertified, I can tell Puppet, “use SHA3, not SHA2,” instead of filing an issue like this one and doing code changes. (I don’t know what migration issues this scenario may pose.)

How can I make Red Hat Enterprise Linux 5 FIPS 140-2 compliant? (Red Hat login required)


Related issues

Related to Puppet - Bug #6663: puppet.conf says keylength defaults to 1024 -- should be ... Closed 03/10/2011
Related to Puppet - Feature #13435: CSRs should be signed with SHA1, not MD5 Closed 03/27/2012
Related to Puppet - Bug #17295: Puppet not honouring --digest Closed
Related to Puppet - Feature #21029: Allow control over the digest used to create CA certificates Accepted
Related to Puppet - Feature #21257: Add a configuration option for the digest algorithm used ... Needs Decision

History

#1 Updated by Anonymous almost 5 years ago

  • Category set to security
  • Assignee set to Nigel Kersten

From an implementation PoV, we will want to be able to support MD5 for older clients, even when we move the core checksum to something modern and supportable. Thankfully, with access to the client version in the catalog compiler we can determine if this is actually a requirement for compatibility.

Overall, this is a good change to be making in the longer term. (Also, wouldn’t it be nice if the FIPS version of OpenSSL wouldn’t just terminate the application? Oh, well. It is more secure.)

#2 Updated by Anonymous almost 5 years ago

Daniel Pittman wrote:

(Also, wouldn’t it be nice if the FIPS version of OpenSSL wouldn’t just terminate the application? Oh, well. It is more secure.)

That’s a good point.

OpenSSL FIPS User Guide, sec. 2.6.2: “By design, the OpenSSL API attempts to disable non-FIPS algorithms, when in FIPS mode, at the EVP [envelope, I believe] level and via most low level function calls. Failure to check the return code from low level functions could result in unexpected behavior…”

The SIGSEGV I saw came when a null function pointer was called. That function pointer was in a structure describing the hash algorithm in use, which had been returned from an OpenSSL get_me_the_algorithm(“MD5”) kind of call. The SIGABRT I saw went like this:

digest.c(149): OpenSSL internal error, assertion failed: Digest init previous FIPS forbidden algorithm error ignored
Aborted

So it seems that application termination could be averted, by improving Ruby’s openssl module. My ruby-talk searches have turned up only one person mentioning FIPS compliance, who was never answered, so there’s room for improvement here.

I guess I’ll bring this up on ruby-talk.

#4 Updated by James Turnbull almost 5 years ago

  • Status changed from Unreviewed to Needs Decision

#5 Updated by Peter Meier almost 5 years ago

+1 for making it configurable

#6 Updated by Anonymous almost 5 years ago

More places to replace Digest::MD5: lib/puppet/file_bucket/dipper.rb and lib/puppet/file_bucket/file.rb. I guess I wasn’t exercising them. A grep is more exhaustive than my anecdotal findings. But in my brutish replacement of Digest::MD5 with Digest::SHA256, I missed

Puppet::FileBucketFile::File#request_to_checksum_and_path
.

I’d come up with decent patches but I don’t think I understand the design well enough yet.

#7 Updated by Anonymous almost 5 years ago

I may have “come up with decent patches,” i.e. fixed this at my site in a non-hackish manner. I added a setting “digest_algorithm” in the “main” section of puppet.conf which defaults to “md5”, made all the file (checksum, bucket, download) machinery default to Puppet[:digest_algorithm] and then to :md5, and added a “digest” DSL function which uses the algorithm named by the digest_algorithm setting, or defaults to MD5. I changed all the tests (er, the RSpec examples, whatnot) so that everywhere it could matter which digest_algorithm setting we use, we try the test for digest_algorithm set to each of nil, “md5” and “sha256”.

There were a couple of places in puppet/ssl* where things are cryptographically signed and MD5 is named specifically. Perhaps these should pay attention to the “digest” setting in one or another section of puppet.conf. I just changed them to SHA1 for now.

I feel sure that the digest setting which is presently used for some SSL stuff, and the digest_algorithm setting which I’m using for checksumming files, should be independent settings, but as I’ve implemented it, their names would be easy to confuse. Digest_algorithm is not only used for files, but also for the digest function; I don’t see a reason to set those separately. Any ideas on a better name?

#8 Updated by Anonymous over 4 years ago

  • Branch set to https://github.com/puppetlabs/puppet/pull/195

The patches are now on github.

#9 Updated by Nigel Kersten over 4 years ago

  • Assignee changed from Nigel Kersten to Jason McKerr

Jason, can we get someone to review these?

#10 Updated by Anonymous over 4 years ago

I have taken a look at the code, and it generally looks reasonable enough. There are some small concerns that would need to be addressed if we were going to merge this, but my strong advise is that we do not:

The change to SHA1 from MD5 in the SSL area is pretty trivial, and should be fine. We don’t really have any great compatibility concerns there because the SSL libraries take care of handling multiple digests for us.

Unfortunately, this patchset doesn’t account for any compatibility on the Puppet side. It is still support for one, and only one, digest, and will fail hard if anything else is used. That means that you have to change the setting on every node in your network, all at once, to the same digest, or it will not work.

It also fails if, say, you had a bunch of MD5 hashed files in a file bucket, then change the digest. The previous files are not upgraded or changed in any way, and there are no migration tools supplied. That makes it easy to lose files, and so break, eg, links between the Dashboard and the historic files that were inspected.

Technically, it doesn’t actually break that, because there is no change to the digest stuff in the Dashboard, which will now silently truncate (thanks, MySQL) or fail to import reports, but the older values will still refer to the older hash, and that would work, except that the FileBucket will reject a hash where the digest length doesn’t match the new setting…

I would happily advise people who needed to avoid MD5 digests right now to apply this, provided they were in a green-fields deployment, or could scrap their existing history in Puppet.

I wouldn’t put it into the main product repository without planning for the extra two to eight weeks designing and implementing compatibility into the changes, so that we don’t have to upgrade every node, and so that we can migrate our older data (where possible) to the new content.

(eg: support both old and new hash to access a file in the bucket, upgrade Dashboard and other API to handle multiple digest lengths, and to type them, handle compatibility fallbacks when agent and master have different digests configured, perhaps publish the digest configuration from the master to the agent, etc.)

So, again, this is a good change from the PoV of just getting away from MD5, but a bad change in terms of our compatibility story at this point in time. It would serve as the heart of building the rest of the changes, but we shouldn’t ship it until we are invested in doing that, because of the ability to blow your own foot off that it presently adds.

#11 Updated by Anonymous over 4 years ago

I am following this up internally to see where we go next.

#12 Updated by Anonymous about 4 years ago

Daniel Pittman wrote:

I am following this up internally to see where we go next.

This is on the roadmap to identify the best way to move forward despite the weight of our legacy clients, but our team hasn’t had time to work on that yet. We hope that will shortly get to the head of the list.

#13 Updated by micah - about 4 years ago

Daniel Pittman wrote:

From an implementation PoV, we will want to be able to support MD5 for older clients, even when we move the core checksum to something modern and supportable

and jared jennings wrote:

I may have “come up with decent patches,” i.e. fixed this at my site in a non-hackish manner. I added a setting “digest_algorithm” in the “main” section of puppet.conf which defaults to “md5”

I completely understand and agree that supporting MD5 for older clients should be possible, however I think that setting the default to MD5 is the wrong approach. It means that everyone will be using a poor hash by default, unless they change the configuration. This seems backwards, it should be everyone should be using a modern algorithm by default, and those who are stuck with old and broken clients should be the ones who step down their algorithm to MD5. The old and broken clients should not drag down the rest of the community by default.

#14 Updated by Anonymous about 4 years ago

micah – wrote:

Daniel Pittman wrote:

From an implementation PoV, we will want to be able to support MD5 for older clients, even when we move the core checksum to something modern and supportable

and jared jennings wrote:

I may have “come up with decent patches,” i.e. fixed this at my site in a non-hackish manner. I added a setting “digest_algorithm” in the “main” section of puppet.conf which defaults to “md5”

I completely understand and agree that supporting MD5 for older clients should be possible, however I think that setting the default to MD5 is the wrong approach. It means that everyone will be using a poor hash by default, unless they change the configuration. This seems backwards, it should be everyone should be using a modern algorithm by default, and those who are stuck with old and broken clients should be the ones who step down their algorithm to MD5. The old and broken clients should not drag down the rest of the community by default.

Absolutely. The default will be secure, and the real question is how we support older clients, and how we migrate existing pools of data across the entire Puppet platform.

#15 Updated by Anonymous about 4 years ago

Looking through the puppet dashboard code it seems like we might be able to come up with a way of providing backwards compatibility. The digests that are stored in the dashboard’s database are formated as “{md5}%s” which provides us with the algorithm for that digest. If we extend this scheme of formatting digests throughout more of the puppet system, then we should have a way of tracking files across digest algorithm changes. We’ll probably need to track multiple digests for the same file in the filebucket system.

#16 Updated by Jeff Weiss about 4 years ago

In terms of Ruby compatibility, it looks like ossl_digest_initialize in the trunk of 1.8.7 and 1.9.2-p381 do not check the appropriate return code from EVP_DigestInit_ex.

If we’re going to support running in a FIPS 140-2 environment, for now we’ll need to specify that it will only work with 1.9.3-p0 or greater.

#17 Updated by Anonymous about 4 years ago

In the specific case of RHEL and derivatives, the above-linked Red Hat issue has resulted in a patch for Ruby 1.8.7, http://rhn.redhat.com/errata/RHSA-2011-1581.html. (I recognize you wouldn’t usually go into distro-specific detail for announcements or documentation, but FIPS 140-2 validation is frequently that detailed, so FIPS-specific Puppet documentation may need to be, also.)

#18 Updated by Anonymous over 3 years ago

  • Branch changed from https://github.com/puppetlabs/puppet/pull/195 to https://github.com/puppetlabs/puppet/pull/1033

As the comments in the pull requests say, I’ve made a new topic branch with the same changes against a newer commit of the master branch.

This new pull request does not solve any of the migration issues discussed above. It’s just probably easier to pull these days than the original one.

#19 Updated by Anonymous over 3 years ago

  • Branch changed from https://github.com/puppetlabs/puppet/pull/1033 to https://github.com/puppetlabs/puppet/pull/1035

I committed (ah hehe) some sort of grievous error with git. Accordingly I closed pull request 1034 and opened pull request 1035.

As before, no new changes: just the old ones rebased on a new master commit.

#20 Updated by Adrien Thebo over 2 years ago

  • Status changed from Needs Decision to Code Insufficient
  • Assignee deleted (Jason McKerr)
  • Branch deleted (https://github.com/puppetlabs/puppet/pull/1035)

This pull request was based on the 2.7.x line which would have been painful to get updated on top of the current master. I’ve suggested that we open a new set of pull requests against master and add features incrementally so we can get things merged in chunks instead of One Grand Pull Request ™.

#21 Updated by eric sorenson about 2 years ago

Redmine Issue #8120 has been migrated to JIRA:

https://tickets.puppetlabs.com/browse/PUP-1840

Also available in: Atom PDF