feat: detect vulnerable GitHub Actions #1021

behnazh-w · 2025-03-21T07:39:42Z

Summary

This pull request introduces a new check mcn_githubactions_vulnerabilities_1 to detect vulnerable GitHub Actions, enhancing the security of workflows and automating the identification of potential risks in CI/CD pipelines. The key changes include:

Changes

A check for specific versions of third-party GitHub Actions that are known to be affected by vulnerabilities. The version can be a commit SHA associated with a tag (version).
A new module has been added to implement interactions with OSV (Open Source Vulnerability) API.

Documentation

The documentation website has been updated to reflect this new check.
A tutorial has been added to guide users through the process of setting up and using the check in their workflows.

Tests

Integration and unit tests have been added.

benmss · 2025-04-08T02:40:47Z

docs/source/index.rst

@@ -46,12 +46,12 @@ Current checks in Macaron
 The table below shows the current set of actionable checks derived from
 the requirements that are currently supported by Macaron.

-.. list-table:: Mapping SLSA requirements to Macaron checks
+.. list-table:: Macaron checks descriptions


Suggested change

.. list-table:: Macaron checks descriptions

.. list-table:: Macaron check descriptions

See commit a684508.

benmss · 2025-04-08T02:44:12Z

docs/source/pages/tutorials/detect_vulnerable_github_actions.rst

+How to detect vulnerable GitHub Actions
+=======================================
+
+This tutorial explains how to use a check in Macaron that detects vulnerable third-party GitHub Actions. This check is important for preventing security issues in your CI/CD pipeline, especially in light of recent incidents, such as vulnerabilities discovered in popular GitHub Actions like `tj-actions/changed-files <https://www.cve.org/CVERecord?id=CVE-2025-30066>`_, and `reviewdog/action-setup <https://www.cve.org/CVERecord?id=CVE-2025-30154>`_.


Suggested change

This tutorial explains how to use a check in Macaron that detects vulnerable third-party GitHub Actions. This check is important for preventing security issues in your CI/CD pipeline, especially in light of recent incidents, such as vulnerabilities discovered in popular GitHub Actions like `tj-actions/changed-files <https://www.cve.org/CVERecord?id=CVE-2025-30066>`_, and `reviewdog/action-setup <https://www.cve.org/CVERecord?id=CVE-2025-30154>`_.

This tutorial explains how to use a check in Macaron to detect vulnerable third-party GitHub Actions. This check is important for preventing security issues in your CI/CD pipeline, especially in light of recent incidents, such as vulnerabilities discovered in popular GitHub Actions: `tj-actions/changed-files <https://www.cve.org/CVERecord?id=CVE-2025-30066>`_, and `reviewdog/action-setup <https://www.cve.org/CVERecord?id=CVE-2025-30154>`_.

See commit a684508.

benmss · 2025-04-08T02:44:31Z

docs/source/pages/tutorials/detect_vulnerable_github_actions.rst

+
+This tutorial explains how to use a check in Macaron that detects vulnerable third-party GitHub Actions. This check is important for preventing security issues in your CI/CD pipeline, especially in light of recent incidents, such as vulnerabilities discovered in popular GitHub Actions like `tj-actions/changed-files <https://www.cve.org/CVERecord?id=CVE-2025-30066>`_, and `reviewdog/action-setup <https://www.cve.org/CVERecord?id=CVE-2025-30154>`_.
+
+We will guide you on how to enable and use this check to enhance the security of your development pipeline.


Suggested change

We will guide you on how to enable and use this check to enhance the security of your development pipeline.

We will demonstrate how to enable and use this check to enhance the security of your development pipeline.

See commit a684508.

benmss · 2025-04-08T02:56:39Z

src/macaron/slsa_analyzer/checks/github_actions_vulnerability_check.py

+                if isinstance(callee, GitHubWorkflowNode) and callee.node_type in [
+                    GitHubWorkflowType.EXTERNAL,
+                    GitHubWorkflowType.REUSABLE,
+                ]:


Suggested change

if isinstance(callee, GitHubWorkflowNode) and callee.node_type in [

GitHubWorkflowType.EXTERNAL,

GitHubWorkflowType.REUSABLE,

]:

if isinstance(callee, GitHubWorkflowNode) and callee.node_type in {

GitHubWorkflowType.EXTERNAL,

GitHubWorkflowType.REUSABLE,

}:

See commit a684508.

benmss · 2025-04-08T02:59:32Z

src/macaron/slsa_analyzer/checks/github_actions_vulnerability_check.py

+                    if not workflow_name:
+                        logger.debug("Workflow %s is not relevant. Skipping...", callee.name)
+                        continue


This will only trigger if callee.name has nothing before the @ symbol it contains. Is that expected in some cases?

It also triggers when there is no @ in the callee.name, e.g., when an internal reusable workflow is called in the same repo. We are not interested in these internal workflows. I will improve the condition to be more explicit.

See commit a684508.

benmss · 2025-04-08T03:16:13Z

src/macaron/slsa_analyzer/checks/github_actions_vulnerability_check.py

+        CheckResultData
+            The result of the check.
+        """
+        result_tables: list[CheckFacts] = []


I suggest moving this further down the function to just before where it's actually used. E.g. for vuln_res in batch_vulns:

See commit a684508.

benmss · 2025-04-08T06:52:20Z

src/macaron/slsa_analyzer/package_registry/osv_dev.py

+        section_name = "osv_dev"
+        if not defaults.has_section(section_name):
+            return []
+        section = defaults[section_name]
+
+        url_netloc = section.get("url_netloc")
+        if not url_netloc:
+            raise APIAccessError(
+                f'The "url_netloc" key is missing in section [{section_name}] of the .ini configuration file.'
+            )
+        url_scheme = section.get("url_scheme", "https")
+        query_endpoint = section.get("querybatch_endpoint")
+        if not query_endpoint:
+            raise APIAccessError(
+                f'The "query_endpoint" key is missing in section [{section_name}] of the .ini configuration file.'
+            )
+        try:
+            url = urllib.parse.urlunsplit(
+                urllib.parse.SplitResult(
+                    scheme=url_scheme,
+                    netloc=url_netloc,
+                    path=query_endpoint,
+                    query="",
+                    fragment="",
+                )
+            )
+        except ValueError as error:
+            raise APIAccessError("Failed to construct the API URL.") from error
+
+        response = send_post_http_raw(url, json_data=query_data, headers=None)
+        res_obj = None
+        if response:
+            try:
+                res_obj = response.json()
+            except requests.exceptions.JSONDecodeError as error:
+                raise APIAccessError(f"Unable to get a valid response from {url}: {error}") from error
+
+        results = res_obj.get("results") if res_obj else None


Most of this code is identical to call_osv_query_api. I think we should be re-using it instead of duplicating it.

See commit a684508.

benmss · 2025-04-08T07:01:35Z

tests/integration/cases/oracle-macaron/test.yaml

 # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/.

 description: |
-  Analyzing with PURL and repository path without dependency resolution.
+  Analyzing the staging branch of the Macaron repo to detect vulnerable GitHub Actions.


This test should be updated to use branch main.

See commit a684508.

benmss · 2025-04-08T23:59:33Z

tests/slsa_analyzer/checks/test_github_actions_vulnerability_check.py

+        provenances=[],
+        build_info_results=InTotoV01Payload(statement=InferredProvenance().payload),
+    )
+    match ci_name:


What is the reason for using a match statement here?

I find this approach tidier than using if-else statements, and it also makes it easier to extend support for additional CI types. But I can switch to if-else if preferred, it’s not a big deal.

See commit a684508.

benmss · 2025-04-09T00:11:43Z

tests/slsa_analyzer/package_registry/test_osv_dev.py

+def test_is_affected_version_invalid_commit() -> None:
+    """Test if the function can handle invalid commits"""
+    with pytest.raises(APIAccessError):
+        OSVDevService.is_version_affected(
+            vuln={}, pkg_name="pkg", pkg_version="invalid_commit", ecosystem="GitHub Actions"
+        )
+
+
+def test_is_affected_version_invalid_response() -> None:
+    """Test if the function can handle empty OSV response."""
+    with pytest.raises(APIAccessError):
+        OSVDevService.is_version_affected(
+            vuln={"vulns": []}, pkg_name="repo/workflow", pkg_version="1.0.0", ecosystem="GitHub Actions"
+        )


To me it seems that both of these functions fail because vuln is not correctly populated. In is_version_affected we have the following block:

affected = json_extract(vuln, ["affected"], list) if not affected: raise APIAccessError(f"Failed to extracted info for {pkg_name}@{pkg_version}.")

Can we be sure that the error is being raised from the expected part of the function?

I will improve the test to make a distinction between these two cases explicitly.

See commit a684508.

tromai · 2025-04-10T03:43:38Z

docs/source/pages/cli_usage/command_analyze.rst

@@ -1,4 +1,4 @@
-.. Copyright (c) 2023 - 2023, Oracle and/or its affiliates. All rights reserved.
+.. Copyright (c) 2023 - 2025, Oracle and/or its affiliates. All rights reserved.


Hmmm, I don't think this file's copyright notice should be updated.

I think I noticed that the copyright year was not updated even though the content is changed in previous PRs. I will remove it from my PR, but we should watch for these updates because we don't automatically change the headers in rst files.

See commit a684508.

tromai · 2025-04-10T03:46:08Z

src/macaron/slsa_analyzer/git_url.py

@@ -907,3 +907,79 @@ def is_empty_repo(git_obj: Git) -> bool:
        return False
    except GitCommandError:
        return True
+
+
+def is_commit_hash(version_str: str) -> bool:


Could we rename this variable to make it more generic? From the doc string, I don't think it must be a version string.

See commit a684508.

src/macaron/slsa_analyzer/package_registry/osv_dev.py

tromai · 2025-04-10T05:05:48Z

src/macaron/slsa_analyzer/package_registry/osv_dev.py

+        if isinstance(results, list):
+            if expected_size:
+                if len(results) != expected_size:
+                    raise APIAccessError(f"Unable to get a valid result from {url}")


nit: can we add the reason why this result is invalid (for example, saying that the results doesn't have the expected number of elements).

See commit a684508.

tromai · 2025-04-10T05:09:08Z

src/macaron/slsa_analyzer/package_registry/osv_dev.py

+
+        results = res_obj.get("results") if res_obj else None
+
+        if isinstance(results, list):


If results is not of type list, should we raise an exception instead ? Or we treat that as an "empty" result 🤔 .

As the dosctring indicates, in this case we return an empty list. But I can change that behavior to raise an exception.

Ah I see. Thanks.
I think it's okay to leave it as is, and perhaps it would be good to add an example of improperly formatted being that results is not of an expected type.

See commit a684508.

src/macaron/slsa_analyzer/package_registry/osv_dev.py

src/macaron/slsa_analyzer/checks/github_actions_vulnerability_check.py

Signed-off-by: behnazh-w <[email protected]>

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 21, 2025

behnazh-w force-pushed the behnaz/add-gh-vuln-gha-check branch 2 times, most recently from 3f5b70c to e056b7c Compare March 26, 2025 00:30

behnazh-w force-pushed the behnaz/add-gh-vuln-gha-check branch 5 times, most recently from 6e429f7 to 9776ec8 Compare April 4, 2025 05:38

behnazh-w marked this pull request as ready for review April 7, 2025 04:48

behnazh-w requested a review from tromai as a code owner April 7, 2025 04:48

benmss self-requested a review April 7, 2025 04:53

benmss reviewed Apr 8, 2025

View reviewed changes

behnazh-w changed the base branch from staging to main April 8, 2025 04:41

benmss reviewed Apr 8, 2025

View reviewed changes

benmss reviewed Apr 9, 2025

View reviewed changes

tromai reviewed Apr 10, 2025

View reviewed changes

src/macaron/slsa_analyzer/package_registry/osv_dev.py Show resolved Hide resolved

tromai reviewed Apr 10, 2025

View reviewed changes

src/macaron/slsa_analyzer/package_registry/osv_dev.py Show resolved Hide resolved

tromai reviewed Apr 10, 2025

View reviewed changes

src/macaron/slsa_analyzer/checks/github_actions_vulnerability_check.py Show resolved Hide resolved

feat: detect vulnerable GitHub Actions

c285cea

Signed-off-by: behnazh-w <[email protected]>

behnazh-w added 3 commits April 22, 2025 16:40

docs: update the ER diagram

01fbe4a

test(integration): change staging to main branch

e13d576

Signed-off-by: behnazh-w <[email protected]>

chore: address PR feedback

a684508

Signed-off-by: behnazh-w <[email protected]>

behnazh-w force-pushed the behnaz/add-gh-vuln-gha-check branch from 9776ec8 to a684508 Compare April 22, 2025 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: detect vulnerable GitHub Actions #1021

feat: detect vulnerable GitHub Actions #1021

behnazh-w commented Mar 21, 2025 •

edited

Loading

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

benmss Apr 8, 2025

behnazh-w Apr 22, 2025

behnazh-w Apr 22, 2025

benmss Apr 9, 2025

behnazh-w Apr 22, 2025

behnazh-w Apr 22, 2025

tromai Apr 10, 2025

behnazh-w Apr 22, 2025 •

edited

Loading

behnazh-w Apr 22, 2025

tromai Apr 10, 2025

behnazh-w Apr 22, 2025

tromai Apr 10, 2025

behnazh-w Apr 22, 2025

tromai Apr 10, 2025

behnazh-w Apr 22, 2025

tromai Apr 22, 2025

behnazh-w Apr 22, 2025

	.. list-table:: Macaron checks descriptions
	.. list-table:: Macaron check descriptions


		This tutorial explains how to use a check in Macaron that detects vulnerable third-party GitHub Actions. This check is important for preventing security issues in your CI/CD pipeline, especially in light of recent incidents, such as vulnerabilities discovered in popular GitHub Actions like `tj-actions/changed-files <https://www.cve.org/CVERecord?id=CVE-2025-30066>`_, and `reviewdog/action-setup <https://www.cve.org/CVERecord?id=CVE-2025-30154>`_.

		We will guide you on how to enable and use this check to enhance the security of your development pipeline.

	We will guide you on how to enable and use this check to enhance the security of your development pipeline.
	We will demonstrate how to enable and use this check to enhance the security of your development pipeline.

		@@ -1,4 +1,4 @@
		.. Copyright (c) 2023 - 2023, Oracle and/or its affiliates. All rights reserved.
		.. Copyright (c) 2023 - 2025, Oracle and/or its affiliates. All rights reserved.


		results = res_obj.get("results") if res_obj else None

		if isinstance(results, list):

feat: detect vulnerable GitHub Actions #1021

Are you sure you want to change the base?

feat: detect vulnerable GitHub Actions #1021

Conversation

behnazh-w commented Mar 21, 2025 • edited Loading

Summary

Changes

Documentation

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behnazh-w Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behnazh-w commented Mar 21, 2025 •

edited

Loading

behnazh-w Apr 22, 2025 •

edited

Loading