Add immutable action check

[sailikhith-stepsecurity]

No description provided.

[step-security-bot]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

knowledge-base/actions/homebrew/actions/remove-disabled-formulae/action-security.yml

• [High]Use a version control system that prevents merging of code without passing a test phase
  The git patch in the provided unified diff format does not seem to include any tests that ensure the code is functional and correct. All code that gets merged should pass a series of tests before it is merged into the main branch. Implement a continuous integration system or hook in a unit test suite that ensures tests pass before allowing code to be merged into the main branch.
• [Medium]Avoid the use of commented-out code
  The provided git patch includes commented-out code. Commented-out code is not a productive form of version control. In general, it is better to remove code that is not being used as it can lead to confusion and unnecessarily increases the size of a codebase. Remove all commented-out code from the provided git patch.
• [Medium]Use valid branch names
  The provided git patch includes a local branch with the name "disabled-formulae." Branch names should avoid using spaces or other potentially problematic characters. Rename the branch to remove the space.
• [Low]Use clear and descriptive commit messages
  The provided git patch includes a commit message that does not convey much information. Write a clear and descriptive commit message that explains the change being made in the commit.

knowledge-base/actions/homebrew/actions/remove-disabled-packages/action-security.yml

{
"recommendations": [
{
"Severity": "High",
"Recommendation": "Remove unused import of os",
"Description": "The os module is not being used in the file, which means that it is an unused import.",
"Remediation": "Remove the line with the import statement for os."
},
{
"Severity": "Low",
"Recommendation": "Add a newline at the end of the file",
"Description": "The file does not have a newline at the end of the file, which can cause issues with some tools.",
"Remediation": "Add a newline character at the end of the file."
}
]
}

remediation/workflow/metadata/actionmetadata_test.go

• [High]Input Validation
  The function 'doesActionRepoExist' assumes that the input 'filePath' is a valid path. However, there is no validation of 'filePath' input. This may lead to directory traversal attacks by passing a relative path or a filesystem path that could interrupt the normal application execution. Validate the input for 'filePath' thoroughly and provide an error message to the user if the input is not in the correct format. This can be achieved by using a secure coding practice like 'whitelist validation' or a third-party package like 'filepath.Abs.'
• [Low]Error Handling
  The function 'doesActionRepoExist' returns 'false' when an error occurs, but fails to log an error message or provide any additional information, making it difficult to debug errors when they occur. In production, this lack of error-handling could result in silent failures which could persist undetected and cause issues with data integrity or system availability. Provide detailed error messages and logs using the provided logger package to indicate what went wrong and where in the code the error occurred. Use 'log.Error' to log errors in logs.

remediation/workflow/pin/action_image_manifest.go

• [High]Don't Error Hidden [errcheck]
  Errors should be returned or handled without being hidden. Replace the logrus.Error statements with return nil, err statements to return the error instead of just logging it or add handling logic.
• [High]Use a Templating Engine to Construct Database Queries, Keys, and Commands [GOW100]
  Constructing a query using string concatenation is not a recommended way to build queries. When injecting data into a string these values may contain special characters and SQL syntax that causing the query to behave in unexpected ways. Update the code to use a query library like go-template or prepared statements instead of string concatenation.
• [Medium]Avoid Using Global Variables [G101]
  Use of global variables can lead to race conditions and unintended results if not managed properly. It also makes it difficult to reuse and test code. Avoid the use of global variables and instead use dependency injection or other methods for sharing variables between functions and packages.
• [Medium]Avoid nil Channels [G104]
  Attempting to write to a nil channel causes deadlock and can therefore impact system stability. Check that channel has been initialized before use and don't use closed channels.
• [Low]Don't Use defer in Loops [G318]
  Defer statements in loops can lead to resource starvation and slow performance. Ensure that any defer statements in loops are only run once per loop iteration by moving the defer outside of the loop or by using a Func() {} hack to ensure closure variables are declared inside a function scope.
• [Low]Use Synchronized Functions with Mutexes for Shared Resources [G304]
  Without synchronization, multiple threads can simultaneously access the same resources causing race conditions and/or deadlocks. Use a sync.Mutex{} instance for any variables or resources that are shared across multiple functions or threads to ensure data integrity is properly enforced.

remediation/workflow/pin/action_image_manifest_test.go

• [High]Avoid using test functions as package exportables
  Test functions should be internal to the package and not exported as a package symbol. Change the function signature of Test_isImmutableAction to test_isImmutableAction so that it is not exported as a symbol. Then update the references to the function name within the test case.
