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.

Sign up for GitHub

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

Jump to bottom

Triton Inference Server Rce Detector #451

Merged

tooryx merged 18 commits into google:master from secureness:triton-all-version

May 20, 2024

Contributor

secureness commented Apr 4, 2024 •

edited

secureness added 2 commits

April 4, 2024 14:00

v1

959e027


          chore

a0fece5

leonardo-doyensec self-assigned this

leonardo-doyensec reviewed

View reviewed changes

Collaborator

leonardo-doyensec left a comment •

edited

Hi @secureness,
Thank you for your contribution! I did a review of your plugin, and i've found that is not properly working. Below you can find a list of things to address, some of them are minor stylistic issues but there are some important things to change (for example the issue at line 168).

Feel free to reach out.

~ Leonardo (Doyensec)

...om/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceDetectorBootstrapModule.java Outdated

+               * See the License for the specific language governing permissions and
+               * limitations under the License.
+               */
+              package com.google.tsunami.plugins.detectors.rce;

Collaborator

leonardo-doyensec Apr 12, 2024

Please add a space from the line above

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated Show resolved Hide resolved

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

+              import com.google.common.collect.ImmutableList;
+              import com.google.common.flogger.GoogleLogger;
+              import com.google.common.util.concurrent.Uninterruptibles;
+              import com.google.gson.*;

Collaborator

leonardo-doyensec Apr 12, 2024 •

edited

Avoid using wildcard imports. Import only needed packages

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

+              import com.google.tsunami.plugin.annotations.PluginInfo;
+              import com.google.tsunami.plugin.payload.Payload;
+              import com.google.tsunami.plugin.payload.PayloadGenerator;
+              import com.google.tsunami.proto.*;

Collaborator

leonardo-doyensec Apr 12, 2024 •

edited

Avoid using wildcard imports. Import only needed packages

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

+                    if (modelNames.bodyString().isEmpty()) {
+                      return false;
+                    }
+                    JsonArray modelNamesJO =

Collaborator

leonardo-doyensec Apr 12, 2024

Methods and non-static variables should be named in lowerCamelCase, with acronyms treated as words. Please consider changing variable name to modelNamesJo

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

+                    }
+                    JsonArray modelNamesJO =
+                        JsonParser.parseString(modelNames.bodyString().get()).getAsJsonArray();
+                    if (modelNamesJO.isEmpty()) {

Collaborator

leonardo-doyensec Apr 12, 2024

This statement creates some problem during the run. Running on my side it will result in a crash during the Tsunami scan. Please fix this by replacing with modelNamesJO.isJsonNull()

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

+                            .setRequestBody(
+                                ByteString.copyFromUtf8(
+                                    String.format(
+                                        "{\"parameters\":{\"config\" : \"{}\", \"file:config.pbtxt\" :\"%s\" }}",

Collaborator

leonardo-doyensec Apr 12, 2024

Please consider declaring this as a constant

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

+                            .setRequestBody(
+                                ByteString.copyFromUtf8(
+                                    String.format(
+                                        "{\"parameters\":{\"config\" : \"{}\", \"file:1/model.py\" : \"%s\" }}",

Collaborator

leonardo-doyensec Apr 12, 2024

Please consider declaring this as a constant

...main/java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetector.java Outdated

Comment on lines 142 to 144

+                  if (!payloadGenerator.isCallbackServerEnabled()) {
+                    return false;
+                  }

Collaborator

leonardo-doyensec Apr 12, 2024

The isCallbackServerEnabled() method return false when all config fields are empty so that improper config (e.g., missing certain fields) can be exposed.
In case where the correct config file is used, but if the callback server is not run, the plugin will crash.

Consider changing the business logic in order to catch this case

.../java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetectorTest.java Outdated

+               * See the License for the specific language governing permissions and
+               * limitations under the License.
+               */
+              package com.google.tsunami.plugins.detectors.rce;

Collaborator

leonardo-doyensec Apr 12, 2024

Please add a space from the line above

Contributor Author

secureness Apr 12, 2024

Hi, I didn't understand about this part of the review, unfortunately.

Collaborator

leonardo-doyensec Apr 15, 2024

This should be changed in order to be compliant with Google's Java Format. You can find more information here https://google.github.io/styleguide/javaguide.html#s3-source-file-structure

Collaborator

leonardo-doyensec commented Apr 12, 2024 •

edited

I've also noticed that the issue that this PR is referring to is incorrect. I think this plugin is linked to #433


          first review round

86fc686

Contributor Author

secureness commented Apr 12, 2024

I've also noticed that the issue that this PR is referring to is incorrect. I think this plugin is linked to #433

Yes you are right, sorry about this :))


          better callback control flow, I found out it from cve202348022 detector

1a91386

Collaborator

leonardo-doyensec commented Apr 15, 2024

Hi @secureness,
I'm noticing that after your changes the plugin still not working properly. I think that the root cause is the modelNamesJo.isEmpty() method. Please remove it.

~ Leonardo (Doyensec)

Collaborator

maoning commented Apr 25, 2024

@secureness could you check if the proposed change works? I would like to merge in this PR asap to unblock you from picking up the hive weak cred plugin.


          remove modelNamesJo.isEmpty()

b810477

Contributor Author

secureness commented Apr 27, 2024

@maoning @leonardo-doyensec I'm sorry if it took too long to fix the review suggestion.

Collaborator

leonardo-doyensec commented May 7, 2024

Hi @secureness ,
thank you for your changes. However after the last commits, the plugin no longer builds.
This is an issue related to gradlew. Please upgrade it to version 7. You can take a look at the solution adopted here 251cdd2

~ Leonardo (Doyensec)

secureness and others added 2 commits

May 7, 2024 13:46


          Merge branch 'google:master' into triton-all-version

5011cac


          gradle 7

9e824f8

Contributor Author

secureness commented May 7, 2024

@leonardo-doyensec thanks for the help. I just wanted to let you know that it is done now.

leonardo-doyensec reviewed

View reviewed changes

Collaborator

leonardo-doyensec left a comment

Hi @secureness ,
thank you for your changes. I've added some comment on the code. I'm also noticing that when the plugin is running, and the callback server receives the request (so that the vulnerability is confirmed) the amount of vulnerability detected is not updated. This means that in Tsunami's final log the vulnerability count is zero.
Can you please fix also that?

~ Leonardo (Doyensec)

.../java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetectorTest.java Outdated

+              package com.google.tsunami.plugins.detectors.rce;
+              import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
+              import static com.google.tsunami.common.data.NetworkEndpointUtils.*;

Collaborator

leonardo-doyensec May 7, 2024

Please, import only needed packages and remove star imports.

.../java/com/google/tsunami/plugins/detectors/rce/TritonInferenceServerRceVulnDetectorTest.java Outdated

Comment on lines 102 to 109

+                public void detect_whenVulnerable_returnsVulnerability() throws IOException {
+                  mockWebServer.enqueue(
+                      new MockResponse().setResponseCode(200).setBody("[{\"name\":\"metasploit\"}]"));
+                  mockWebServer.enqueue(new MockResponse().setResponseCode(200));
+                  mockWebServer.enqueue(new MockResponse().setResponseCode(200));
+                  mockWebServer.enqueue(new MockResponse().setResponseCode(200));
+                  mockWebServer.enqueue(new MockResponse().setResponseCode(200));
+                  mockCallbackServer.enqueue(PayloadTestHelper.generateMockSuccessfulCallbackResponse());

Collaborator

leonardo-doyensec May 7, 2024

Ideally would be better to implement more reliable tests. Keep in mind that tests should mimic the vulnerable service as close as possible.

secureness added 3 commits

May 8, 2024 15:34


          test upgrade

cddb281


          remove unused imports


          general improvement

a4e0990

Contributor Author

secureness commented May 8, 2024

@leonardo-doyensec I made some changes according to the official Google(the google/ directory) tsunami plugins. I have the # of detected vulnerability: 1. message after the scan results.
Also, I updated the testbed google/security-testbeds#51.


          fix the callback server message:)

e897bdf

Collaborator

leonardo-doyensec commented May 9, 2024 •

edited

LGTM - Approved
@maoning you can merge it. We also need to merge the security testbed google/security-testbeds#51

Reviewer: Leonardo, Doyensec

Plugin: Triton Inference Server Model Overwrite lead to RCE
Feedback: The overall quality is below standard. The plugin was not working at first and several interactions were required to make it work properly. Furthermore, some elements of formatting were missing, which further hindered the review of the plugin. The security testbed was easy to deploy
Drawbacks: None

vsutedjo self-requested a review

May 17, 2024 09:44

vsutedjo self-assigned this

vsutedjo added the Contributor main label

vsutedjo added 4 commits

May 17, 2024 15:23


          Update TritonInferenceServerRceDetectorBootstrapModule.java

610d7a5


          Update TritonInferenceServerRceVulnDetector.java

c2c28b9


          Update TritonInferenceServerRceVulnDetectorTest.java


          Update TritonInferenceServerRceVulnDetector.java

d061471

copybara-service bot pushed a commit that referenced this pull request


          Copybara import of the project:

dc0e552

--
959e027 by secureness <nosecureness@gmail.com>:

v1

--
a0fece5 by secureness <nosecureness@gmail.com>:

chore

--
86fc686 by secureness <nosecureness@gmail.com>:

first review round

--
1a91386 by secureness <nosecureness@gmail.com>:

better callback control flow, I found out it from cve202348022 detector

--
b810477 by secureness <nosecureness@gmail.com>:

remove modelNamesJo.isEmpty()

--
cddb281 by secureness <nosecureness@gmail.com>:

test upgrade

--
0483886 by secureness <nosecureness@gmail.com>:

remove unused imports

--
a4e0990 by secureness <nosecureness@gmail.com>:

general improvement

--
e897bdf by secureness <nosecureness@gmail.com>:

fix the callback server message:)

--
610d7a5 by Viviana Sutedjo <37225049+vsutedjo@users.noreply.github.com>:

Update TritonInferenceServerRceDetectorBootstrapModule.java
--
c2c28b9 by Viviana Sutedjo <37225049+vsutedjo@users.noreply.github.com>:

Update TritonInferenceServerRceVulnDetector.java
--
3666842 by Viviana Sutedjo <37225049+vsutedjo@users.noreply.github.com>:

Update TritonInferenceServerRceVulnDetectorTest.java
--
d061471 by Viviana Sutedjo <37225049+vsutedjo@users.noreply.github.com>:

Update TritonInferenceServerRceVulnDetector.java

COPYBARA_INTEGRATE_REVIEW=#451 from secureness:triton-all-version d061471
PiperOrigin-RevId: 634763499
Change-Id: I949987a9081ddb196d4bd867c138c6d91a51a3ef


          Merge branch 'master' into triton-all-version

717c395

Member

tooryx commented May 20, 2024

Hi @secureness,

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!

tooryx added 2 commits

May 20, 2024 14:05


          Merge branch 'master' into triton-all-version

18cd9a2


          Update gradlew

a3deab2

tooryx merged commit 40430c4 into google:master

1 check passed

lanced00m mentioned this pull request

Slurm Exposed REST API Detector #496

Open

Contributor Author

secureness commented Jun 7, 2024

@tooryx After 27 days I didn't get any response yet. Is there anything wrong with this submission?

Member

tooryx commented Jun 10, 2024

Hi @secureness,

I will check with the team and get back to you this week.

Cheers,
~tooryx

Member

tooryx commented Jun 10, 2024

You will receive an update on the tracking bug soon. Sorry for the delay!

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