Add support for NTP authentication

Brandon Enochs enochs.brandon at gmail.com
Fri Oct 12 14:08:04 UTC 2018


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 <
rep.dot.nop at gmail.com> wrote:

> On 11 October 2018 18:38:44 CEST, Brandon Enochs <enochs.brandon at gmail.com>
> 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 <
> >rep.dot.nop at gmail.com> wrote:
> >
> >> On 11 October 2018 16:41:13 CEST, Brandon Enochs
> ><enochs.brandon at gmail.com>
> >> 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,
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20181012/773322fe/attachment-0001.html>


More information about the busybox mailing list