diff --git a/api/src/gmsa_service.cpp b/api/src/gmsa_service.cpp index 31025ced..4f123cbb 100644 --- a/api/src/gmsa_service.cpp +++ b/api/src/gmsa_service.cpp @@ -26,6 +26,8 @@ #define LEASE_ID_LENGTH 10 #define UNIX_SOCKET_NAME "credentials_fetcher.sock" #define INPUT_CREDENTIALS_LENGTH 104 +//https://devblogs.microsoft.com/oldnewthing/20120412-00/?p=7873 +#define DOMAIN_LENGTH 253 // invalid character in username/account name //https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/bb726984 @@ -59,9 +61,9 @@ void secureClearString(std::string& str) { */ bool isValidDomain(const std::string& value) { - // Regex to check valid domain name. - std::regex pattern("^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9](?:\\.[a-zA-Z]{2,})+$"); + // referenced from https://www.rfc-editor.org/rfc/rfc1123 + std::regex pattern("^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]{0,61}[a-zA-Z0-9])(\\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]{0,61}[a-zA-Z0-9]))*$"); // If the domain name // is empty return false @@ -475,10 +477,26 @@ class CredentialsFetcherImpl final krb_ticket_arns->credential_spec_arn = results[0]; int parse_result = parse_cred_spec_domainless( response, krb_ticket_info, krb_ticket_arns ); + if(parse_result != 0) + { + err_msg = "ERROR: invalid credentialspec fields"; + std::cout << getCurrentTime() << '\t' << err_msg + << std::endl; + break; + } // only add the ticket info if the parsing is successful if ( parse_result == 0 ) { + std::string secretsArn = + krb_ticket_arns->credential_domainless_user_arn; + if(secretsArn.empty()) + { + err_msg = "ERROR: invalid secrets manager arn"; + std::cout << getCurrentTime() << '\t' << err_msg + << std::endl; + break; + } // retrieve domainless user credentials std::tuple userCreds = retrieve_credspec_from_secrets_manager( @@ -495,7 +513,7 @@ class CredentialsFetcherImpl final if ( !username.empty() && !password.empty() && !domain.empty() && username.length() < INPUT_CREDENTIALS_LENGTH && - password.length() < INPUT_CREDENTIALS_LENGTH ) + password.length() < INPUT_CREDENTIALS_LENGTH && domain.length() < DOMAIN_LENGTH) { std::string krb_files_path = @@ -866,9 +884,27 @@ class CredentialsFetcherImpl final int parse_result = parse_cred_spec_domainless( response, krb_ticket_info, krb_ticket_arns ); + if(parse_result != 0) + { + err_msg = "ERROR: invalid credentialspec fields"; + std::cout << getCurrentTime() << '\t' << err_msg + << std::endl; + break; + } + // only add the ticket info if the parsing is successful if ( parse_result == 0 ) { + std::string secretsArn = + krb_ticket_arns->credential_domainless_user_arn; + if(secretsArn.empty()) + { + err_msg = "ERROR: invalid secrets manager arn"; + std::cout << getCurrentTime() << '\t' << err_msg + << std::endl; + break; + } + // retrieve domainless user credentials std::tuple userCreds = retrieve_credspec_from_secrets_manager( @@ -886,7 +922,7 @@ class CredentialsFetcherImpl final if ( !username.empty() && !password.empty() && !domain.empty() && username.length() < INPUT_CREDENTIALS_LENGTH && - password.length() < INPUT_CREDENTIALS_LENGTH ) + password.length() < INPUT_CREDENTIALS_LENGTH && domain.length() < DOMAIN_LENGTH) { std::string renewal_path = renew_gmsa_ticket( krb_ticket, domain, username, password, cf_logger ); @@ -898,6 +934,7 @@ class CredentialsFetcherImpl final "credentials should not be more than 256 charaters"; std::cout << getCurrentTime() << '\t' << err_msg << std::endl; + break; } } else @@ -905,6 +942,7 @@ class CredentialsFetcherImpl final err_msg = "ERROR: invalid domainName/username"; std::cout << getCurrentTime() << '\t' << err_msg << std::endl; + break; } } } @@ -1103,6 +1141,14 @@ class CredentialsFetcherImpl final int parse_result = parse_cred_spec( create_krb_request_.credspec_contents( i ), krb_ticket_info ); + if(parse_result != 0) + { + err_msg = "ERROR: invalid credentialspec fields"; + std::cout << getCurrentTime() << '\t' << err_msg + << std::endl; + break; + } + // only add the ticket info if the parsing is successful if ( parse_result == 0 ) { @@ -1375,18 +1421,38 @@ class CredentialsFetcherImpl final !contains_invalid_characters_in_ad_account_name(username)) { if ( !username.empty() && !password.empty() && !domain.empty() && username.length() < INPUT_CREDENTIALS_LENGTH && password.length() < - INPUT_CREDENTIALS_LENGTH ) + INPUT_CREDENTIALS_LENGTH + && domain.length() < DOMAIN_LENGTH && create_domainless_krb_request_ + .credspec_contents_size() > 0) { create_domainless_krb_reply_.set_lease_id( lease_id ); for ( int i = 0; i < create_domainless_krb_request_.credspec_contents_size(); i++ ) { + std::string credspecContent = create_domainless_krb_request_ + .credspec_contents( i ); + if(credspecContent.empty()) + { + err_msg = "Error: credentialspec content shouldn't be empty " + "formatted"; + std::cout << getCurrentTime() << '\t' << err_msg << std::endl; + break; + } creds_fetcher::krb_ticket_info* krb_ticket_info = new creds_fetcher::krb_ticket_info; + int parse_result = parse_cred_spec( create_domainless_krb_request_.credspec_contents( i ), krb_ticket_info ); + if(parse_result != 0) + { + err_msg = "ERROR: invalid credentialspec fields"; + std::cout << getCurrentTime() << '\t' << err_msg + << std::endl; + break; + } + // only add the ticket info if the parsing is successful if ( parse_result == 0 ) { @@ -1678,7 +1744,8 @@ class CredentialsFetcherImpl final !contains_invalid_characters_in_ad_account_name(username)) { if ( !username.empty() && !password.empty() && !domain.empty() && username.length() < INPUT_CREDENTIALS_LENGTH && password.length() < - INPUT_CREDENTIALS_LENGTH ) + INPUT_CREDENTIALS_LENGTH + && domain.length() < DOMAIN_LENGTH) { std::list renewed_krb_file_paths = renew_kerberos_tickets_domainless( krb_files_dir, domain, username, diff --git a/api/tests/gmsa_test_client.cpp b/api/tests/gmsa_test_client.cpp index 4cfe404d..7f11ee5c 100644 --- a/api/tests/gmsa_test_client.cpp +++ b/api/tests/gmsa_test_client.cpp @@ -572,7 +572,7 @@ int run_stress_test( CredentialsFetcherClient& client, int num_of_leases, } // unit tests -int parse_credspec_domainless_test(std::string credspec) +bool parse_credspec_domainless_test(std::string credspec) { creds_fetcher::krb_ticket_info* krb_ticket_info = new creds_fetcher::krb_ticket_info; @@ -581,7 +581,19 @@ int parse_credspec_domainless_test(std::string credspec) int response = parse_cred_spec_domainless(credspec, krb_ticket_info, krb_ticket_arn_mapping ); std::cout << krb_ticket_arn_mapping->credential_spec_arn; std::cout << krb_ticket_arn_mapping->krb_file_path; - return response; + if(response == 0) + { + return true; + } + return false; +} + +int validate_domain() +{ + return (isValidDomain("a.com") && isValidDomain("ab.toto-abc.com") && + !isValidDomain("p/") && isValidDomain("test4.gmsa-pentest.com") && + !isValidDomain ("-testdomain.org") && isValidDomain("contoso.com") && + !isValidDomain(".org")); } #if AMAZON_LINUX_DISTRO @@ -678,9 +690,16 @@ int main( int argc, char** argv ) } else if (arg == "--unit_test") { - parse_credspec_domainless_test(credspec_contents_domainless_str); - //retrieve_credspec_from_s3_test(); - return 0; + + bool testStatus = (parse_credspec_domainless_test(credspec_contents_domainless_str) && validate_domain()); + if(!testStatus){ + std::cout << "client tests failed" << std::endl; + return EXIT_FAILURE; + } + else{ + std::cout << "client tests successful" << std::endl; + return EXIT_SUCCESS; + } } else if ( arg == "--check" ) { diff --git a/auth/kerberos/src/krb.cpp b/auth/kerberos/src/krb.cpp index 82ef656e..06f0f643 100644 --- a/auth/kerberos/src/krb.cpp +++ b/auth/kerberos/src/krb.cpp @@ -1126,13 +1126,20 @@ std::string retrieve_secret_from_ecs_config(std::string ecs_variable_name) while ( std::getline( config_file, line ) ) { - // TBD: Error handling for incorrectly formatted /etc/ecs/ecs.config results = split_string(line, '='); std::string key = results[0]; std::string value = results[1]; if ( ecs_variable_name.compare( key ) == 0 ) { value.erase( std::remove( value.begin(), value.end(), '"' ), value.end() ); + + if( contains_invalid_characters_in_ad_account_name(value)) + { + std::cout << getCurrentTime() << '\t' << "invalid domain controller name" << + std::endl; + return ""; + } + return value; } } diff --git a/common/daemon.h b/common/daemon.h index c033a544..ec5fdaf9 100644 --- a/common/daemon.h +++ b/common/daemon.h @@ -85,10 +85,17 @@ namespace creds_fetcher { if ( level >= log_level ) { - sd_journal_print( level, fmt, logs... ); + std::string logFmt = fmt; + for (int i = 0; logFmt[i] != '\0'; ++i) { + if (logFmt[i] == '\n') { + logFmt[i] = ' '; // Replace '\n' with space + } + } + sd_journal_print( level, logFmt.c_str(), logs... ); } } + void init_file_logger () { std::string log_file_path = LOG_FILE_PATH; @@ -190,6 +197,7 @@ int renewal_failure_krb_dir_not_found_test(); * Methods in config module */ int parse_options( int argc, const char* argv[], creds_fetcher::Daemon& cf_daemon ); +bool isValidDomain(const std::string& value); int HealthCheck(std::string serviceName); int parse_config_file( creds_fetcher::Daemon& cf_daemon ); @@ -203,6 +211,7 @@ bool contains_invalid_characters_in_credentials( const std::string& value ); int RunGrpcServer( std::string unix_socket_dir, std::string krb_file_path, creds_fetcher::CF_logger& cf_logger, volatile sig_atomic_t* shutdown_signal, std::string aws_sm_secret_name ); +bool contains_invalid_characters_in_ad_account_name( const std::string& value ); int parse_cred_spec( std::string credspec_data, creds_fetcher::krb_ticket_info* krb_ticket_info );