-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auth Bypass to RCE in Airflow #456
Auth Bypass to RCE in Airflow #456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @am0o0,
Thank you for your contribution! I did a review of this plugin, and I've noticed that it is missing the require Java formatting. I highly recommend to run Google's Java formatter in order to be compliant with the standards.
You can do it by following the guidelines in the following repo https://github.com/google/google-java-format
Also, you can find my comments on your contribution below.
Feel free to reach out.
~ Leonardo (Doyensec)
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Show resolved
Hide resolved
..._17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorModule.java
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
fix wildcard imports fix recommendation and title of detection report set a constant for session cookie payload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @am0o0,
Thank you for your changes. I'm noticing that the package
statements are still missing a space from the line above. I've also found some minor stylistic issues to address, you can find them below.
~ Leonardo (Doyensec)
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Show resolved
Hide resolved
@am0o0 Please update your PR according to the feedback, so that we can merge your PR in asap. |
…ase for switch statement
Hi, @leonardo-doyensec, apologize for the delay. |
Hi @am0o0 , ~ Leonardo (Doyensec) |
@leonardo-doyensec I fixed the issue. |
LGTM - Approved Reviewer: Leonardo, Doyensec Plugin: Auth Bypass Lead to RCE in Apache Airflow |
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Show resolved
Hide resolved
.build(), | ||
networkService); | ||
|
||
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(25)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make the time here injected via guice? So that the unit test can inject a different value to make the test run faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a lot to work with Guice but I couldn't succeed :(
If necessary please confirm it and I can put my time into learning Guice as a first step :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you choose the value of 25 here? At least for now, let's switch to 20 if there is no constraint to it, that allows the test to pass. I will give a quick go at having the guice binding.
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Show resolved
Hide resolved
@am0o0 Could you check if the vulnerable configuration of the service recorded at https://github.com/google/security-testbeds/blob/6022938f728d5114f4b9c1d55bb498872018bd03/apache/airflow/CVE-2020-17526/README.md is still working as intended. I tried to do the manual verification and run the plugin, neither works. |
@maoning as this PR took one and half months to be reviewed there is a benefit for this :) the source code of the burp suite plugin: if you are interested as a bounty PRP I can wrap all of these into very simple and smaller Java classes because it is really helpful. After all, we have submissions that need to sign a flask session key with a constant secret key. you can look at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some additional comments. This PR is almost ready though. Thank you for your contribution!
import com.google.common.primitives.Bytes; | ||
import java.security.InvalidKeyException; | ||
import java.security.NoSuchAlgorithmException; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the explicit list of import, do not use wildcards
.build(), | ||
networkService); | ||
|
||
Uninterruptibles.sleepUninterruptibly(Duration.ofSeconds(25)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you choose the value of 25 here? At least for now, let's switch to 20 if there is no constraint to it, that allows the test to pass. I will give a quick go at having the guice binding.
The 10 seconds was the time to get an out-of-band callback for a fresh server, so I thought it should be more for an airflow application with multiple deployments. |
Ack, thank you! I will give a try at the guice change when I have a bit of time (probably early next week) and come back to you. Otherwise we will just reduce the value so that unit tests pass. |
Hi @am0o0, I found a plugin that you can use to implement the Guice injection. See the phpunit detector Here are the required steps:
Let me know if you encounter any issue in the process. Cheers, |
@tooryx thank you a lot for helping me on this! |
...e_2020_17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526Detector.java
Outdated
Show resolved
Hide resolved
...17526/src/main/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorConfigs.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
...20_17526/src/test/java/com/google/tsunami/plugins/cve202017526/Cve202017526DetectorTest.java
Outdated
Show resolved
Hide resolved
Thank you for your work on this @am0o0. I asked a second member of the team to take a quick look. I expect this should be merged before the end of the week. Cheers, |
Hi @am0o0, Your PR has been merged. This usually means a reward will be granted. Google will start the internal QC process and the reward amount will be determined based on the quality of the detector report. Please be patient and allow up to a week for the QC process to finish. You'll be notified once the decision is made. Thanks! |
Hi, it is according to this PRP issue #428