[PUP-7834] Change all calls to YAML.load into YAML.safe_load Created: 2017/08/11  Updated: 2018/09/19  Resolved: 2018/09/05

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: None
Fix Version/s: PUP 6.0.0

Type: Improvement Priority: Normal
Reporter: Thomas Hallgren Assignee: Josh Cooper
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
is duplicated by PUP-7519 Enable rubocop security cop scan on r... Closed
relates to PUP-9104 YAML safe_load prevents aliases Closed
Acceptance Criteria:

That rubocop can run clean on the puppet source with Security/YAMLLoad and Security/JSONLoad enabled.

Team: Coremunity
Story Points: 1
Sprint: Platform Core KANBAN
Release Notes: Bug Fix
Release Notes Summary: Puppet now uses YAML.safe_load consistently to ensure only known classes are loaded.
QA Risk Assessment: Needs Assessment


Now, when all serialization of YAML data is ensured to be Data, we must also ensure that no unsafe data can be loaded using YAML. Psych provides the method YAML.safe_load to accomplish this.

We do have some places were we still load objects that are not Data for backward compatibility causes. We allow Symbol keys in hiera in some places and we provide a YAML-specificit tag for Puppet::Node::Facts to make it directly deserializable into instances of that class. Such exceptions can (and should) be declared specifically as arguments to YAML.safe_load.

Also need to review JSON.load

Comment by Henrik Lindberg [ 2018/02/26 ]

Josh Cooper What is left to do here? - The questions raised by you on the PR have been answered from what I can see.

Comment by Josh Cooper [ 2018/03/08 ]

Now that we have a 6.0 branch (master), I'd prefer to land this PR there instead of 5.y. LGTM.

Comment by Josh Cooper [ 2018/04/02 ]

See last PR comments.

Comment by Jayant Sane [ 2018/08/06 ]

Wonder if we need to either disable Marshal.Load cop/rule or exclude cipher_test-v3.rb file (two instances that are not under any of the excluded folders)

Comment by Josh Cooper [ 2018/08/06 ]

AFAICT Security/MarshalLoad is included in the current .rubocop.yml configuration (for puppet not sure about other repos). I didn't see cipher_test-v3.rb is that a PE acceptance thing?

Comment by Jayant Sane [ 2018/08/06 ]

You are right - I too was indicating MarshalLoad has been implicitly enabled and hence had mistakenly wondered about cipher_test-v3.rb. My bad on cipher_test-v3.rb - for some reason these had ended up in my local puppet repo which I forgot to do a git status on.

Comment by Kris Bosland [ 2018/08/30 ]

Merged to master at 1a63db1.

Generated at Mon Dec 16 04:26:18 PST 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.