OK, I've addressed all of your comments except for my usage of getopt32. How do I set it up so that -K can only appear if -k appears?
On Fri, Oct 12, 2018 at 3:49 AM Bernhard Reutner-Fischer < [email protected]> wrote: > On 11 October 2018 18:38:44 CEST, Brandon Enochs <[email protected]> > wrote: > >diff --git a/networking/ntpd.c b/networking/ntpd.c > >index 1ebdc34..ff9ac5d 100644 > >--- a/networking/ntpd.c > >+++ b/networking/ntpd.c > >@@ -62,6 +62,24 @@ > > //config: help > > //config: Make ntpd look in /etc/ntp.conf for peers. Only "server > >address" > > //config: is supported. > >+//config:config FEATURE_NTP_AUTH > >+//config: bool "Make ntpd support authentication" > >+//config: default n > >+//config: depends on NTPD > >+//config: help > >+//config: Make ntpd support authentication" > >+//config:config FEATURE_NTP_AUTH_MD5 > >+//config: bool "Make ntpd support MD5 authentication" > >+//config: default n > >+//config: depends on FEATURE_NTP_AUTH > >+//config: help > >+//config: Make ntpd support MD5 authentication" > >+//config:config FEATURE_NTP_AUTH_SHA1 > >+//config: bool "Make ntpd support SHA1 authentication" > >+//config: default n > >+//config: depends on FEATURE_NTP_AUTH > >+//config: help > >+//config: Make ntpd support SHA1 authentication" > > > > //applet:IF_NTPD(APPLET(ntpd, BB_DIR_USR_SBIN, BB_SUID_DROP)) > > > >@@ -78,6 +96,8 @@ > >//usage: "\n -w Do not set time (only query peers), implies > >-n" > > //usage: "\n -S PROG Run PROG after stepping time, stratum > >change, and every 11 mins" > >//usage: "\n -p PEER Obtain time from PEER (may be repeated)" > >+//usage: "\n -K key number for preceding PEER (may be > >repeated)" > >+//usage: "\n -k key file (see man 5 ntp.keys)" > > //usage: IF_FEATURE_NTPD_CONF( > > //usage: "\n If -p is not given, 'server HOST' lines" > > //usage: "\n from /etc/ntp.conf are used" > >@@ -227,15 +247,24 @@ > > #define FLL (MAXPOLL + 1) > > /* Parameter averaging constant */ > > #define AVG 4 > >- > >+#define MAX_KEY_NUMBER 65535 > >+#define KEYID_SIZE sizeof(uint32_t) > > > > enum { > > NTP_VERSION = 4, > > NTP_MAXSTRATUM = 15, > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+ NTP_MD5_DIGESTSIZE = 16, > >+ NTP_SHA1_DIGESTSIZE = 20, > >+ NTP_MSGSIZE_NOAUTH = 48, > >+ NTP_MSGSIZE_MD5_AUTH = NTP_MSGSIZE_NOAUTH + NTP_MD5_DIGESTSIZE + > >KEYID_SIZE, > >+ NTP_MSGSIZE_SHA1_AUTH = NTP_MSGSIZE_NOAUTH + NTP_SHA1_DIGESTSIZE + > >KEYID_SIZE, > >+#else > > NTP_DIGESTSIZE = 16, > > NTP_MSGSIZE_NOAUTH = 48, > > NTP_MSGSIZE = (NTP_MSGSIZE_NOAUTH + 4 + NTP_DIGESTSIZE), > >+#endif > > > > /* Status Masks */ > > MODE_MASK = (7 << 0), > >@@ -288,7 +317,11 @@ typedef struct { > > l_fixedpt_t m_rectime; > > l_fixedpt_t m_xmttime; > > uint32_t m_keyid; > >+#if ENABLE_FEATURE_NTP_AUTH > >+ uint8_t m_digest[NTP_SHA1_DIGESTSIZE]; > >+#else > > uint8_t m_digest[NTP_DIGESTSIZE]; > >+#endif > > } msg_t; > > > > typedef struct { > >@@ -297,6 +330,26 @@ typedef struct { > > double d_dispersion; > > } datapoint_t; > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+enum hash_type { > >+ HASH_NONE, > >+#if ENABLE_FEATURE_NTP_AUTH_MD5 > >+ HASH_MD5, > >+#endif > >+#if ENABLE_FEATURE_NTP_AUTH_SHA1 > >+ HASH_SHA1 > >+#endif > >+}; > >+ > >+typedef struct { > >+ unsigned id; > >+ enum hash_type type; > >+ char *key; > >+ size_t key_length; > >+ size_t msg_size; > >+} key_entry_t; > >+#endif > >+ > > typedef struct { > > len_and_sockaddr *p_lsa; > > char *p_dotted; > >@@ -326,6 +379,9 @@ typedef struct { > > /* last sent packet: */ > > msg_t p_xmt_msg; > > char p_hostname[1]; > >+#if ENABLE_FEATURE_NTP_AUTH > >+ key_entry_t *key_entry; > >+#endif > > } peer_t; > > > > > >@@ -337,13 +393,25 @@ enum { > > OPT_q = (1 << 1), > > OPT_N = (1 << 2), > > OPT_x = (1 << 3), > >+#if ENABLE_FEATURE_NTP_AUTH > >+ OPT_k = (1 << 4), > >+ OPT_K = (1 << 5), > >+#endif > > /* Insert new options above this line. */ > > /* Non-compat options: */ > >+#if ENABLE_FEATURE_NTP_AUTH > >+ OPT_w = (1 << 6), > >+ OPT_p = (1 << 7), > >+ OPT_S = (1 << 8), > >+ OPT_l = (1 << 9) * ENABLE_FEATURE_NTPD_SERVER, > >+ OPT_I = (1 << 10) * ENABLE_FEATURE_NTPD_SERVER, > >+#else > > OPT_w = (1 << 4), > > OPT_p = (1 << 5), > > OPT_S = (1 << 6), > > OPT_l = (1 << 7) * ENABLE_FEATURE_NTPD_SERVER, > > OPT_I = (1 << 8) * ENABLE_FEATURE_NTPD_SERVER, > >+#endif > > /* We hijack some bits for other purposes */ > > OPT_qq = (1 << 31), > > }; > >@@ -816,8 +884,21 @@ resolve_peer_hostname(peer_t *p) > > return lsa; > > } > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+static void > >+free_key_entry(void *data) { > >+ key_entry_t *key_entry = (key_entry_t *) data; > >+ > >+ free(key_entry->key); > >+ free(key_entry); > >+} > >+ > >+static void > >+add_peers(const char *s, key_entry_t *key_entry) > >+#else > > static void > > add_peers(const char *s) > >+#endif > > { > > llist_t *item; > > peer_t *p; > >@@ -840,12 +921,18 @@ add_peers(const char *s) > > bb_error_msg("duplicate peer %s (%s)", s, p->p_dotted); > > free(p->p_lsa); > > free(p->p_dotted); > >+#if ENABLE_FEATURE_NTP_AUTH > >+ free_key_entry(p->key_entry); > >+#endif > > free(p); > > return; > > } > > } > > } > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+ p->key_entry = key_entry; > >+#endif > > llist_add_to(&G.ntp_peers, p); > > G.peer_cnt++; > > } > >@@ -870,6 +957,80 @@ do_sendto(int fd, > > return 0; > > } > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+ > >+static void > >+hash(key_entry_t *key_entry, const msg_t *msg, uint8_t *output) > >+{ > >+ size_t hash_size = sizeof(msg_t) - sizeof(msg->m_keyid) - > >sizeof(msg->m_digest); > >+ > >+ switch (key_entry->type) { > >+#if ENABLE_FEATURE_NTP_AUTH_MD5 > >+ case HASH_MD5: { > >+ md5_ctx_t ctx; > >+ > >+ md5_begin(&ctx); > >+ md5_hash(&ctx, key_entry->key, key_entry->key_length); > >+ md5_hash(&ctx, msg, hash_size); > >+ md5_end(&ctx, output); > >+ } > >+ break; > >+#endif > >+#if ENABLE_FEATURE_NTP_AUTH_SHA1 > >+ case HASH_SHA1: { > >+ sha1_ctx_t ctx; > >+ > >+ sha1_begin(&ctx); > >+ sha1_hash(&ctx, key_entry->key, key_entry->key_length); > >+ sha1_hash(&ctx, msg, hash_size); > >+ sha1_end(&ctx, output); > >+ } > >+ break; > >+#endif > >+ case HASH_NONE: > >+ bb_perror_msg_and_die("ntpd is in an invalid state."); > > There is no errno involved here so perror would print OK. You want > bb_error_msg_and_die(). > > And should be default: > > >+ break; > >+ } > >+} > >+ > >+static void > >+hash_peer(peer_t *p) > >+{ > >+ p->p_xmt_msg.m_keyid = htonl(p->key_entry->id); > >+ hash(p->key_entry, &p->p_xmt_msg, p->p_xmt_msg.m_digest); > >+} > >+ > >+static int compare_hashes(peer_t *p, const msg_t *msg) > >+{ > >+ int result = 1; > >+ > > bool result = hash(p->key_entry, msg, digest); > if (!result) > return false; > > >+ switch (p->key_entry->type) { > >+#if ENABLE_FEATURE_NTP_AUTH_MD5 > >+ case HASH_MD5: { > >+ uint8_t digest[NTP_MD5_DIGESTSIZE]; > >+ > >+ hash(p->key_entry, msg, digest); > > drop. > > >+ result = memcmp(digest, msg->m_digest, > >NTP_MD5_DIGESTSIZE); > > == 0; > > >+ } > >+ break; > >+#endif > >+#if ENABLE_FEATURE_NTP_AUTH_SHA1 > >+ case HASH_SHA1: { > >+ uint8_t digest[NTP_SHA1_DIGESTSIZE]; > >+ > >+ hash(p->key_entry, msg, digest); > > likewise. > > >+ result = memcmp(digest, msg->m_digest, > >NTP_SHA1_DIGESTSIZE); > > likewise. > >+ } > >+ break; > >+#endif > >+ case HASH_NONE: > >+ default: > >+ bb_perror_msg_and_die("ntpd is in an invalid state."); > > Whole case is redundant, comes from hash(). > > >+ } > >+ return result; > >+} > >+#endif > >+ > > static void > > send_query_to_peer(peer_t *p) > > { > >@@ -946,9 +1107,18 @@ send_query_to_peer(peer_t *p) > > */ > > p->reachable_bits <<= 1; > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+ if (p->key_entry != NULL) { > >+ hash_peer(p); > >+ } > >+ if (do_sendto(p->p_fd, /*from:*/ NULL, /*to:*/ &p->p_lsa->u.sa, > >/*addrlen:*/ p->p_lsa->len, > >+ &p->p_xmt_msg, p->key_entry == NULL ? NTP_MSGSIZE_NOAUTH : > >p->key_entry->msg_size) == -1 > > I'd check for < 0 > > > >+ ) { > >+#else > > if (do_sendto(p->p_fd, /*from:*/ NULL, /*to:*/ &p->p_lsa->u.sa, > >/*addrlen:*/ p->p_lsa->len, > > &p->p_xmt_msg, NTP_MSGSIZE_NOAUTH) == -1 > > Likewise. > > > ) { > >+#endif > > close(p->p_fd); > > p->p_fd = -1; > > /* > >@@ -1924,10 +2094,21 @@ recv_and_process_peer_pkt(peer_t *p) > > bb_perror_msg_and_die("recv(%s) error", p->p_dotted); > > } > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+ if (size != NTP_MSGSIZE_NOAUTH && size != NTP_MSGSIZE_MD5_AUTH && > >size > >!= NTP_MSGSIZE_SHA1_AUTH) { > >+ bb_error_msg("malformed packet received from %s", > >p->p_dotted); > >+ return; > >+ } > >+ if (p->key_entry != NULL && compare_hashes(p, &msg) != 0) { > > !compare... > > >+ bb_error_msg("invalid cryptographic hash received from %s", > >p->p_dotted); > >+ return; > >+ } > >+#else > > if (size != NTP_MSGSIZE_NOAUTH && size != NTP_MSGSIZE) { > > bb_error_msg("malformed packet received from %s", p->p_dotted); > > return; > > } > >+#endif > > > > if (msg.m_orgtime.int_partl != p->p_xmt_msg.m_xmttime.int_partl > > || msg.m_orgtime.fractionl != p->p_xmt_msg.m_xmttime.fractionl > >@@ -2135,7 +2316,11 @@ recv_and_process_client_pkt(void /*int fd*/) > > from = xzalloc(to->len); > > > > size = recv_from_to(G_listen_fd, &msg, sizeof(msg), MSG_DONTWAIT, > >from, &to->u.sa, to->len); > >+#if ENABLE_FEATURE_NTP_AUTH > >+ if (size != NTP_MSGSIZE_NOAUTH && size != NTP_MSGSIZE_SHA1_AUTH && > >size != NTP_MSGSIZE_MD5_AUTH) { > >+#else > > if (size != NTP_MSGSIZE_NOAUTH && size != NTP_MSGSIZE) { > >+#endif > > char *addr; > > if (size < 0) { > > if (errno == EAGAIN) > >@@ -2278,6 +2463,28 @@ recv_and_process_client_pkt(void /*int fd*/) > > * with the -g and -q options. See the tinker command for other > >options. > > * Note: The kernel time discipline is disabled with this option. > > */ > >+#if ENABLE_FEATURE_NTP_AUTH > >+static key_entry_t * > >+find_key_entry(llist_t *key_entries, unsigned id) { > >+ key_entry_t *result = NULL; > >+ llist_t *iterator = key_entries; > >+ key_entry_t *temporary; > >+ > >+ while(iterator != NULL && result == NULL) { > >+ temporary = (key_entry_t *) iterator->data; > >+ if(temporary->id == id) { > >+ result = xzalloc(sizeof(key_entry_t)); > >+ result->id = temporary->id; > >+ result->type = temporary->type; > >+ result->key = xstrdup(temporary->key); > >+ result->key_length = temporary->key_length; > >+ result->msg_size = temporary->msg_size; > > You should check if it's smaller to memcpy all and then xstrdup instead of > the individual stores. Store-merging does not do this for you, AFAIR. Might > be a good thing to have for -Os though. > > >+ } > >+ iterator = iterator->link; > >+ } > >+ return result; > >+} > >+#endif > > > > /* By doing init in a separate function we decrease stack usage > > * in main loop. > >@@ -2286,6 +2493,11 @@ static NOINLINE void ntp_init(char **argv) > > { > > unsigned opts; > > llist_t *peers; > >+#if ENABLE_FEATURE_NTP_AUTH > >+ llist_t *key_ids; > >+ llist_t *key_entries = NULL; > >+ char *key_file_path = NULL; > >+#endif > > > > srand(getpid()); > > > >@@ -2304,6 +2516,9 @@ static NOINLINE void ntp_init(char **argv) > > peers = NULL; > > opts = getopt32(argv, "^" > > "nqNx" /* compat */ > >+#if ENABLE_FEATURE_NTP_AUTH > >+ "K:*k:" > >+#endif > > "wp:*S:"IF_FEATURE_NTPD_SERVER("l") /* NOT compat */ > > IF_FEATURE_NTPD_SERVER("I:") /* compat */ > > "d" /* compat */ > >@@ -2311,6 +2526,9 @@ static NOINLINE void ntp_init(char **argv) > > "\0" > > "dd:wn" /* -d: counter; -p: list; -w implies -n */ > > IF_FEATURE_NTPD_SERVER(":Il") /* -I implies -l */ > >+#if ENABLE_FEATURE_NTP_AUTH > >+ , &key_ids, &key_file_path > >+#endif > > , &peers, &G.script_name, > > #if ENABLE_FEATURE_NTPD_SERVER > > &G.if_name, > >@@ -2341,9 +2559,79 @@ static NOINLINE void ntp_init(char **argv) > > logmode = LOGMODE_NONE; > > } > > > >+#if ENABLE_FEATURE_NTP_AUTH > >+ if (key_ids && !(opts & OPT_k)) { > >+ bb_error_msg_and_die("A key file (-k) is required to use a key > >(-K) option"); > > You can express this mutual exclusiveness in the option spec itself. > > > >+ } > >+ > >+ if (opts & OPT_k) { > >+ char *tokens[4]; > >+ parser_t *key_file_parser; > >+ int token_count; > >+ > >+ key_file_parser = config_open(key_file_path); > >+ while ((token_count = config_read(key_file_parser, tokens, 4, > >3, > >"# \t", PARSE_NORMAL | PARSE_MIN_DIE)) == 3) { > >+ enum hash_type hash_type = HASH_NONE; > >+ size_t key_length; > >+ key_entry_t *key_entry = (key_entry_t *) > >xzalloc(sizeof(key_entry_t)); > >+ > >+#if ENABLE_FEATURE_NTP_AUTH_MD5 > >+ if (strcasecmp(tokens[1], "MD5") == 0) { > > Do we really have to support all variants of upper and lowercase? > > >+ hash_type = HASH_MD5; > >+ key_entry->msg_size = NTP_MSGSIZE_MD5_AUTH; > >+ } > >+#endif > >+#if ENABLE_FEATURE_NTP_AUTH_SHA1 > >+ if (strcasecmp(tokens[1], "SHA1") == 0) { > >+ hash_type = HASH_SHA1; > >+ key_entry->msg_size = NTP_MSGSIZE_SHA1_AUTH; > >+ } > >+#endif > >+ if (hash_type == HASH_NONE) { > >+ bb_error_msg_and_die("busybox only supports MD5 and > >SHA1 > >NTP keys"); > >+ } > >+ key_entry->type = hash_type; > >+ key_entry->id = xatou_range(tokens[0], 1, MAX_KEY_NUMBER); > >+ key_length = strlen(tokens[2]); > >+ if (key_length <= 20) { > >+ key_entry->key = xstrdup(tokens[2]); > >+ key_entry->key_length = key_length; > >+ } else if ((key_length & 1) == 0) { > >+ char buffer[64]; > >+ char *key; > >+ size_t byte_count = key_length / 2; > >+ > >+ memset(buffer, 0, sizeof(buffer)); > >+ key = hex2bin(buffer, tokens[2], byte_count); > > Wouldn't you want to check if this would overflow buffer? And why don't > you allocate buffer on the heap in the first place? > > >+ if (key == NULL) { > >+ bb_error_msg_and_die("key #%d is malformed", > > "malformed key #%d" > Throughout. It's shorter. > > thanks, > > >key_entry->id); > >+ } > >+ key_entry->key = xstrdup(buffer); > >+ key_entry->key_length = byte_count; > >+ } else { > >+ bb_error_msg_and_die("key #%d is malformed", > >key_entry->id); > >+ } > >+ llist_add_to(&key_entries, key_entry); > >+ } > >+ config_close(key_file_parser); > >+ } > >+ > >+#endif > > if (peers) { > >- while (peers) > >+ while (peers) { > >+#if ENABLE_FEATURE_NTP_AUTH > >+ key_entry_t *key_entry = NULL; > >+ > >+ if (key_ids) { > >+ int key_id = xatou_range(llist_pop(&key_ids), 1, > >MAX_KEY_NUMBER); > >+ > >+ key_entry = find_key_entry(key_entries, key_id); > >+ } > >+ add_peers(llist_pop(&peers), key_entry); > >+#else > > add_peers(llist_pop(&peers)); > >+#endif > >+ } > > } > > #if ENABLE_FEATURE_NTPD_CONF > > else { > >@@ -2353,7 +2641,11 @@ static NOINLINE void ntp_init(char **argv) > > parser = config_open("/etc/ntp.conf"); > > while (config_read(parser, token, 3, 1, "# \t", PARSE_NORMAL)) { > > if (strcmp(token[0], "server") == 0 && token[1]) { > >+#if ENABLE_FEATURE_NTP_AUTH > >+ add_peers(token[1], NULL); > >+#else > > add_peers(token[1]); > >+#endif > > continue; > > } > > bb_error_msg("skipping %s:%u: unimplemented command '%s'", > >@@ -2394,6 +2686,9 @@ static NOINLINE void ntp_init(char **argv) > > | (1 << SIGCHLD) > > , SIG_IGN > > ); > >+#if ENABLE_FEATURE_NTP_AUTH > >+ llist_free(key_entries, free_key_entry); > >+#endif > > } > > > >int ntpd_main(int argc UNUSED_PARAM, char **argv) > >MAIN_EXTERNALLY_VISIBLE; > > > >On Thu, Oct 11, 2018 at 11:22 AM Bernhard Reutner-Fischer < > >[email protected]> wrote: > > > >> On 11 October 2018 16:41:13 CEST, Brandon Enochs > ><[email protected]> > >> wrote: > >> >OK, I added config support to disable NTP authentication and used > >> >unconditional frees. > >> > >> hash() and compare_hash() look redundant. Can you use the former to > >> implement the latter? > >> > >> free_keys could maybe use llist_free() (sp?) if you'd change the keys > >to > >> be list elts? > >> > >> And please always provide bloat-o-meter output: > >> make baseline (with pristine master) > >> ./scripts/bloat-o-meter.py old yours, see make bloat-check (IIRC). > >> > >> thanks, > >> > >
_______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
