Add validation to helm values file path to prevent potential dirtrav vulnerability #3726
Conversation
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. resolve symbolic linkpipecd/pkg/app/piped/cloudprovider/kubernetes/helm.go Lines 236 to 239 in a6d49a3 This was created by todo plugin since "TODO:" was found in a6d49a3 when #3726 was merged. cc: @Szarny. |
…3719-helm-value-validation
Co-authored-by: Khanh Tran <[email protected]>
| func verifyHelmValueFilePath(appDir, valueFilePath string) error { | ||
| url, err := url.Parse(valueFilePath) | ||
| if err == nil && url.Scheme != "" { | ||
| for _, s := range allowedURLSchemes { |
There was a problem hiding this comment.
was it a miss or did we really think about allowing the remote valueFiles? I feel a bit unsafe to allow remote file value, wdyt?
There was a problem hiding this comment.
Since our concern here is the unauthorized rendering of unrelated sensitive information on the host on which Piped is running, remote files are outside the scope of this PR's concern, I think.
However, there may be such a case: SSRF, i.e., when a server accessible only from a host running Piped holds the confidential values file and attackers tries to render it (e.g. valueFile: https://private-server-that-can-be-accessible-only-from-piped-host.com/values.yaml) illegally.
There was a problem hiding this comment.
To prevent this, it may be necessary to specify an allowlist of URLs from which the values file can be read.
However, I think that allowing reachable HTTP(S) endpoints is an acceptable risk under the current circumstances.
There was a problem hiding this comment.
This concept follows the advisory of GHSA-63qx-x74g-jcr7.
There was a problem hiding this comment.
I understand that this is out of context on this PR 👍 My concern is currently, do we allow the remote valueFiles that way? and if it's yes, then the docs we updated may need to be updated as well.
Only files stored under the application directory are allowed.
There was a problem hiding this comment.
okay, lets go with that option 👍
There was a problem hiding this comment.
@Szarny
Sorry, I didn't know that ability.
Please let me confirm, will Helm automatically fetches the remote value files to apply when it was specified with HTTP scheme?
There was a problem hiding this comment.
@nghialv Yes, please see this pr: helm/helm#2769
There was a problem hiding this comment.
Thanks for teaching me. Didn't know about that before. 👍
Co-authored-by: Khanh Tran <[email protected]>
| } | ||
|
|
||
| // absAppDir is a path where ".pipecd.yaml" is located. | ||
| absAppDir, err := filepath.Abs(appDir) |
There was a problem hiding this comment.
appDir passed to this function is an absolute path so I think we don't need this.
There was a problem hiding this comment.
Removed absAppDir with this commit: 3ac6d69
There was a problem hiding this comment.
@khanhtc1202 Could you confirm the above changes? (it needs to approve again before merging)
What this PR does / why we need it:
To prevent potential directory traversal vuln, a mechanism has been added in this PR to restrict the path that can be specified as the Helm values file to the directory inside which the application configuration file exists.
For example, if an attacker specifies the values file path is
../../../../../../path/to/secret.yamlto attempt to illegally render the manifest of an application containing unrelated confidential material that stores on the host where Piped is running will fail because of this mechanism.Which issue(s) this PR fixes:
Fixes #3719
Does this PR introduce a user-facing change?: