diff options
author | Ladar Levison <ladar@lavabit.com> | 2017-03-31 10:08:36 +0300 |
---|---|---|
committer | Ladar Levison <ladar@lavabit.com> | 2017-03-31 10:08:36 +0300 |
commit | f441af85e6006a761f65d996b44966688e60fd67 (patch) | |
tree | ea17d58f79d51b9f9ce8d9bc78397d93dff8e411 /check/magma/servers | |
parent | 8a70541a696f5c41f6f21758879e43ddefb13290 (diff) |
Misc changes associated with the SMTP and POP unit test code review.
Diffstat (limited to 'check/magma/servers')
-rw-r--r-- | check/magma/servers/pop/pop_check.c | 4 | ||||
-rw-r--r-- | check/magma/servers/pop/pop_check_network.c | 52 | ||||
-rw-r--r-- | check/magma/servers/smtp/smtp_check.c | 28 | ||||
-rw-r--r-- | check/magma/servers/smtp/smtp_check.h | 4 | ||||
-rw-r--r-- | check/magma/servers/smtp/smtp_check_network.c | 46 |
5 files changed, 60 insertions, 74 deletions
diff --git a/check/magma/servers/pop/pop_check.c b/check/magma/servers/pop/pop_check.c index 6e3fb089..b3b1c45e 100644 --- a/check/magma/servers/pop/pop_check.c +++ b/check/magma/servers/pop/pop_check.c @@ -20,9 +20,6 @@ START_TEST (check_pop_network_basic_tcp_s) { else if (status() && !check_pop_network_basic_sthread(errmsg, tcp->network.port, false)) { outcome = false; } - else { - errmsg = NULL; - } log_test("POP / NETWORK / BASIC / TCP / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); @@ -42,7 +39,6 @@ START_TEST (check_pop_network_basic_tls_s) { } else if (status() && !check_pop_network_basic_sthread(errmsg, tls->network.port, true)) { outcome = false; - errmsg = NULL; } log_test("POP / NETWORK / BASIC / TLS / SINGLE THREADED:", errmsg); diff --git a/check/magma/servers/pop/pop_check_network.c b/check/magma/servers/pop/pop_check_network.c index b45c477d..348e683d 100644 --- a/check/magma/servers/pop/pop_check_network.c +++ b/check/magma/servers/pop/pop_check_network.c @@ -12,61 +12,71 @@ * @brief Calls client_read_line on a client until it reaches a period only line, returning * the number of messages in the inbox. * - * @param client The client_t* to read from (which should be connected to a POP server) - * @param size A uint64_t*. If not null, the total size of the lines read will be placed + * @param client The client_t pointer to read from (which should be connected to a POP server) + * @param size A uint64_t pointer. If not NULL, the total size of the lines read will be placed * at this address. - * @param token If not NULL and size if not NULL, then size will only be incremented after - * reaching a line that begins with token. + * @param token If not NULL, then the size variable will only include the number of bytes read after the token. * @return true if a line containing a single period is found, false if not. */ bool_t check_pop_client_read_end(client_t *client, uint64_t *size, chr_t *token) { - if (size) *size = 0; bool_t token_found = false; + if (size) *size = 0; + else if (!token) token_found = true; + + // There shouldn't be a token, if we aren't also supposed to be counting the number of bytes. + else if (!size && token) return false; + while (client_read_line(client) > 0) { + // Break when a line with just a period is found. if (!st_cmp_cs_eq(&(client->line), NULLER(".\r\n"))) return true; - else if (size && st_cmp_cs_starts(&(client->line), NULLER(token)) == 0) token_found = true; + + // If we have a size and a token, then keep checking for the token until its found. + else if (size && token && !token_found && st_cmp_cs_starts(&(client->line), NULLER(token)) == 0) token_found = true; if (size && token_found) *size += pl_length_get(client->line); } + return false; } /** - * Calls client_read_line on a client until it reaches a period only line, returning the - * number of messages in the inbox. + * @brief Calls client_read_line until it reaches a line containing only a period, then returns the number + * of messages it encountered. * - * @param client The client_t* to read from (which should be connected to a POP server). - * @param errmsg The stringer_t* to which error messages will be printed in event of an error. - * @return a uint32_t containing the number of messages in the inbox. + * @param client The client_t pointer to read from (which should be connected to a POP server). + * @param errmsg The stringer_t pointer to which error messages will be printed in event of an error. + * @return an uint32_t containing the number of messages in the inbox. */ uint64_t check_pop_client_read_list(client_t *client, stringer_t *errmsg) { placer_t fragment = pl_null(); uint64_t counter = 1, sequence = 0; - client_read_line(client); - while (client_read_line(client) > 0) { + /// LOW: Parse out the total message number from the first line returned and check against that at the end of the + /// function, returning an error if it and the counter do not match. + if (client_read_line(client) <= 0 || !pl_starts_with_char(client->line, '+')) { + st_sprint(errmsg, "The message list response failed to return a valid response."); + return 0; + } + while (client_read_line(client) > 0 && !pl_starts_with_char(client->line, '.')) { - if (pl_starts_with_char(client->line, '.')) { - return counter-2; - } - else if (tok_get_st(&(client->line), ' ', 0, &fragment) >= 0 && !uint64_conv_pl(fragment, &sequence) == 0) { - if (sequence != counter) return 0; - } - else { + // If the sequence number doesn't match our counter variable, we'll indicate an error. + if (tok_get_st(&(client->line), ' ', 0, &fragment) >= 0 && !uint64_conv_pl(fragment, &sequence) == 0 && sequence != counter) { + st_sprint(errmsg, "The message sequence appears to have skipped, because the internal counter no longer matches the sequence."); return 0; } counter++; } - return 0; + return counter - 1; } +/// LOW: This should use stringer parameters. bool_t check_pop_client_auth(client_t *client, chr_t *user, chr_t *pass, stringer_t *errmsg) { if (client_print(client, "USER %s\r\n", user) != (ns_length_get(user) + 7) || client_read_line(client) <= 0 || diff --git a/check/magma/servers/smtp/smtp_check.c b/check/magma/servers/smtp/smtp_check.c index a0b7e8d3..836d3072 100644 --- a/check/magma/servers/smtp/smtp_check.c +++ b/check/magma/servers/smtp/smtp_check.c @@ -21,9 +21,6 @@ START_TEST (check_smtp_network_basic_tcp_s) { else if (status() && !check_smtp_network_basic_sthread(errmsg, server->network.port, false)) { outcome = false; } - else { - errmsg = NULL; - } log_test("SMTP / NETWORK / BASIC / TCP / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); @@ -44,9 +41,6 @@ START_TEST (check_smtp_network_basic_tls_s) { else if (status() && !check_smtp_network_basic_sthread(errmsg, server->network.port, true)) { outcome = false; } - else { - errmsg = NULL; - } log_test("SMTP / NETWORK / BASIC / TLS / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); @@ -59,8 +53,7 @@ START_TEST (check_smtp_accept_store_message_s) { bool_t outcome = true; stringer_t *errmsg = MANAGEDBUF(2048); - outcome = check_smtp_accept_message_sthread(errmsg); - if (outcome) errmsg = NULL; + if (status()) outcome = check_smtp_accept_message_sthread(errmsg); log_test("SMTP / ACCEPT / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); @@ -73,8 +66,7 @@ START_TEST (check_smtp_checkers_greylist_s) { bool_t outcome = true; stringer_t *errmsg = MANAGEDBUF(1024); - outcome = check_smtp_checkers_greylist_sthread(errmsg); - if (outcome) errmsg = NULL; + if (status()) outcome = check_smtp_checkers_greylist_sthread(errmsg); log_test("SMTP / CHECKERS / GREYLIST / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); @@ -93,8 +85,6 @@ START_TEST (check_smtp_checkers_filters_s) { if (status() && outcome) outcome = check_smtp_checkers_filters_sthread(errmsg, SMTP_FILTER_ACTION_LABEL, 3); if (status() && outcome) outcome = check_smtp_checkers_filters_sthread(errmsg, SMTP_FILTER_ACTION_MARK_READ, 4); - if (outcome) errmsg = NULL; - log_test("SMTP / CHECKERS / FILTERS / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); } @@ -114,9 +104,10 @@ START_TEST (check_smtp_network_auth_plain_s) { else if (status() && !check_smtp_network_auth_sthread(errmsg, server->network.port, false)) { outcome = false; } - else { - errmsg = NULL; - } + + /// LOW: Add a variation of this test which takes place over TCP and thus fails specifically because the connection + /// lacks transport security (aka TLS). In other words, test for valid credentials first, and that it works via TLS, + /// before ensuring the same inputs fail via TCP. log_test("SMTP / NETWORK / AUTH PLAIN / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); @@ -137,9 +128,10 @@ START_TEST (check_smtp_network_auth_login_s) { else if (status() && !check_smtp_network_auth_sthread(errmsg, server->network.port, true)) { outcome = false; } - else { - errmsg = NULL; - } + + /// LOW: Add a variation of this test which takes place over TCP and thus fails specifically because the connection + /// lacks transport security (aka TLS). In other words, test for valid credentials first, and that it works via TLS, + /// before ensuring the same inputs fail via TCP. log_test("SMTP / NETWORK / AUTH LOGIN / SINGLE THREADED:", errmsg); ck_assert_msg(outcome, st_char_get(errmsg)); diff --git a/check/magma/servers/smtp/smtp_check.h b/check/magma/servers/smtp/smtp_check.h index 0e55dfbf..845b9f43 100644 --- a/check/magma/servers/smtp/smtp_check.h +++ b/check/magma/servers/smtp/smtp_check.h @@ -18,8 +18,8 @@ bool_t check_smtp_checkers_filters_sthread(stringer_t *errmsg, int_t action, int /// smtp_check_network.c bool_t check_smtp_client_read_end(client_t *client); bool_t check_smtp_client_mail_rcpt_data(client_t *client, chr_t *from, chr_t *to, stringer_t *errmsg); -bool_t check_smtp_client_auth_plain(client_t *client, chr_t *pass, stringer_t *errmsg); -bool_t check_smtp_client_auth_login(client_t *client, chr_t *user, chr_t *pass, stringer_t *errmsg); +bool_t check_smtp_client_auth_plain(client_t *client, stringer_t *auth); +bool_t check_smtp_client_auth_login(client_t *client, stringer_t *user, stringer_t *pass); bool_t check_smtp_client_quit(client_t *client, stringer_t *errmsg); bool_t check_smtp_network_basic_sthread(stringer_t *errmsg, uint32_t port, bool_t secure); bool_t check_smtp_network_auth_sthread(stringer_t *errmsg, uint32_t port, bool_t login); diff --git a/check/magma/servers/smtp/smtp_check_network.c b/check/magma/servers/smtp/smtp_check_network.c index 0a68fbed..2f75098f 100644 --- a/check/magma/servers/smtp/smtp_check_network.c +++ b/check/magma/servers/smtp/smtp_check_network.c @@ -34,7 +34,7 @@ bool_t check_smtp_client_read_end(client_t *client) { bool_t check_smtp_client_mail_rcpt_data(client_t *client, chr_t *from, chr_t *to, stringer_t *errmsg) { chr_t *line_from = "MAIL FROM: <%s>\r\n", *line_to = "RCPT TO: <%s>\r\n"; - size_t size_from = ns_length_get(line_from) + ns_length_get(from) -2, size_to = ns_length_get(line_to) + ns_length_get(to) -2; + size_t size_from = ns_length_get(line_from) + ns_length_get(from) - 2, size_to = ns_length_get(line_to) + ns_length_get(to) - 2; // Issue MAIL command. if (client_print(client, line_from, from) != size_from || !check_smtp_client_read_end(client) || @@ -69,15 +69,11 @@ bool_t check_smtp_client_mail_rcpt_data(client_t *client, chr_t *from, chr_t *to * and error * @return True if no errors, false otherwise */ -bool_t check_smtp_client_auth_plain(client_t *client, chr_t *auth, stringer_t *errmsg) { +bool_t check_smtp_client_auth_plain(client_t *client, stringer_t *auth) { - chr_t *line_auth = "AUTH PLAIN %s\r\n"; - - if (client_print(client, line_auth, auth) != (ns_length_get(line_auth) + ns_length_get(auth)) -2 || + if (client_print(client, "AUTH PLAIN %.*s\r\n", st_length_int(auth), st_char_get(auth)) != st_length_get(auth) + 13 || !check_smtp_client_read_end(client) || client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("235"))) { - - st_sprint(errmsg, "Failed to return a successful status after submitting credentials"); return false; } @@ -88,30 +84,22 @@ bool_t check_smtp_client_auth_plain(client_t *client, chr_t *auth, stringer_t *e * @brief Submits the AUTH LOGIN command to the passed client using the passed parameters * @param client A client_t* connected to an SMTP server that has had the HELO/EHLO command * already submitted - * @param user A chr_t* containing the username of the user - * @param pass A chr_t* containing the password of the user - * @param errmsg A stringer_t* that will have the error message printed to it in the event of - * and error - * @return True if no errors, false otherwise + * @param user A NULL string containing the username of the user. + * @param pass A NULL string containing the password of the user. + * @return true if no errors, false otherwise */ -bool_t check_smtp_client_auth_login(client_t *client, chr_t *user, chr_t *pass, stringer_t *errmsg) { +bool_t check_smtp_client_auth_login(client_t *client, stringer_t *user, stringer_t *pass) { if (client_write(client, PLACER("AUTH LOGIN\r\n", 12)) != 12 || !check_smtp_client_read_end(client) || client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("334"))) { - - st_sprint(errmsg, "Failed to return a proceed status code after AUTH LOGIN."); return false; } - else if (client_print(client, "%s\r\n", user) != ns_length_get(user) + 2 || !check_smtp_client_read_end(client) || - client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("334"))) { - - st_sprint(errmsg, "Failed to return a proceed status code after submitting username."); + else if (client_print(client, "%.*s\r\n", st_length_int(user), st_char_get(user)) != st_length_get(user) + 2 || + !check_smtp_client_read_end(client) || client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("334"))) { return false; } - else if (client_print(client, "%s\r\n", pass) != ns_length_get(pass) + 2 || !check_smtp_client_read_end(client) || - client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("235"))) { - - st_sprint(errmsg, "Failed to return a successful status after submitting credentials."); + else if (client_print(client, "%.*s\r\n", st_length_int(pass), st_char_get(pass)) != st_length_get(pass) + 2 || + !check_smtp_client_read_end(client) || client_status(client) != 1 || st_cmp_cs_starts(&(client->line), NULLER("235"))) { return false; } @@ -250,16 +238,16 @@ bool_t check_smtp_network_auth_sthread(stringer_t *errmsg, uint32_t port, bool_t return false; } // Issue AUTH with incorrect credentials. - else if ((login ? check_smtp_client_auth_login(client, "bWFnbWE=", "aW52YWxpZHBhc3N3b3Jk", errmsg) - : check_smtp_client_auth_plain(client, "bWFnbWEAbWFnbWEAaW52YWxpZHBhc3N3b3Jk", errmsg))) { - + else if ((login ? check_smtp_client_auth_login(client, NULLER("bWFnbWE="), NULLER("aW52YWxpZHBhc3N3b3Jk")) + : check_smtp_client_auth_plain(client, NULLER("bWFnbWEAbWFnbWEAaW52YWxpZHBhc3N3b3Jk")))) { + st_sprint(errmsg, "Invalid credentials appear to have authenticated when they should have failed."); client_close(client); return false; } // Issue AUTH with correct credentials. - else if (!(login ? check_smtp_client_auth_login(client, "bWFnbWE=", "cGFzc3dvcmQ=", errmsg) - : check_smtp_client_auth_plain(client, "bWFnbWEAbWFnbWEAcGFzc3dvcmQ=", errmsg))) { - + else if (!(login ? check_smtp_client_auth_login(client, NULLER("bWFnbWE="), NULLER("cGFzc3dvcmQ=")) + : check_smtp_client_auth_plain(client, NULLER("bWFnbWEAbWFnbWEAcGFzc3dvcmQ=")))) { + st_sprint(errmsg, "Failed to authenticate even though we supplied valid credentials."); client_close(client); return false; } |