• [Medium]Handle invalid action input as error
  The handling of an invalid input action is returning false. Return an error instead to ensure calling code can differentiate between valid and invalid input. Within the function IsImmutableAction, handle the invalid scenario and return as an error instead of a boolean. The calling code will then need up update to handle this error.
• [Medium]Handle action existence validation as error
  The handling of an action that does not exist is returning false. Return an error instead to ensure calling code can differentiate between a legitimate action that doesn't exist and an invalid input. Within the function IsImmutableAction, handle the missing-action scenario and return as an error instead of a boolean. The calling code will then need up update to handle this error.

remediation/workflow/pin/pinactions.go

• [High]Avoid executing immutable code
  The current code execution allows execution of immutable code. Disallow execution of immutable code by adding a check against immutable actions.
• [High]Sanitize user input
  The inputYaml parameter is not sanitized, leaving it open to attacks such as command injection. Sanitize the input before passing it into the function by validating and filtering out any invalid characters
• [Medium]Follow naming conventions for functions in Go
  The function name 'PinAction' does not follow the naming conventions for functions in Go. Rename the function to 'pinAction'.
• [Medium]Validate input parameters
  The function does not validate its parameters. Validate the parameters of the function by checking that they are not nil or empty before using them.
• [Low]Avoid using hard-coded values
  The function uses a hard-coded value, making it less maintainable. Use a named constant instead of a hard-coded value for the return value of the function.

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.

[codecov]

Codecov Report

Attention: Patch coverage is 93.61702% with 3 lines in your changes missing coverage. Please review.

Project coverage is 65.54%. Comparing base (d61982f) to head (fe96d6f).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
remediation/workflow/pin/action_image_manifest.go	93.47%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2496      +/-   ##
==========================================
- Coverage   67.34%   65.54%   -1.80%     
==========================================
  Files          16       17       +1     
  Lines        1283     1750     +467     
==========================================
+ Hits          864     1147     +283     
- Misses        332      515     +183     
- Partials       87       88       +1     

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

[sailikhith-stepsecurity]

homebrew/actions/remove-disabled-formulae is replaced by remove-disabled-packages.( REF - Homebrew/actions#567. )
which is causing our test workflow to fail here - https://github.com/step-security/secure-repo/actions/runs/12809184736/job/35713468641.

So, updating our kb.

[varunsh-coder]

if it is an immutable action, we might need to still pin it. e.g. if it is @v1 it should be pinned to the semantic tag that corresponds to v1, e.g. v1.2.3. if it is an immutable action and already in semantic version, then we can ignore it.

[sailikhith-stepsecurity]

As per my understanding from readme here, action can be immutable only with semantic versioning tag .

In mentioned example, If action is with @v1 tag...

1. we won't be able to find v1 tag in the ghcr image tags...so, we return action as not immutable wrt v1 tag
2. Then we go ahead and pin this action as per existing logic using sha.

But, since this has to be pinned with semantic version tag(in case when action is immutable).
we might to need to add a check if action is immutable with semantic-versioning-tag and pin it with semantic-versioning-tag instead of sha.

Please let me know if this aligns with expected behaviour or not. I'll go ahead and make changes.

[step-security-bot]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

remediation/workflow/pin/action_image_manifest.go

• [High]Use JSON decoding to parse OCIManifest instead of Unmarshal and don't pass untrusted data to Unmarshal
  JSON Unmarshalling can lead to arbitrary code execution if untrusted data is passed as input instead use an JSON Decoder which doesn't have that vulnerability. Instead of using Unmarshal, use an JSON Decoder like this:

package main

import (
"encoding/json"
"io"
)
f:=func(rs io.Reader) (u *User, err error) {
u = &User{}
d := json.NewDecoder(rs)
t, err := d.Token()
if err != nil {
return nil, err
}
if t != json.Delim('{') {
return nil, fmt.Errorf("expected JSON object, got %s", t)
}
for d.More() {
var key string
if err = d.Decode(&key); err != nil {
return nil, err
}
switch key {
case "name":
err = d.Decode(&u.Name)
case "age":
err = d.Decode(&u.Age)
default:
err = fmt.Errorf("unexpected key: %s", key)
}
if err != nil {
return nil, fmt.Errorf("error decoding %s: %w", key, err)
}
}
if t, err = d.Token(); err != nil {
return nil, err
} else if t != json.Delim('}') {
return nil, fmt.Errorf("expected '}' after object, got %s", t)
}
return
}

• [High]Use only required data in JSON.Unmarshal function to avoid security issues with unmarshalling untrusted data
  It is a good practice to use only required data during JSON unmarshalling as this function can cause security vulnerability if untrusted data is passed as input. It can lead to arbitrary code execution or Denial of Service (DoS). In the code package, package pin, file named pin.go, change the following functions with these code snippets:

func getOCIManifestForImage(imageRef string) (string, error) {

reference, err := name.ParseReference(imageRef)
if err != nil {
    return "", fmt.Errorf("error parsing reference: %w", err)
}

return getManifest(reference.Context().String())

}

func getManifest(ref string) (string, error) {

var manifest ociManifest
err := callRegistryCloseWithError("GET", "/v2/"+ref+"/manifests/latest", nil, &manifest)

if err != nil {
    return "", err
}

b, err := json.Marshal(manifest)
if err != nil {
    return "", err
}

return string(b), nil

}

• [High]Input data validation should be added in regex pattern to avoid error in parsing of image url
  It is highly recommended to validate input data for regex pattern as it could lead to denial of service and issues while parsing the image URL. In the code package, package pin, file named pin.go, replace the variable tagRegex with this pattern:

var tagRegex = regexp.MustCompile(^v\d+\.\d+\.\d+$)

This ensures that only the expected string pattern is accepted.

• [Medium]Keep secrets/data outside of code repository and in a secure vault such as AWS Secret Manager or Hashicorp Vault
  It is recommended to keep the secrets outside of the code repository to maintain a secure development environment. It is a good practice to use a secure vault to store sensitive information like passwords, certificates, and API keys (e.g. AWS Secret Manager or Hashicorp Vault). Insecure storage or leakage of secrets can lead to major security breaches. In the code package, package pin, file named pin.go, replace local secret keys with environment variables, save them in an environment file, and don't push it to the repository.

• [Medium]Use context package to configure timeouts and deadlines
  It is recommended to configure context package in order to avoid executing a process or blocking a request indefinitely. This can be done by setting appropriate timeouts and deadlines when making network calls to avoid any possible downtime. In the code package, package pin, file named pin.go, for this function, getOCIManifestForImage, add context parameter, and add a timeout of 10 seconds:
  func getOCIManifestForImage(ctx context.Context, imageRef string) (string, error) {

  // Parse the image reference
  ref, err := name.ParseReference(imageRef)
  if err != nil {
  return "", fmt.Errorf("error parsing reference: %w", err)
  }

  // Get the image manifest
  desc, err := remote.Get(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain))
  if err != nil {
  return "", fmt.Errorf("error getting manifest: %w", err)
  }

  return string(desc.Manifest), nil
  }

• [Low]Avoid multiple assignments on the same code line
  It is a good practice to avoid multiple assignments on the same line of code as this can reduce code readability and, in some cases, make the code impossible to compile. In the code package, package pin, file named pin.go, move the action and err variables to new lines for readability.

if err != nil {
// log the error
logrus.WithFields(logrus.Fields{"action": action}).WithError(err).Error("error in getting OCI manifest for image")
return false
}
artifactType, err := getOCIImageArtifactTypeForGhAction(action)
if err != nil {
return false
}

• [Low]Use string formating to avoid concatenating strings
  It is recommended to use string formatting instead of concatenating strings with + as this enhances code readability and ensures that the code is well documented. In the code package, package pin, file named pin.go, replace string concatenation with string fmt.Spritnf while building image :

image := fmt.Sprintf("ghcr.io/%s:%s", parts[0], parts[1])

remediation/workflow/pin/pinactions.go

• [High]Avoid hardcoding of sensitive data
  The code contains a hardcoded value that represents the action name. Hardcoded values can be unintentionally leaked or displayed to unauthorized users. Replace the hardcoded value with a secure method of storage like environment variables or external database tables.
• [Medium]Proper use of naming conventions
  The function 'PinAction' has a name that is not in accordance with the recommended Golang naming conventions. Rename 'PinAction' function to 'PinActionName' or any other relevant name in accordance with Golang naming conventions.
• [Medium]Prevent code duplication
  The function 'PinAction' has duplicate code that checks if the action name is absolute or immutable. Create a separate function called 'isImmutableAction' that has the exact same code to check if action name is immutable, and call this function in 'PinAction'.

knowledge-base/actions/homebrew/actions/remove-disabled-formulae/action-security.yml

