Fix ReDoS (GHSL-2024-323)

[kevinbackhouse]

• Have you followed the guidelines for contributing?
• Have you signed the CLA?
• Have you successfully run tox run -m lint?
• Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

---

Fixes a ReDoS: https://bugs.launchpad.net/snapcraft/+bug/2086622 (GHSL-2024-323)

[mr-cal]

Hi, I'm not sure what this is solving, can you provide details?

https://bugs.launchpad.net/snapcraft/+bug/2086622 doesn't seem to exist.

[kevinbackhouse]

GitHub Security Lab (GHSL) Vulnerability Report, snapcraft: GHSL-2024-323

The GitHub Security Lab team has identified a potential security vulnerability in snapcraft.

We are committed to working with you to help resolve this issue. In this report you will find everything you need to effectively coordinate a resolution of this issue with the GHSL team.

If at any point you have concerns or questions about this process, please do not hesitate to reach out to us at securitylab@github.com (please include GHSL-2024-323 as a reference). See also this blog post written by GitHub's Advisory Curation team which explains what CVEs and advisories are, why they are important to track vulnerabilities and keep downstream users informed, the CVE assigning process, and how they are used to keep open source software secure.

If you are NOT the correct point of contact for this report, please let us know!

Summary

A snap with a crafted yaml file can cause a denial of service in snapcraft.

Project

snapcraft

Tested Version

8.4.4

Details

ReDoS in _validate_time (GHSL-2024-323)

_validate_time uses a regex to check that time values in a snap's yaml file are formatted correctly:

@pydantic.field_validator(
    "start_timeout", "stop_timeout", "watchdog_timeout", "restart_delay"
)
@classmethod
def _validate_time(cls, timeval):
    if not re.match(r"^[0-9]+(ns|us|ms|s|m)*$", timeval):
        raise ValueError(f"{timeval!r} is not a valid time value")

    return timeval

For example, 10s or 10ms are both valid times. But the regex has a ReDoS vulnerability, which means that it runs very slowly on invalid inputs like this:

0msmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsx

To reproduce the ReDoS with snapcraft, first create an empty snap by running this command:

snapcraft init

Then replace the contents of ./snap/snapcraft.yaml with this text:

name: GHSL-2024-323
base: core24

apps:
  GHSL-2024-323:
    command: /bin/echo kevwozere
    restart-delay: 0msmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsmsx

Now run the following command and observe that it takes a long time to finish:

snapcraft

The running time increases exponentially with the length of the invalid string.

Impact

This issue may lead to denial of service. We have only tested our PoC locally, but it may also be possible to trigger a denial of service on https://snapcraft.io/ by uploading a malicious snap.

Remediation

This is our suggested fix:

diff --git a/snapcraft/models/project.py b/snapcraft/models/project.py
index 0b2b0bee..63aab072 100644
--- a/snapcraft/models/project.py
+++ b/snapcraft/models/project.py
@@ -449,7 +449,7 @@ class App(models.CraftBaseModel):
     )
     @classmethod
     def _validate_time(cls, timeval):
-        if not re.match(r"^[0-9]+(ns|us|ms|s|m)*$", timeval):
+        if not re.match(r"^[0-9]+(ns|us|s|m)*$", timeval):
             raise ValueError(f"{timeval!r} is not a valid time value")

         return timeval

It fixes the ReDoS without changing the behavior of the regex.

GitHub Security Advisories

We recommend you create a private GitHub Security Advisory for this finding. This also allows you to invite the GHSL team to collaborate and further discuss this finding in private before it is published.

Credit

This issue was discovered and reported by GHSL team member @kevinbackhouse (Kevin Backhouse).

Contact

You can contact the GHSL team at securitylab@github.com, please include a reference to GHSL-2024-323 in any communication regarding this issue.

Disclosure Policy

This report is subject to a 90-day disclosure deadline, as described in more detail in our coordinated disclosure policy.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.68%. Comparing base (654871d) to head (afbd2de).
Report is 1175 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (654871d) and HEAD (afbd2de). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (654871d)	HEAD (afbd2de)
	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5210      +/-   ##
==========================================
- Coverage   94.88%   89.68%   -5.20%     
==========================================
  Files         658      341     -317     
  Lines       55189    22614   -32575     
==========================================
- Hits        52364    20282   -32082     
+ Misses       2825     2332     -493     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lengau]

Fortunately I don't believe this will affect anything but snapcraft - the only service that would ever run that code is a Launchpad remote build.

[lengau]

Given that these are timeout strings that get passed unaffected to snapd and parsed using golang's time.ParseDuration, I believe a more correct version of this regex would be:

Suggested change

	if not re.match(r"^[0-9]+(ns|us|s|m)*$", timeval):
	if not re.match(r"^([0-9]+(ns|us|ms|s|m)){1,5}$" timeval):

[lengau]

Reading this brought up a few things in my mind related to how we check these duration strings, as I don't think the original regex is even correct.

I've made a separate PR that should both correct the values we allow and resolve this issue.

[kevinbackhouse]

@lengau: Thanks! Your version is better. My goal was just to fix the ReDoS without changing the behavior of the regex, but yours changes it so that the regex is more precise. I'll close this PR now.

[lengau]

Thanks again for alerting us to this @kevinbackhouse ! The reproducer was great as it showed both the particular issue and the problem my PR fixes 😀