Re: [PATCH v2] Ntpd config file support
How about ... for instance merging this thing ... ? On Mon, Mar 24, 2014 at 3:13 AM, Isaac Dunham ibid...@gmail.com wrote: On Sun, Mar 23, 2014 at 03:06:08PM +0100, Denys Vlasenko wrote: On Saturday 22 March 2014 23:46, Isaac Dunham wrote: On Sat, Mar 22, 2014 at 08:40:48PM +0100, Harald Becker wrote: Hi Isaac ! Your program will fail on lines starting with the word server (eg. serverxyz), that is it does not check for clear word boundary and gives wrong results in that case. ...which are not legitimate entries in ntp.conf. My aim is to parse a correct ntp.conf, and not cause security problems on incorrect ones. bbox has config parsing routines to avoid coding this again and again. How about this? function old new delta add_peers - 98 +98 packed_usage 29470 29511 +41 ntp_init 407 428 +21 pw_encrypt14 27 +13 -- (add/remove: 1/0 grow/shrink: 3/0 up/down: 173/0) Total: 173 bytes Definitely better than mine. I'm inclined to agree with Harald as to the skipping messages; _if_ they are included, they should happen in verbose mode only. Any idea why pw_encrypt is changing? Now, regarding the default n vs default y/whether compat should always be/is always on... I'm aware of at least two compatability options that default to the smaller less compatible version: CONFIG_MODPROBE_SMALL is certainly not more compatible (-ablD being missing in the small version.) CONFIG_EXTRA_COMPAT is default n, and that is a good thing. All the people I've see asking for the feature use distro binaries, rather than defconfig. That said, I take no position on whether to make this default n. Thanks, Isaac Dunham ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] Ntpd config file support
On Mon, Mar 24, 2014 at 2:15 AM, Harald Becker ra...@gmx.de wrote: + bb_error_msg(skipping %s:%u: unimplemented command '%s', + /etc/ntp.conf, parser-lineno, token[0] + ); Bad idea to do this! The default ntpd.conf contains other options than just server lines. All those lines will now produce skipping messages on startup. Multiple messages on any startup, probably. Most users will wonder what has gone wrong and look for something suspicious, and will ask why BB throws those errors. They _should_ wonder. If they have other lines there, they probably expect them to have some effect. IMO not so good to simply get compatibility. With those messages nobody can really use the default ntpd.conf for BB without modifications. Your intention? My thinking is that users need to know that their ntpd ignored some instructions. ... *AND* I'm still insisting in having this config file code not included in standard build binaries. As others also stated we do not need this feature (examples have bean given) Logically, you should be objecting to a few dozens of other default y config options since we don't need them too. What rules do you propose to be used in deciding which option should default to y or n? ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] Ntpd config file support
On Saturday 22 March 2014 23:46:44 Isaac Dunham wrote:
On Sat, Mar 22, 2014 at 08:40:48PM +0100, Harald Becker wrote:
Hi Isaac !
Your program will fail on lines starting with the word server
(eg. serverxyz), that is it does not check for clear word
boundary and gives wrong results in that case.
...which are not legitimate entries in ntp.conf.
My aim is to parse a correct ntp.conf, and not cause security problems
on incorrect ones.
while (cbuf[i] 35) i++;
Unwise to do this in a not poor ASCII environment, as most
systems are nowadays. This way you allow unprintable and any
kind of illegal characters in time server addresses.
Fixing.
The fix expects chars exclusively in the set [-.:0-9a-zA-Z], which all
valid hostnames and IP addresses (ipv4/ipv6) have.
... and what
about buffer overflow? Won't this loop then run to unknown
locations?
Not possible. i is size_t, and getline() is _always_ \0 terminated.
However, the previous loop did have a potential buffer overrun if the
line ended prematurely:
server \n\0 would result in it walking over the end and writing 0 to the
first character less than 36 after a sequence of chars greater than
35 ('#')...
Beside this: Make it a default NO configuration, not being
included in binaries build from standard options.
OK.
(Denys gets the final say on that, though.)
Here's a version that has the issues mentioned fixed, and removes the 10
byte overhead.
It accepts peer as well as server, and runs 320 bytes.
Thanks,
Isaac Dunham
HI,
couldn't this parser use bb's parse infrastructure in libbb/parse_config.c?
///config:Typical usage of parse API:
config: char *t[3];
config: parser_t *p = config_open(filename);
config: while (config_read(p, t, 3, 0, delimiters, flags)) { //
1..3 tokens
config: bb_error_msg(TOKENS: '%s''%s''%s', t[0],
t[1], t[2]);
config: }
config: config_close(p);
I think /etc/ntp.conf string should be moved to libbb.h.
Ciao,
Tito
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] Ntpd config file support
On Saturday 22 March 2014 23:46, Isaac Dunham wrote:
On Sat, Mar 22, 2014 at 08:40:48PM +0100, Harald Becker wrote:
Hi Isaac !
Your program will fail on lines starting with the word server
(eg. serverxyz), that is it does not check for clear word
boundary and gives wrong results in that case.
...which are not legitimate entries in ntp.conf.
My aim is to parse a correct ntp.conf, and not cause security problems
on incorrect ones.
bbox has config parsing routines to avoid coding this again and again.
How about this?
function old new delta
add_peers - 98 +98
packed_usage 29470 29511 +41
ntp_init 407 428 +21
pw_encrypt14 27 +13
--
(add/remove: 1/0 grow/shrink: 3/0 up/down: 173/0) Total: 173 bytes
diff -ad -urpN busybox.5/networking/Config.src busybox.6/networking/Config.src
--- busybox.5/networking/Config.src 2014-02-09 09:48:42.0 +0100
+++ busybox.6/networking/Config.src 2014-03-23 14:35:04.0 +0100
@@ -664,6 +664,14 @@ config FEATURE_NTPD_SERVER
Make ntpd usable as a NTP server. If you disable this option
ntpd will be usable only as a NTP client.
+config FEATURE_NTPD_CONF
+ bool Make ntpd understand /etc/ntp.conf
+ default y
+ depends on NTPD
+ help
+ Make ntpd look in /etc/ntp.conf for peers. Only server address
+ is supported.
+
config PSCAN
bool pscan
default y
diff -ad -urpN busybox.5/networking/ntpd.c busybox.6/networking/ntpd.c
--- busybox.5/networking/ntpd.c 2014-02-09 15:33:14.0 +0100
+++ busybox.6/networking/ntpd.c 2014-03-23 15:01:55.0 +0100
@@ -42,6 +42,13 @@
//usage: )
//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: IF_FEATURE_NTPD_CONF(
+//usage: \n If -p is not given, read /etc/ntp.conf
+//usage: )
+
+// -l and -p options are not compatible with standard ntpd:
+// it has them as -l logfile and -p pidfile.
+// -S and -w are not compat either, standard ntpd has no such opts.
#include libbb.h
#include math.h
@@ -730,7 +737,7 @@ reset_peer_stats(peer_t *p, double offse
}
static void
-add_peers(char *s)
+add_peers(const char *s)
{
peer_t *p;
@@ -2087,14 +2094,34 @@ static NOINLINE void ntp_init(char **arg
d /* compat */
46aAbgL, /* compat, ignored */
peers, G.script_name, G.verbose);
- if (!(opts (OPT_p|OPT_l)))
- bb_show_usage();
+
// if (opts OPT_x) /* disable stepping, only slew is allowed */
// G.time_was_stepped = 1;
if (peers) {
while (peers)
add_peers(llist_pop(peers));
- } else {
+ }
+#if ENABLE_FEATURE_NTPD_CONF
+ else {
+ parser_t *parser;
+ char *token[3];
+
+ parser = config_open(/etc/ntp.conf);
+ while (config_read(parser, token, 3, 1, # \t, PARSE_NORMAL)) {
+ if (strcmp(token[0], server) == 0 token[1]) {
+ add_peers(token[1]);
+ continue;
+ }
+ bb_error_msg(skipping %s:%u: unimplemented command
'%s',
+ /etc/ntp.conf, parser-lineno, token[0]
+ );
+ }
+ config_close(parser);
+ }
+#endif
+ if (G.peer_cnt == 0) {
+ if (!(opts OPT_l))
+ bb_show_usage();
/* -l but no peers: stratum 1 server mode */
G.stratum = 1;
}
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] Ntpd config file support
Hi Denys ! + bb_error_msg(skipping %s:%u: unimplemented command '%s', + /etc/ntp.conf, parser-lineno, token[0] + ); Bad idea to do this! The default ntpd.conf contains other options than just server lines. All those lines will now produce skipping messages on startup. Multiple messages on any startup, probably. Most users will wonder what has gone wrong and look for something suspicious, and will ask why BB throws those errors. IMO not so good to simply get compatibility. With those messages nobody can really use the default ntpd.conf for BB without modifications. Your intention? ... *AND* I'm still insisting in having this config file code not included in standard build binaries. As others also stated we do not need this feature (examples have bean given) and this it's not more than a paranoid kind of compatibility. Shall those maintainers step through the options and enable the feature, who really like to have it, not the many others live with extra bloat. -- Harald ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: [PATCH v2] Ntpd config file support
On Sun, Mar 23, 2014 at 03:06:08PM +0100, Denys Vlasenko wrote: On Saturday 22 March 2014 23:46, Isaac Dunham wrote: On Sat, Mar 22, 2014 at 08:40:48PM +0100, Harald Becker wrote: Hi Isaac ! Your program will fail on lines starting with the word server (eg. serverxyz), that is it does not check for clear word boundary and gives wrong results in that case. ...which are not legitimate entries in ntp.conf. My aim is to parse a correct ntp.conf, and not cause security problems on incorrect ones. bbox has config parsing routines to avoid coding this again and again. How about this? function old new delta add_peers - 98 +98 packed_usage 29470 29511 +41 ntp_init 407 428 +21 pw_encrypt14 27 +13 -- (add/remove: 1/0 grow/shrink: 3/0 up/down: 173/0) Total: 173 bytes Definitely better than mine. I'm inclined to agree with Harald as to the skipping messages; _if_ they are included, they should happen in verbose mode only. Any idea why pw_encrypt is changing? Now, regarding the default n vs default y/whether compat should always be/is always on... I'm aware of at least two compatability options that default to the smaller less compatible version: CONFIG_MODPROBE_SMALL is certainly not more compatible (-ablD being missing in the small version.) CONFIG_EXTRA_COMPAT is default n, and that is a good thing. All the people I've see asking for the feature use distro binaries, rather than defconfig. That said, I take no position on whether to make this default n. Thanks, Isaac Dunham ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
[PATCH v2] Ntpd config file support
On Sat, Mar 22, 2014 at 08:40:48PM +0100, Harald Becker wrote:
Hi Isaac !
Your program will fail on lines starting with the word server
(eg. serverxyz), that is it does not check for clear word
boundary and gives wrong results in that case.
...which are not legitimate entries in ntp.conf.
My aim is to parse a correct ntp.conf, and not cause security problems
on incorrect ones.
while (cbuf[i] 35) i++;
Unwise to do this in a not poor ASCII environment, as most
systems are nowadays. This way you allow unprintable and any
kind of illegal characters in time server addresses.
Fixing.
The fix expects chars exclusively in the set [-.:0-9a-zA-Z], which all
valid hostnames and IP addresses (ipv4/ipv6) have.
... and what
about buffer overflow? Won't this loop then run to unknown
locations?
Not possible. i is size_t, and getline() is _always_ \0 terminated.
However, the previous loop did have a potential buffer overrun if the
line ended prematurely:
server \n\0 would result in it walking over the end and writing 0 to the
first character less than 36 after a sequence of chars greater than
35 ('#')...
Beside this: Make it a default NO configuration, not being
included in binaries build from standard options.
OK.
(Denys gets the final say on that, though.)
Here's a version that has the issues mentioned fixed, and removes the 10
byte overhead.
It accepts peer as well as server, and runs 320 bytes.
Thanks,
Isaac Dunham
From 6c825a8e6c83715306f82540e540bac8d48af64f Mon Sep 17 00:00:00 2001
From: Isaac Dunham ibid...@gmail.com
Date: Sat, 22 Mar 2014 11:44:30 -0700
Subject: [PATCH] ntpd: parse /etc/ntp.conf
This is a rudimentary parser for /etc/ntp.conf; it only looks for
lines like this:
server some.pool.ntp.org
peer 0.0.0.0
All options are ignored.
+0 bytes when disabled.
Enabled, it's +320 bytes with gcc 4.4 and glibc 2.11.3:
function old new delta
ntp_init 406 593+187
add_peers - 107+107
.rodata 141382 141402 +20
packed_usage 29386 29392 +6
--
(add/remove: 1/0 grow/shrink: 3/0 up/down: 320/0) Total: 320 bytes
text data bss dec hex filename
759413 2059 9020 770492 bc1bc busybox_old
759727 2059 9020 770806 bc2f6 busybox_unstripped
---
networking/Config.src |8
networking/ntpd.c | 33 +
2 files changed, 41 insertions(+)
diff --git a/networking/Config.src b/networking/Config.src
index ca0ddcd..ec50134 100644
--- a/networking/Config.src
+++ b/networking/Config.src
@@ -664,6 +664,14 @@ config FEATURE_NTPD_SERVER
Make ntpd usable as a NTP server. If you disable this option
ntpd will be usable only as a NTP client.
+config FEATURE_NTPD_CONF
+ bool Make ntpd understand /etc/ntp.conf
+ default n
+ depends on NTPD
+ help
+ Make ntpd look in /etc/ntp.conf for peers. Only server address
+ and peer address are supported.
+
config PSCAN
bool pscan
default y
diff --git a/networking/ntpd.c b/networking/ntpd.c
index 44592ce..b65bb91 100644
--- a/networking/ntpd.c
+++ b/networking/ntpd.c
@@ -42,6 +42,9 @@
//usage: )
//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: IF_FEATURE_NTPD_CONF(
+//usage: \nIf -p is not specified, look in /etc/ntp.conf
+//usage: )
#include libbb.h
#include math.h
@@ -2087,14 +2090,44 @@ static NOINLINE void ntp_init(char **argv)
d /* compat */
46aAbgL, /* compat, ignored */
peers, G.script_name, G.verbose);
+#if ENABLE_FEATURE_NTPD_CONF
+#else
if (!(opts (OPT_p|OPT_l)))
bb_show_usage();
+#endif
// if (opts OPT_x) /* disable stepping, only slew is allowed */
// G.time_was_stepped = 1;
if (peers) {
while (peers)
add_peers(llist_pop(peers));
} else {
+#if ENABLE_FEATURE_NTPD_CONF
+ size_t bsiz = 0, i=0;
+ char *buf = 0, *cbuf;
+ FILE *stream = fopen(/etc/ntp.conf,r);
+
+ while (stream !errno) {
+ getline(buf, bsiz, stream);
+ cbuf = buf;
+ if (!strncmp(buf, server, 6)) cbuf += 6;
+ if (!strncmp(buf, peer, 4)) cbuf += 4;
+ if (cbuf buf){
+while (cbuf[0] = ' ' cbuf[0]) cbuf++;
+while (cbuf[i]=='.' || cbuf[i]=='-' ||
+cbuf[i]==':' || isalnum(cbuf[i]))
+ i++;
+cbuf[i] = 0;
+add_peers(xstrdup(cbuf));
+ }
+ }
+ if (buf) free(buf);
+ if (stream) fclose(stream);
+ errno = 0;
+ }
+ if (!(opts OPT_l) !G.peer_cnt) {
+ bb_show_usage();
+ } else if (!G.peer_cnt) {
+#endif
/* -l but no peers: stratum 1 server mode */
G.stratum = 1;
}
--
1.7.10.4
___
busybox mailing list
busybox@busybox.net