• [High]Avoid committing commented out code
  The code commit includes commented out code that is not being used in the program's execution and might cause confusion, increase code maintenance costs, and decrease code readability in the long run. Remove the commented out code
• [Low]Maintain consistent newline at the end of the file
  The last line of the file does not end with a newline character, which can cause issues with certain tools and scripts. Add a newline character at the end of the file

remediation/workflow/metadata/actionmetadata_test.go

• [High]Ensure that the array index accesses is in bounds
  The code splits the file path and accesses index 5 and 6 without checking if the array is big enough. if len(splitOnSlash) >= 7 {owner = splitOnSlash[5]; repo = splitOnSlash[6];} else{//Do something, maybe return an error}
• [Low]Check for errors after splitting the string
  The code splits a string without checking if there's an error. splitOnSlash = strings.Split(filePath, "/"); if err != nil { //handle the error }

remediation/workflow/pin/action_image_manifest_test.go

• [High]Ensure that the server certificate is verified to prevent man-in-the-middle (MITM) attacks
  The server validation is not done on the response of the HTTP request. An attacker can man-in-the-middle (MITM) attacks on web requests to intercept and tamper with data. Verify the server certificate before any request by setting InsecureSkipVerify flag to false under HTTPS transport. Replace calls which accepts Transport parameter such as httptest.NewUnstartedServer with httptest.NewTLSServer which will perform certificate validation by default.
• [High]Avoid using global variables to prevent unintended access and update of variables
  The global variable testFilesDir can be read or modified from anywhere and this increases the code complexity and makes it harder to reason about the code which make it easy to cause bugs. Remove Global variables and replace them by passing dependencies through function parameters.
• [Medium]Use Table-driven testing to simplify and scale testing
  Tests use nested arrays to define their expected input and output which can be challenging to maintain for multiple tests. Replace nested test arrays with table-driven testing where each entry is defined in a table and the same code is executed for each input and expectation.
• [Medium]Following the naming convention for member variables should be followed to ensure code consistency
  The struct contains an unexported variable (starting with lowercase) which is not following the naming convention-go struct fields should have names with disjoint words, and these names should start with a capital letter. Export the variable by starting it with a capital letter.
• [Medium]Do not shadow package names as it reduces the code readability
  The file/package name pin is included at the top level and a variable of the same name is declared in the package main, which may affect code clarity. Rename the variable in main package to use a different name than the package and avoid using package names as variable names.
• [Low]Use a web framework for test servers to simplify and scale testing
  The use of the built-in net/http package instead of a web framework can increase code complexity, reduce productivity, and lead to potential security vulnerabilities. Use a web framework such as Gin, Echo for creating the test server or for routes instead of using the built-in http.HandlerFunc.

testfiles/pinactions/immutableActionResponses/default.json

• [High]Sanitize and Validate Input
  The code is vulnerable to an injection attack at the 'message' variable passed to the 'errors' array. Escape special characters and validate input for known patterns.
• [High]Implement Proper Error Handling
  The code throws a generic error without providing the specific reason for the error. This makes it difficult to identify and fix issues. Provide specific and meaningful error messages to identify the issue and guide the user on how to fix it.
• [Medium]Use a Standard Input Format
  The input message is not in a fixed format, which can lead to confusion and inconsistency in how it is processed. Use a standardized format for input that is enforced by the application.
• [Medium]Implement HTTPS
  The code does not use HTTPS, which can lead to sensitive data being intercepted and compromised. Implement HTTPS to ensure secure communication between the server and browser.
• [Low]Remove Unused Code
  The code includes unused code, which can make the codebase unnecessarily complex and difficult to maintain. Remove the unused code to simplify the codebase.

testfiles/pinactions/immutableActionResponses/ghcr.io_actions_checkout:4.2.2.json

• [High]The schema should be explicitly defined and versioned to ensure backwards compatibility as per Open Containers Initiative (OCI) Image Format Specification
  The schemaVersion field is not defined or set to 1.0.0. This may lead to compatibility issues with future changes to the OCI Image Format Specification. { "schemaVersion": 2, "mediaType": "application/vnd.oci.image.manifest.v1+json" }
• [High]Annotations should follow the Open Containers Image Format Specification recommendation
  The current annotations do not follow the OCI Image Format Specification which may cause compatibility issues. { "annotations": { "org.opencontainers.image.created": "2024-10-23T14:46:13.071Z", "org.opencontainers.image.title": "actions-checkout_4.2.2.tar.gz", "com.github.package.type": "actions_oci_pkg", "com.github.package.version": "4.2.2", "com.github.source.repo.id": "197814629", "com.github.source.repo.owner.id": "44036562", "com.github.source.commit": "11bd71901bbe5b1630ceea73d27597364c9af683" }}
• [Medium]All fields defined by the Open Container Image specification should be included
  The mediaType and artifactType fields are not included. This information is important for defining the image's purpose and intended use. { "mediaType": "application/vnd.oci.image.manifest.v1+json", "artifactType": "application/vnd.github.actions.package.v1+json", "config": { "mediaType": "application/vnd.oci.empty.v1+json", "size": 2, "digest": "sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a" }}
• [Medium]All image layers should have the same digest algorithm
  The manifest includes layers with different digest algorithms which can cause compatibility issues. Rebuild image layers using the same digest algorithm.
• [Medium]Image layer digests should be verifiable
  It is not possible to verify the authenticity of the image layer digests. Rebuild the image layers with securely verifiable digests.
• [Low]Image layer sizes should be kept as small as possible
  The compressed size of the image layer is relatively large and could be optimized. Optimize the size of the image layers.

testfiles/pinactions/immutableActionResponses/ghcr.io_step-security_wait-for-secrets:1.2.0.json

• [High]Verify dependencies
  The provided code does not include a way of declaring or verifying dependencies. This may result in a runtime failure due to missing libraries or unsuitable versions. Include a manifest file that specifies the dependencies as well as their versions or ranges. Use a package manager or a bundler to ensure that the dependencies are present and compatible.
• [High]Audit dependencies
  The provided code includes a dependency on an unknown package. This package might contain vulnerabilities or malicious code that can compromise the security of the software. Audit the package's code and dependencies using a reliable tool. If possible, use a version of the package that has been audited and verified to be secure.
• [High]Limit image privileges
  The provided code creates a container image without setting the user namespace, which allows the image to run with root privileges. This increases the risk of security vulnerabilities, especially if the image is used in a production environment. Create the image using a non-root user or a user with limited privileges. Set the user namespace to prevent the image from running with root privileges.
• [Medium]Reduce image size
  The provided code includes unnecessary layers in the container image, which increases the size of the image and might affect its performance. Optimize the Dockerfile to remove any unnecessary run, copy, or add instructions. Combine multiple instructions into a single RUN instruction to reduce the number of layers in the image. Use multi-stage builds to create smaller images.
• [Medium]Use safer base image
  The provided code uses a base image that might contain vulnerabilities or security issues. Using a more secure base image might be recommended. Use a more secure base image such as one that is regularly updated, has secure defaults or is recommended by security auditors.
• [Medium]Avoid empty filesystems
  The provided code creates a layer with an empty filesystem for an OCI config file. An empty filesystem increases the attack surface and poses security risks. Instead of creating an empty filesystem, use an appropriate tool to set the required configuration values for the OCI config file. Avoid setting unnecessary or insecure values.
• [Low]Use specific digests
  The provided code uses tags to refer to container images instead of digests. This increases the risk that the wrong image will be used, potentially leading to security vulnerabilities in production. Use digests instead of tags to refer to container images. Digets specify the exact content of the image and are less likely to be changed unintentionally.
• [Low]Remove unused annotation
  The provided code includes an annotation that is unused and might cause confusion if it is expected to contain useful information. Remove the unused annotation or replace it with useful metadata about the image.
• [Low]Simplify image reference
  The provided code uses a complex media type to refer to a container image. This might affect compatibility or introduce errors if the media type is not supported by the container runtime. Use a simpler and widely supported media type to refer to the container image. For example, use "application/vnd.docker.distribution.manifest.v2+json" to refer to a Docker image manifest.
• [Low]Include newline at EOF
  The provided code does not include a newline at the end of the file. Some tools or scripts might fail or behave unexpectedly as a result. Add a newline at the end of the file to ensure compatibility with tools and scripts.

knowledge-base/actions/homebrew/actions/remove-disabled-packages/action-security.yml

• [High]Unused credentials should be removed
  The GITHUB_TOKEN is not used in the code and can be safely removed to prevent accidental exposure. Remove the GITHUB_TOKEN variable from the code since it is not being used.
• [Medium]Code should conform to POSIX standards
  The absence of a newline at the end of the file may cause issues with POSIX compliant tools. Add a newline to the end of the file to conform with POSIX standards.

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.