CVE-2022-29165 Argo CD Auth Bypass, publicly exposed UI with admin access #452
Conversation
|
@maoning I found a way to execute RCE, it needs to deploy payload on an arbitrary path on any Git server like Github! I tried a simple HTTP and HTTPS server to serve the git repo but it doesn't work. |
|
@JamesFoxxx I'm testing out github payload hosting with this other plugin: #449 You can also add your payload in the same /payloads directory, and open a separate PR for it, so that I can merge it in first. |
|
#467 for the payload (for tracking purpose) |
|
@maoning I'll improve this PR to check with a RCE callback instead of simple response checking this week. |
|
@maoning please merge these PRs first: PR is ready for review :) the test cases are not comprehensive yet, I'll add more test cases, please review the plugin first and till then I will find time to add more test cases. |
|
apologize for the delay, it was a hard journey for me, I had to analyze a CVE with no public PoC, plus I had to find a way to disable authentication. after that, I upgraded the plugin to get an OOB confirmation. (I'll be happy if you can consider a bonus for this submission because I'm comparing this to most of the other bounty submissions) |
There was a problem hiding this comment.
Hi @JamesFoxxx, thank you for your contribution!
I reviewed your plugin with the latest changes, (including google/security-testbeds#43 and google/security-testbeds#42) and I confirm that it works, although I left some comments on a few things that can be improved.
Please also remember to run the google-java-format tool before submitting the changes to ensure that the code is properly formatted.
Moreover, I also left some comments on the testbed PRs:
- Simple Update to argo-cd setup reference security-testbeds#43 (review)
- setup guide for argo-cd exposed admin ui security-testbeds#42 (review)
To recap, the following things need to be done before this submission can be be merged:
- Address the comments in this review
- Address the comments in the testbed PRs
- The testbed PRs need to be merged
- New Payload for testing with RCE in argo-cd plugin #472 needs to be merged and the URL updated in the code
Feel free to reach out.
~ Savio (Doyensec)
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetector.java
Outdated
Show resolved
Hide resolved
| // This url might be changed in the future, so I make it easy to change | ||
| private final String PAYLOAD_GIT_URL = "https://github.com/JamesFoxxx/argo-cd-app"; | ||
| // The Path to the directory of payload on the git repository | ||
| private final String PAYLOAD_GIT_PATH = "payloads/jsonnet-guestbook-tla"; |
There was a problem hiding this comment.
Update the payload URL and path once #472 is merged. (Putting this here as a reminder)
There was a problem hiding this comment.
could you please approve this PR too so it can be merged faster I guess.
...st/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCDDetectorTest.java
Outdated
Show resolved
Hide resolved
…be exist so then we can upsert the new duplicate app, thanks to @lokiuox
…istent with software namings
fix naming with 'argo cd instance' and convert to 'argo cd API server' add initial support for response matching detection method
…against `api/v1/certificates` endpoint which needs authenticaiton
|
@lokiuox I wanted to apply one more update, I changed the phrase "argo cd instances" to "argo cd API server" in the namings and descriptions because the documentation explicitly says "Access The Argo CD API Server" in here. Also, the instances in argo cd should be related to application instances, so it was some kind of wrong naming that I did before. |
There was a problem hiding this comment.
It looks like the plugin is working correctly!
I left you some comments about a few minor things to fix, but I'd say the plugin is almost ready to be merged.
Please also fix the following things:
- Align all file names to one naming scheme (the one you prefer):
Cve202017526DetectorConfigs.javastill has the name of the plugin it was copied fromExposedArgoCdApiDetectorBootstrapModule.javafollows the new naming withApiin the nameExposedArgoCdDetector.javaandExposedArgoCdDetectorTest.javafollow the old naming
- Also left you one more comment on the setup README for the vulnerable version: Simple Update to argo-cd setup reference security-testbeds#43 (review)
.../java/com/google/tsunami/plugins/detectors/exposedui/argocd/Cve202017526DetectorConfigs.java
Outdated
Show resolved
Hide resolved
...ogle/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdApiDetectorBootstrapModule.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdDetector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdDetector.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I commented on a last-minute improvement that can be added, everything else is good.
Moreover, before I can give my final approval, the testbed and payload PRs have to be merged:
- New Payload for testing with RCE in argo-cd plugin #472
- setup guide for argo-cd exposed admin ui security-testbeds#42
- Simple Update to argo-cd setup reference security-testbeds#43
Hey @maoning, can you please merge the PRs listed above? This is the last step needed before I can complete the review of this plugin
...ain/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdApiDetector.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/tsunami/plugins/detectors/exposedui/argocd/ExposedArgoCdApiDetector.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hey @JamesFoxxx, the exception text is still printed due to withCause(e) being still present.
I also did some more tests, and I realized that the string and the exception are printed even when scanning completely unrelated applications, which doesn't really make sense. I checked the implementations of other plugins, and I noticed that all of them just return false in case the target isn't vulnerable, without logging anything, so I suggest you to do the same here and just return without printing anything. I also commented on two more logged strings that I think should be deleted for the same reason.
Please also remember to run the google-java-format before submitting the changes. :)
| .get("name") | ||
| .getAsString(); | ||
| } catch (IllegalStateException | NullPointerException | JsonParseException e) { | ||
| logger.atWarning().withCause(e).log("The application does not appear to be vulnerable"); |
| .get("server") | ||
| .getAsString(); | ||
| } catch (IllegalStateException | NullPointerException | JsonParseException e) { | ||
| logger.atWarning().withCause(e).log("The application does not appear to be vulnerable"); |
| String targetUrl = NetworkServiceUtils.buildWebApplicationRootUrl(networkService); | ||
|
|
||
| String targetUri = targetUrl + "api/v1/certificates"; | ||
| logger.atInfo().log("targetUri is %s", targetUri); |
There was a problem hiding this comment.
Consider removing this line, since sent HTTP requests are already logged by default
| // This is a blocking call. | ||
| HttpResponse response = | ||
| httpClient.send(get(targetUri).setHeaders(baseHeaders.build()).build(), networkService); | ||
| logger.atInfo().log("the response is %s", response); |
There was a problem hiding this comment.
Consider removing this line, as it's too verbose and the status code of HTTP responses is already logged by default
|
@lokiuox I'm sorry about this logging issue, I shoud've tested your last review before pushing it here. |
Hi, it is related to my PRP #431.