Apache Axis2 weak credential tester

[GiuseppePorcu]

Hi there,
This PR contains the implementation of the weak credential tester for Apache Axis2 Administration Panel.
As described within the Issue, authenticated access to the Administration Panel allows to upload new services thus leading to RCE.

Below it is possible to find the necessary information for review:
Issue: #569
PR Testbed: google/security-testbeds#112
Thank you.

[giacomo-doyensec]

Hello @GiuseppePorcu, thanks for your contribution! The overall quality of the detector is good, I just left some comments and suggestions to improve style and readability.

[giacomo-doyensec]

This function can be simplified maintaining the same logic and a good readability

Suggested change

	boolean isWebService = NetworkServiceUtils.isWebService(networkService);
	if (!isWebService) {
	return false;
	}
	if (!NetworkServiceUtils.isWebService(networkService)) {
	return false;
	}

[giacomo-doyensec]

Suggested change

	boolean canAcceptByCustomFingerprint = false;
	String url = NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + "axis2/";
	try {
	logger.atInfo().log("probing Axis2 Home Page - custom fingerprint phase");
	HttpResponse response = httpClient.send(get(url).withEmptyHeaders().build());
	canAcceptByCustomFingerprint =
	response.status().isSuccess()
	&& response
	.bodyString()
	.map(Axis2CredentialTester::bodyContainsAxis2Elements)
	.orElse(false);
	} catch (Exception e) {
	logger.atWarning().withCause(e).log("Unable to query '%s'.", url);
	return false;
	}
	return canAcceptByCustomFingerprint;
	String url = NetworkServiceUtils.buildWebApplicationRootUrl(networkService) + "axis2/";
	try {
	logger.atInfo().log("probing Axis2 Home Page - custom fingerprint phase");
	HttpResponse response = httpClient.send(get(url).withEmptyHeaders().build());
	return response.status().isSuccess()
	&& response
	.bodyString()
	.map(Axis2CredentialTester::bodyContainsAxis2Elements)
	.orElse(false);
	} catch (Exception e) {
	logger.atWarning().withCause(e).log("Unable to query '%s'.", url);
	return false;
	}

[giacomo-doyensec]

The else clause can be removed, and a single return statement is sufficient if the result of the check is stored in a variable.

Suggested change

	if (Ascii.toLowerCase(title).contains(AXIS_PAGE_TITLE)) {
	logger.atInfo().log("Found Axis2 Home Page (AXIS_PAGE_TITLE string present in the page)");
	return true;
	} else {
	return false;
	}
	boolean containsAxis2Elements = Ascii.toLowerCase(title).contains(AXIS_PAGE_TITLE);
	if (containsAxis2Elements) {
	logger.atInfo().log("Found Axis2 Home Page (AXIS_PAGE_TITLE string present in the page)");
	}
	return containsAxis2Elements;

[giacomo-doyensec]

As per Google Java Style Guide - 4.1.1:

Braces are used with if [...] statements, even when the body is empty or contains only a single statement.

Suggested change

	if (isAxis2Accessible(networkService, defaultUser)) return ImmutableList.of(defaultUser);
	if (isAxis2Accessible(networkService, defaultUser)) {
	return ImmutableList.of(defaultUser);
	}

[giacomo-doyensec]

This comment can be confusing for the reader, please restructure the sentence in a more clearer way.

Suggested change

	/**
	* No need for detect_weakCredentialsExist_returnsAllWeakCredentials since Axis2 only supports a
	* single administrator user
	*/
	/**
	* A separate test for detecting multiple weak credentials is unnecessary because
	* Axis2 only supports a single administrator user. Therefore, this test
	* (`detect_weakCredentialsExists_returnsWeakCredentials`) effectively covers the
	* intended behavior.
	*/

[giacomo-doyensec]

I just realized I didn't include this in the review while transposing my notes. Is there a specific reason why the default credentials aren’t in the appropriate proto file? If possible, please move them to:

google/detectors/credentials/generic_weak_credential_detector/src/main/resources/detectors/credentials/genericweakcredentialdetector/data/service_default_credentials.textproto.

Thanks!

[GiuseppePorcu]

Hi @giacomo-doyensec thanks for your feedback, I'll resolve them as suggested!

Default credentials of Axis2 have been inserted within the class since nmap identifies the service as "http", so in order to not test such credentials against each http service we decided to add the credentials inside the detector.

Following I've inserted a screenshot of nmap discovery in order to show the issue:

Do you prefer to add the credential in the proto file or we can leave it here and add a comment in order to explain why the default credentials are inserted inside the detector instead of the proto file?