Skip to content

Commit

Permalink
Always ask for OTP code if nullok has not been set (#70)
Browse files Browse the repository at this point in the history 
This prevents user account existence information leak.

Fixes #69
  • Loading branch information
@nielsbasjes @ThomasHabets
nielsbasjes authored and ThomasHabets committed Aug 18, 2017
1 parent db91c8a commit dd0cfc0
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 49 deletions.
124 changes: 81 additions & 43 deletions src/pam_google_authenticator.c
View file
Expand Up @@ -160,6 +160,10 @@ static const char *get_user_name(pam_handle_t *pamh, const Params *params) {

static char *get_secret_filename(pam_handle_t *pamh, const Params *params,
const char *username, int *uid) {
if (!username) {
return NULL;
}

// Check whether the administrator decided to override the default location
// for the secret file.
const char *spec = params->secret_filename_spec
Expand Down Expand Up @@ -577,6 +581,9 @@ static uint8_t *get_shared_secret(pam_handle_t *pamh,
const Params *params,
const char *secret_filename,
const char *buf, int *secretLen) {
if (!buf) {
return NULL;
}
// Decode secret key
const int base32Len = strcspn(buf, "\n");
*secretLen = (base32Len*5 + 7)/8;
Expand Down Expand Up @@ -773,6 +780,9 @@ static int get_timestamp(pam_handle_t *pamh, const char *secret_filename,
}

static long get_hotp_counter(pam_handle_t *pamh, const char *buf) {
if (!buf) {
return -1;
}
const char *counter_str = get_cfg_value(pamh, "HOTP_COUNTER", buf);
if (counter_str == &oom) {
// Out of memory. This is a fatal error
Expand Down Expand Up @@ -1561,16 +1571,40 @@ static int google_authenticator(pam_handle_t *pamh, int flags,

// Read and process status file, then ask the user for the verification code.
int early_updated = 0, updated = 0;
if ((username = get_user_name(pamh, &params)) &&
(secret_filename = get_secret_filename(pamh, &params, username, &uid)) &&
!drop_privileges(pamh, username, uid, &old_uid, &old_gid) &&
(fd = open_secret_file(pamh, secret_filename, &params, username, uid,
&orig_stat)) >= 0 &&
(buf = read_file_contents(pamh, &params, secret_filename, &fd,
orig_stat.st_size)) &&
(secret = get_shared_secret(pamh, &params, secret_filename, buf, &secretLen)) &&
rate_limit(pamh, secret_filename, &early_updated, &buf) >= 0) {
const long hotp_counter = get_hotp_counter(pamh, buf);

username = get_user_name(pamh, &params);
secret_filename = get_secret_filename(pamh, &params, username, &uid);

int stopped_by_rate_limit = 0;

if (secret_filename &&
!drop_privileges(pamh, username, uid, &old_uid, &old_gid)) {
fd = open_secret_file(pamh, secret_filename, &params, username, uid, &orig_stat);
if (fd >= 0) {
buf = read_file_contents(pamh, &params, secret_filename, &fd, orig_stat.st_size);
}

if (buf) {
if (rate_limit(pamh, secret_filename, &early_updated, &buf) >= 0) {
secret = get_shared_secret(pamh, &params, secret_filename, buf, &secretLen);
} else {
stopped_by_rate_limit=1;
}
}
}

const long hotp_counter = get_hotp_counter(pamh, buf);

// Only if nullok and we do not have a code will we NOT ask for a code.
// In all other cases (i.e "have code" and "no nullok and no code") we DO ask for a code.
if (!stopped_by_rate_limit &&
( secret || (!secret && params.nullok != SECRETNOTFOUND ) )
) {

if (!secret) {
log_message(LOG_WARNING , pamh, "No secret configured for user %s, asking for code anyway.", username);
}

int must_advance_counter = 0;
char *pw = NULL, *saved_pw = NULL;
for (int mode = 0; mode < 4; ++mode) {
Expand Down Expand Up @@ -1670,43 +1704,47 @@ static int google_authenticator(pam_handle_t *pamh, int flags,
}
}

// Check all possible types of verification codes.
switch (check_scratch_codes(pamh, &params, secret_filename, &updated, buf, code)){
case 1:
if (hotp_counter > 0) {
switch (check_counterbased_code(pamh, secret_filename, &updated,
&buf, secret, secretLen, code,
&params, hotp_counter,
&must_advance_counter)) {
case 0:
rc = PAM_SUCCESS;
break;
case 1:
goto invalid;
default:
break;
}
} else {
switch (check_timebased_code(pamh, secret_filename, &updated, &buf,
secret, secretLen, code, &params)) {
case 0:
rc = PAM_SUCCESS;
break;
case 1:
goto invalid;
default:
break;
// Only if we actually have a secret will we try to verify the code
// In all other cases will we just remain at PAM_AUTH_ERR
if (secret) {
// Check all possible types of verification codes.
switch (check_scratch_codes(pamh, &params, secret_filename, &updated, buf, code)) {
case 1:
if (hotp_counter > 0) {
switch (check_counterbased_code(pamh, secret_filename, &updated,
&buf, secret, secretLen, code,
&params, hotp_counter,
&must_advance_counter)) {
case 0:
rc = PAM_SUCCESS;
break;
case 1:
goto invalid;
default:
break;
}
} else {
switch (check_timebased_code(pamh, secret_filename, &updated, &buf,
secret, secretLen, code, &params)) {
case 0:
rc = PAM_SUCCESS;
break;
case 1:
goto invalid;
default:
break;
}
}
break;
case 0:
rc = PAM_SUCCESS;
break;
default:
break;
}
break;
case 0:
rc = PAM_SUCCESS;
break;
default:

break;
}

break;
}

// Update the system password, if we were asked to forward
Expand Down
23 changes: 17 additions & 6 deletions tests/pam_google_authenticator_unittest.c
View file
Expand Up @@ -253,13 +253,15 @@ int main(int argc, char *argv[]) {
int targc;
int expected_good_prompts_shown;
int expected_bad_prompts_shown;
int password_is_provided_from_external;

switch (otp_mode) {
case 0:
puts("\nRunning tests, querying for verification code");
conv_mode = TWO_PROMPTS;
targc = 1;
expected_good_prompts_shown = expected_bad_prompts_shown = 1;
password_is_provided_from_external = 0;
break;
case 1:
puts("\nRunning tests, querying for verification code, "
Expand All @@ -268,13 +270,15 @@ int main(int argc, char *argv[]) {
targv[1] = strdup("forward_pass");
targc = 2;
expected_good_prompts_shown = expected_bad_prompts_shown = 1;
password_is_provided_from_external = 0;
break;
case 2:
puts("\nRunning tests with use_first_pass");
conv_mode = COMBINED_PASSWORD;
targv[1] = strdup("use_first_pass");
targc = 2;
expected_good_prompts_shown = expected_bad_prompts_shown = 0;
password_is_provided_from_external = 1;
break;
case 3:
puts("\nRunning tests with use_first_pass, forwarding system pass");
Expand All @@ -283,6 +287,7 @@ int main(int argc, char *argv[]) {
targv[2] = strdup("forward_pass");
targc = 3;
expected_good_prompts_shown = expected_bad_prompts_shown = 0;
password_is_provided_from_external = 1;
break;
case 4:
puts("\nRunning tests with try_first_pass, combining codes");
Expand All @@ -291,6 +296,7 @@ int main(int argc, char *argv[]) {
targc = 2;
expected_good_prompts_shown = 0;
expected_bad_prompts_shown = 2;
password_is_provided_from_external = 1;
break;
case 5:
puts("\nRunning tests with try_first_pass, combining codes, "
Expand All @@ -301,13 +307,15 @@ int main(int argc, char *argv[]) {
targc = 3;
expected_good_prompts_shown = 0;
expected_bad_prompts_shown = 2;
password_is_provided_from_external = 1;
break;
case 6:
puts("\nRunning tests with try_first_pass, querying for codes");
conv_mode = TWO_PROMPTS;
targv[1] = strdup("try_first_pass");
targc = 2;
expected_good_prompts_shown = expected_bad_prompts_shown = 1;
password_is_provided_from_external = 1;
break;
default:
assert(otp_mode == 7);
Expand All @@ -318,6 +326,7 @@ int main(int argc, char *argv[]) {
targv[2] = strdup("forward_pass");
targc = 3;
expected_good_prompts_shown = expected_bad_prompts_shown = 1;
password_is_provided_from_external = 1;
break;
}

Expand Down Expand Up @@ -362,7 +371,7 @@ int main(int argc, char *argv[]) {
const char *old_secret = targv[0];
targv[0] = "secret=/NOSUCHFILE";
assert(pam_sm_authenticate(NULL, 0, targc, targv) == PAM_AUTH_ERR);
verify_prompts_shown(0);
verify_prompts_shown(password_is_provided_from_external ? 0 : expected_bad_prompts_shown);
targv[targc++] = "nullok";
targv[targc] = NULL;
assert(pam_sm_authenticate(NULL, 0, targc, targv) == PAM_IGNORE);
Expand Down Expand Up @@ -517,15 +526,17 @@ int main(int argc, char *argv[]) {
(i >= 2 ? PAM_SUCCESS : PAM_AUTH_ERR));
verify_prompts_shown(expected_good_prompts_shown);
}
set_time(12010 * 30);

puts("Testing TIME_SKEW - noskewadj");
set_time(12020 * 30);
char buf[7];
response = buf;
sprintf(response, "%06d", compute_code(binary_secret,
binary_secret_len, 11010));
assert(pam_sm_authenticate(NULL, 0, 1,
(const char *[]){ "noskewadj", 0 }) ==
PAM_AUTH_ERR);
verify_prompts_shown(0);
targv[targc] = "noskewadj";
assert(pam_sm_authenticate(NULL, 0, targc+1, targv) == PAM_AUTH_ERR);
targv[targc] = NULL;
verify_prompts_shown(expected_bad_prompts_shown);
set_time(10000*30);

// Test scratch codes
Expand Down

0 comments on commit dd0cfc0

Please sign in to comment.