HyperSQL weak credential tester #582
Conversation
There was a problem hiding this comment.
Hello @GiuseppePorcu, thanks for your submission!
I found multiple issues that need to be addressed before proceeding with approval. I report them as follows:
-
Missing optimization of credential testing against the service: HyperSQL returns well-known error codes on exceptions, which can be leveraged to avoid unnecessary credential attempts, saving both time and network resources. I've provided suggestions for this optimization in this review (note that all suggestions I provided must be taken as recommendations. Please feel free to improve the code or adopt an alternative solution that you believe might be more optimal).
-
Missing proper handling of non-existent DB error: The DB name,
testdb, used for the connection is hardcoded, as no default database is automatically created during service creation. The code does not handle the case when this DB does not exist. Error-458: database alias does not existis returned when the name is not associated with anything. Please add logic to either find other potential valid names or minimize attempts to that service and log the results. -
Missing entry in
target_service.protofile: To generate the correctTargetServiceentry, addHSQLDB = 26; // HyperSQL (SQL Database)at the end of theenum TargetServiceingoogle/detectors/credentials/generic_weak_credential_detector/src/main/proto/target_service.proto. Also, please update the "Next ID" field in the comment above it. -
Update tests accordingly.
Let me know if anything is unclear, and I'll be happy to provide further assistance.
| import com.google.tsunami.proto.NetworkService; | ||
| import java.sql.Connection; | ||
| import java.sql.SQLException; | ||
| import java.util.List; |
There was a problem hiding this comment.
Import the necessary libraries
| import java.util.List; | |
| import java.util.HashSet; | |
| import java.util.List; |
| public final class HyperSQLCredentialTester extends CredentialTester { | ||
| private static final GoogleLogger logger = GoogleLogger.forEnclosingClass(); | ||
| private final ConnectionProviderInterface connectionProvider; | ||
|
|
There was a problem hiding this comment.
Add error code constant
| private static final int HSQL_INVALID_USER_ERROR = -4001; | |
|
|
||
| @Override | ||
| public boolean batched() { | ||
| return true; |
There was a problem hiding this comment.
Disable batching to avoid unnecessary credential attempts
| return true; | |
| return false; |
| public ImmutableList<TestCredential> testValidCredentials( | ||
| NetworkService networkService, List<TestCredential> credentials) { | ||
| if (!canAccept(networkService)) { | ||
| return ImmutableList.of(); | ||
| } | ||
|
|
||
| return credentials.stream() | ||
| .filter(cred -> isHsqlAccessible(networkService, cred)) | ||
| .collect(toImmutableList()); | ||
| } |
There was a problem hiding this comment.
The invalidUsernames variable will contain a set of all usernames that have been tried and have failed, so they should be removed from the list of valid credentials.
| public ImmutableList<TestCredential> testValidCredentials( | |
| NetworkService networkService, List<TestCredential> credentials) { | |
| if (!canAccept(networkService)) { | |
| return ImmutableList.of(); | |
| } | |
| return credentials.stream() | |
| .filter(cred -> isHsqlAccessible(networkService, cred)) | |
| .collect(toImmutableList()); | |
| } | |
| public ImmutableList<TestCredential> testValidCredentials( | |
| NetworkService networkService, List<TestCredential> credentials) { | |
| HashSet<String> invalidUsernames = new HashSet<String>(); | |
| if (!canAccept(networkService)) { | |
| return ImmutableList.of(); | |
| } | |
| return credentials.stream() | |
| .filter(cred -> !invalidUsernames.contains(cred.username())) | |
| .filter(cred -> isHsqlAccessible(networkService, cred, invalidUsernames)) | |
| .collect(toImmutableList()); | |
| } |
| * connection However hsqldb does not create a default database during installation testdb is the | ||
| * database name used within the documentation | ||
| */ | ||
| private boolean isHsqlAccessible(NetworkService networkService, TestCredential credential) { |
There was a problem hiding this comment.
The method signature should be updated to accept the newly introduced variable.
| private boolean isHsqlAccessible(NetworkService networkService, TestCredential credential) { | |
| private boolean isHsqlAccessible( | |
| NetworkService networkService, TestCredential credential, HashSet<String> invalidUsernames) { |
| } catch (SQLException e) { | ||
| logger.atSevere().log( | ||
| "HyperSQLCredentialTester sql error: %s (%d)", e.getMessage(), e.getErrorCode()); | ||
| } |
There was a problem hiding this comment.
Error code -4001 is returned when the user is invalid.
Error code -4000 is returned when the user exists but the password is invalid.
This check optimizes testing by an order of magnitude, as it tries common passwords only on valid usernames.
| } catch (SQLException e) { | |
| logger.atSevere().log( | |
| "HyperSQLCredentialTester sql error: %s (%d)", e.getMessage(), e.getErrorCode()); | |
| } | |
| } catch (SQLException e) { | |
| if (e.getErrorCode() == HSQL_INVALID_USER_ERROR) { | |
| invalidUsernames.add(credential.username()); | |
| } | |
| logger.atSevere().log( | |
| "HyperSQLCredentialTester sql error: %s (%d)", e.getMessage(), e.getErrorCode()); | |
| } |
|
Hi @GiuseppePorcu, Are you still willing to contribute on that PR? If so, please apply the suggestions provided during the review so that we can proceed with merging it. Thank you, |
|
Hi @tooryx , Sorry for the delay, and thank you for your patience. |
Hi there,
This PR contains the implementation of the weak credential tester for HyperSQL.
As described within the Issue, the service has default credentials that if not changed allow an attacker to access the DBMS.
Below it is possible to find the necessary information for review:
Issue: #576
PR Testbed: google/security-testbeds#117
PR Scanner: google/tsunami-security-scanner#128
Thank you.