Skip to content

Conversation

@jparsai
Copy link
Contributor

@jparsai jparsai commented May 12, 2025

This PR is to fix a path traversal vulnerability CWE-22 where a malicious user could potentially craft a URL with special characters (like ../) to try and access files or directories outside of the intended UI asset directory

Here are logs from scanner:

argo_cd/app/server/server.go:1410:3: taint: The field "r.URL" is a source of untrusted data.
argo_cd/app/server/server.go:1410:3: sink: Calling "uiAssetExists". This call uses "r.URL.Path" for sensitive computation.
argo_cd/app/server/server.go:1387:12: identity: Calling "Trim". This call assigns "filename" to "<return value>".
argo_cd/app/server/server.go:1387:12: sink: Calling "Open". This call uses "Trim(filename, "/")" for sensitive computation. (The interface method resolves to "http.Dir.Open(string)".)
argo_cd/app/server/server.go:1410:3:
# 1408|           }
# 1409|   
# 1410|->         fileRequest := r.URL.Path != "/index.html" && server.uiAssetExists(r.URL.Path)
# 1411|   
# 1412|           // Set X-Frame-Options according to configuration

Signed-off-by: Jayendra Parsai <[email protected]>
@jparsai jparsai requested a review from a team as a code owner May 12, 2025 11:47
@bunnyshell
Copy link

bunnyshell bot commented May 12, 2025

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.00%. Comparing base (610523b) to head (521e850).
⚠️ Report is 470 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #22932      +/-   ##
==========================================
- Coverage   60.03%   60.00%   -0.04%     
==========================================
  Files         344      344              
  Lines       57781    57787       +6     
==========================================
- Hits        34689    34673      -16     
- Misses      20333    20356      +23     
+ Partials     2759     2758       -1     

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

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, security concerns should be reported following these directions: https://github.com/argoproj/argo-cd/blob/master/SECURITY.md#reporting-a-vulnerability


func (server *ArgoCDServer) uiAssetExists(filename string) bool {
f, err := server.staticAssets.Open(strings.Trim(filename, "/"))
f, err := server.staticAssets.Open(filepath.Clean(filename))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server.staticAssets by default only serves an embedded FS, so it shouldn't be vulnerable to traversal.

If the user has specified a --staticassets dir on the API deployment, they expand server.staticAssets to serve on-disk files. By my understanding of os.DirFS can only be escaped via symlinks, not just paths. Either way I'd want to see a PoC of a working exploit.

I think a better way to harden the --staticassets feature would be to use root.FS instead of os.DirFS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I guess I can close this PR then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Let me know if you have feedback on my PR's approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants