Add support for NTP authentication

Bernhard Reutner-Fischer rep.dot.nop at gmail.com
Fri Oct 12 07:49:33 UTC 2018


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,
>>



More information about the busybox mailing list