Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Loader alias to UnsafeLoader and enhance security #851

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

banteg
Copy link

@banteg banteg commented Mar 7, 2025

Background

The Lazarus Group, a North Korean state-sponsored hacking group, has been actively exploiting PyYAML's unsafe loader functionality to conduct advanced persistent threat (APT) attacks. Most recently, they were responsible for the $1.5 billion Bybit cryptocurrency exchange hack in February 2025 - the largest heist in history.

The attackers specifically used PyYAML's yaml.Loader vulnerability to execute remote code execution (RCE) attacks by tricking exchange employees into running seemingly legitimate Python code that contained:

data = yaml.load(response.text, Loader=yaml.Loader)

Changes in this PR

This PR significantly enhances PyYAML's security posture through several critical changes:

  1. Removed the Loader alias to UnsafeLoader: This prevents accidental use of the unsafe loader
  2. Added runtime warnings: When UnsafeLoader, CUnsafeLoader, or unsafe_load() functions are used
  3. Changed default loaders: Switched from UnsafeLoader to SafeLoader in various core functions
  4. Improved documentation: Added explicit security warnings in docstrings

Security Impact

These changes provide multiple layers of protection:

  1. Breaking potential attack vectors: The removal of the Loader alias prevents the specific attack pattern used in the Bybit hack
  2. Raising awareness: Runtime warnings alert developers when they're using unsafe loading methods
  3. Safer defaults: Even if developers don't specify a loader, the safer options are now used by default
  4. Clear documentation: More explicit warnings in docstrings help educate developers about security risks

Backward Compatibility

While this PR makes security-focused breaking changes, the actual impact should be minimal:

  • Code using yaml.SafeLoader or yaml.safe_load() will continue to work without changes
  • Code using yaml.FullLoader or yaml.full_load() will continue to work without changes
  • Code using yaml.UnsafeLoader or yaml.unsafe_load() will continue to work but will now generate runtime warnings
  • Code using the yaml.Loader alias will need to be updated to either use yaml.UnsafeLoader (not recommended) or preferably migrate to yaml.SafeLoader or yaml.FullLoader

References

  1. PyYAML vulnerability documentation
  2. CVE-2020-14343 - Arbitrary code execution in PyYAML
  3. SlowMist Security report on Lazarus Group attack techniques
  4. FBI attribution of Bybit hack to North Korea

This PR helps eliminate a serious security vulnerability that has been exploited by nation-state actors to steal billions in cryptocurrency assets. By removing the unsafe loader alias and promoting safer defaults, we can help prevent future attacks.

* Remove Loader class as an alias to UnsafeLoader
* Add warning messages when using UnsafeLoader, CUnsafeLoader, unsafe_load(), and unsafe_load_all()
* Change default loader in scan(), parse(), compose(), and compose_all() from UnsafeLoader to SafeLoader
* Improve documentation with clear security warnings

These changes help prevent accidental RCE vulnerabilities when processing untrusted YAML data.

🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
@AndrewMohawk
Copy link

Thank you!

* Replace yaml.Loader with yaml.UnsafeLoader in test_structure.py
* Update test_yaml_ext.py to use UnsafeLoader/CUnsafeLoader instead of removed Loader class
* Ensures tests still work with the removal of the Loader alias

🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
@enolife
Copy link

enolife commented Mar 7, 2025

Good job

@MicahZoltu
Copy link

Why not alias Loader to SafeLoader? Are the APIs not compatible with each other? This would minimize the scope of breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants