Add CVE-2022-26148 Detector Plugin

[just-hms]

Hello,

This PR contains the implementation for the CVE-2022-26148 detector.

Below it is possible to find the necessary information for review:

• PR Testbed: Testbed for CVE-2022-26148: Grafana api_jsonrpc.php Zabbix Credentials Disclosure security-testbeds#114

Thank you.

[v1ktor0t]

Hello @just-hms, thank you for your contribution!

Overall, the plugin is well designed as works as intended. I added some suggestions to clean up the description and some improvements on the supplied unit tests.

Let me know if you have any questions.

[v1ktor0t]

The test is currently performing 4 consecutive requests and verifying that the report contains the scanned target. This presents multiple issues:

1. If any of the mocked requests is not vulnerable, the test will still pass, resulting in a false positive. This can be verified by simply changing one of the mocked requests to:

mockGrafanaService.enqueue(
    new MockResponse()
        .setResponseCode(HttpStatus.OK.code())
        .setBody(secureGrafanaLoginResponse));

2. The test mocks 4 requests in an attempt to verify that all vulnerable endpoints are covered. However it does not verify that the detector actually invoked them. The mocks are generic and will be returned for any URL.

With regards to 1) I'd recommend verifying that the number of instances of the target service in the report is equal to the number of mocked requests. This will ensure that all mocked requests did in fact are vulnerable.

Regarding 2), consider adding a custom Dispatcher such as the one defined here, that will return a request only for the expected endpoints. Then you can verify the number of expected request with:

assertThat(mockWebServer.getRequestCount()).isEqualTo(<EXPECTED_COUNT>);

This applies for both test.

[just-hms]

Replying to 1.

Apologies for not being clear earlier. The paths listed here represent possible locations where Zabbix credentials might be exposed. The plugin should return a positive result if the credentials are exposed on any of these paths.
Still, a better approach than mine could be to implement a matrix test to validate that the vulnerability is detected on each path individually.

Replying to 2.

Agreed, that’s a better approach. I’ll add it once we’ve addressed point 1.

[v1ktor0t]

Hey @just-hms! Apologies for the late reply.

I gave it some thought, I might have a simpler solution. My suggestion is this: what about defining the set of vulnerable paths as a constant that can be used by both the plugin and the test. The test can then mock and execute based on that list. And finally, the verification can just be updated with the second suggestion, i.e. checking if the report contains the expected path.

This would ensure that any future updates of the path are also picked up by the test. If any detection fails, the count check should be sufficient to catch the bug.

Does that make sense? What do you think?

[just-hms]

Hi @v1ktor0t , thanks for your suggestions,

I’ve updated the README as recommended and left a comment on the unit test #581 (comment) , let me know your thoughts.

[ikkisoft]

JFYI - @maoning This was actually commissioned by us. Alessandro hasn't fully onboarded at Doyensec yet, hence we asked him to submit using his personal email. Subsequent PRs will be send from his @doyensec email.