diff mbox

[OpenWrt-Devel,v2] base-files: init/sysfixtime - exclude dnsmasq.time

Message ID 20150922175205.GA24405@medion.lan
State Changes Requested
Headers show

Commit Message

Bastian Bittorf Sept. 22, 2015, 5:52 p.m. UTC
dnsmasq maintains dnsmasq.time across reboots and uses it as a means of
determining if current time is good enough to validate dnssec time
stamps.  By including /etc/dnsmasq.time as a time source for sysfixtime,
the mechanism was effectively defeated because time was set to the last
time that dnsmasq considered current even though that time is in
the past.  Since that time is out of date, dns(sec) resolution would
fail thus defeating any ntp based mechanisms for setting the clock
correctly.

In theory the process is defeated by any files in /etc that are newer
than /etc/dnsmasq.time however dnsmasq now updates the file's timestamp
on process TERM so hopefully /etc/dnsmasq.time is the latest file
timestamp in /etc as part of openWrt shutdown/reboot.

Either way, including /etc/dnsmasq.time as a time source for sysfixtime
is not helpful.

for safing time we dont read the filedate of every file,
but only the newest in each subdirectory of /etc and sort them.
this speeds up from 1.72 sec to 0.51 sec on my router.

v1 - original concept from Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
v2 - speedup + update copyright date

Signed-off-by: Bastian Bittorf <bittorf@bluebottle.com>
---
 package/base-files/files/etc/init.d/sysfixtime |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Yousong Zhou Sept. 23, 2015, 2:42 a.m. UTC | #1
On 23 September 2015 at 01:52, Bastian Bittorf <bittorf@bluebottle.com> wrote:
> dnsmasq maintains dnsmasq.time across reboots and uses it as a means of
> determining if current time is good enough to validate dnssec time
> stamps.  By including /etc/dnsmasq.time as a time source for sysfixtime,
> the mechanism was effectively defeated because time was set to the last
> time that dnsmasq considered current even though that time is in
> the past.  Since that time is out of date, dns(sec) resolution would
> fail thus defeating any ntp based mechanisms for setting the clock
> correctly.
>
> In theory the process is defeated by any files in /etc that are newer
> than /etc/dnsmasq.time however dnsmasq now updates the file's timestamp
> on process TERM so hopefully /etc/dnsmasq.time is the latest file
> timestamp in /etc as part of openWrt shutdown/reboot.
>

In theory, a security sensitive mechanism's dependence on a
non-reliable timestamp file with access permission nobody:nogroup
makes little sense to me.  How about that we do --dnssec-no-timecheck
on dnsmasq startup time and notify it of the system time change from
ntpd hotplug script?

Another idea would be to delegate timestamp update task to a specific
service program like ntpd or procd and later on system startup we set
system time from the specific file.

> Either way, including /etc/dnsmasq.time as a time source for sysfixtime
> is not helpful.

Agree.

                yousong

>
> for safing time we dont read the filedate of every file,
> but only the newest in each subdirectory of /etc and sort them.
> this speeds up from 1.72 sec to 0.51 sec on my router.
>
> v1 - original concept from Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
> v2 - speedup + update copyright date
>
> Signed-off-by: Bastian Bittorf <bittorf@bluebottle.com>
> ---
>  package/base-files/files/etc/init.d/sysfixtime |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/package/base-files/files/etc/init.d/sysfixtime b/package/base-files/files/etc/init.d/sysfixtime
> index 4010e06..b3e3862 100755
> --- a/package/base-files/files/etc/init.d/sysfixtime
> +++ b/package/base-files/files/etc/init.d/sysfixtime
> @@ -1,11 +1,20 @@
>  #!/bin/sh /etc/rc.common
> -# Copyright (C) 2013-2014 OpenWrt.org
> +# Copyright (C) 2013-2015 OpenWrt.org
>
>  START=00
>
>  boot() {
>         local curtime="$(date +%s)"
> -       local maxtime="$(find /etc -type f -exec date -r {} +%s \; | sort -nr | head -n1)"
> +       local maxtime="$(maxtime)"
> +
>         [ $curtime -lt $maxtime ] && date -s @$maxtime
>  }
>
> +maxtime() {
> +       local dir file
> +
> +       find /etc -type d | while read dir; do
> +               file="$dir/$( ls -1t "$dir" | head -n1 )"
> +               [ -e "$file" -a "$file" != '/etc/dnsmasq.time' ] && date -r "$file" +%s
> +       done | sort -nr | head -n1
> +}



> --
> 1.7.10.4
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Bastian Bittorf Sept. 23, 2015, 6:13 a.m. UTC | #2
* Yousong Zhou <yszhou4tech@gmail.com> [23.09.2015 07:58]:
> In theory, a security sensitive mechanism's dependence on a
> non-reliable timestamp file with access permission nobody:nogroup
> makes little sense to me.  How about that we do --dnssec-no-timecheck
> on dnsmasq startup time and notify it of the system time change from
> ntpd hotplug script?

this sounds good to me, but will be another patch.

should we drop this patch completely or does it still
make sense to deny reading '/etc/dnsmasq.time'?

and: of which hotplug script you are talking about?
find /etc/hotplug.d -name '*ntp*'
= empty

> Another idea would be to delegate timestamp update task to a specific
> service program like ntpd or procd and later on system startup we set
> system time from the specific file.

unsure if this is overkill, just for 1 service.

thanks for feedback - bye, bastian
Kevin Darbyshire-Bryant Sept. 23, 2015, 9:05 a.m. UTC | #3
On 23/09/15 07:13, Bastian Bittorf wrote:
> * Yousong Zhou <yszhou4tech@gmail.com> [23.09.2015 07:58]:
>> In theory, a security sensitive mechanism's dependence on a
>> non-reliable timestamp file with access permission nobody:nogroup
>> makes little sense to me.  How about that we do --dnssec-no-timecheck
>> on dnsmasq startup time and notify it of the system time change from
>> ntpd hotplug script?
> this sounds good to me, but will be another patch.
>
> should we drop this patch completely or does it still
> make sense to deny reading '/etc/dnsmasq.time'?
In my humble opinion the startup efficiency improvements alone are worth
having and in the short term at least, dnsmasq should not be being fed
with its own timestamp.

There will be another email in reply to the other issues.

Cheers,

Kevin
Steven Barth Sept. 23, 2015, 9:12 a.m. UTC | #4
Using --dnssec-no-timecheck is impractical since it reacts to SIGHUP which
is already overloaded and might be triggered by e.g. config changes.

Btw. an ntp hotplug infrastructure exists:
https://dev.openwrt.org/changeset/43421

Please also consider that some devices have an RTC, so disabling timecheck
indiscriminately at startup might not be ideal either.



Cheers,

Steven
Kevin Darbyshire-Bryant Sept. 23, 2015, 10:18 a.m. UTC | #5
On 23/09/15 03:42, Yousong Zhou wrote:
> On 23 September 2015 at 01:52, Bastian Bittorf <bittorf@bluebottle.com> wrote:
>> dnsmasq maintains dnsmasq.time across reboots and uses it as a means of
>> determining if current time is good enough to validate dnssec time
>> stamps.  By including /etc/dnsmasq.time as a time source for sysfixtime,
>> the mechanism was effectively defeated because time was set to the last
>> time that dnsmasq considered current even though that time is in
>> the past.  Since that time is out of date, dns(sec) resolution would
>> fail thus defeating any ntp based mechanisms for setting the clock
>> correctly.
>>
>> In theory the process is defeated by any files in /etc that are newer
>> than /etc/dnsmasq.time however dnsmasq now updates the file's timestamp
>> on process TERM so hopefully /etc/dnsmasq.time is the latest file
>> timestamp in /etc as part of openWrt shutdown/reboot.
>>
I'm glad there's so much interest in this topic.  I have to declare a
bias/interest in this because I was the guy who persuaded Simon
(dnsmasq) to implement the timestamp check file option.  The motivation
was to provide an easier path for users of dnsmasq to switch on dnssec
and have it 'just work', well hopefully.  The existing
--dnssec-no-timecheck has some practical difficulties and at the time I
was marginally involved in the 'tomato' project which just proved too
difficult to adapt to use '--dnssec-no-timecheck'
> In theory, a security sensitive mechanism's dependence on a
> non-reliable timestamp file with access permission nobody:nogroup
> makes little sense to me.  How about that we do --dnssec-no-timecheck
> on dnsmasq startup time and notify it of the system time change from
> ntpd hotplug script?
If the concern is someone mangling the timestamp file then a relatively
easy solution is to create a dnsmasq user/group, have dnsmasq drop to
that user rather than nobody, create a directory solely for the
timestamp file '/etc/dnsmasq.d' (perm 600) and store the timestamp file
there out of everyone else's reach.

Manipulating the timestamp file and system time before dnsmasq start has
the following effects as I understand it:

1) system time equal or ahead of timestamp file - dnsmasq considers
system time correct (updates timestamp file) and will now check dnssec
signature timestamps.  If the system time is not actually 'internet
time' (within a tolerance of which I'm unclear) and dnsmasq is using
'dnssec-check-unsigned' then ALL dns resolution will fail (everything is
considered BOGUS) - arguably a fail safe in that it just stops!  This
become really problematic when trying to resolve name to ip addresses of
ntp servers ;-)  Manual workaround, stop dnsmasq, restart sysntpd, wait
10 seconds, start dnsmasq, get on with life :-)

2) system time behind timestamp file - dnsmasq considers system time
'incorrect' and doesn't check dnssec signature timestamps, yet. 
dns(sec) resolution will work, though timestamps are not checked.  The
full implications of this I don't understand (at least I'm honest) 
dnsmasq compares system time and filestamp time for each lookup and when
it detects system time > filestamp time starts checking dnssec signature
timestamps (see 1) 

In essence dnsmasq tries to maintain a 'last known good time' timestamp
file.  Moving that timestamp file out of everyone else's reach is
probably a good idea as this removes one source of manipulation.  This
'just' leaves the problem of someone effectively manipulating system
time at boot via the 'sysfixtime' script.  They can only move the time
forward and if they go too far from internet time then name resolution
will stop (see 1 above)

--dnssec-no-timecheck on the other hand assumes time is incorrect and so
by default doesn't check signature timestamps.  Signature checking is
enabled by sending SIGHUP (which does a few other things as well)  A
problem with this approach is handling dnsmasq re-starts
(crashes)/reconfigures.  If by default dnsmasq is always started with
'--dnssec-no-timecheck' then a process restart will put it back into
'less secure' mode until something notices and sends another SIGHUP.  I
guess ntpd could be patched to create a flag that says 'time has been
set' and the dnsmasq startup process modified to include/exclude
'--dnssec-no-timecheck' as required.  Care needs to be taken to remove
this flag at reboot.

>
> Another idea would be to delegate timestamp update task to a specific
> service program like ntpd or procd and later on system startup we set
> system time from the specific file.
>
>> Either way, including /etc/dnsmasq.time as a time source for sysfixtime
>> is not helpful.
> Agree.
:-)  Including it almost guaranteed dnsmasq fell into the trap mentioned
in 1)
>
>                 yousong
>
Bastian Bittorf Sept. 23, 2015, 11:37 a.m. UTC | #6
* Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> [23.09.2015 12:21]:

[...]

> signature timestamps.  If the system time is not actually 'internet
> time' (within a tolerance of which I'm unclear) and dnsmasq is using
> 'dnssec-check-unsigned' then ALL dns resolution will fail (everything is
> considered BOGUS) - arguably a fail safe in that it just stops!  This
> become really problematic when trying to resolve name to ip addresses of
> ntp servers ;-)

good point. so it makes sense to startup dnsmasq without dnssec strict
checks and reconfigure it when NTP was successful?

it would be really helpful if ntp can mark (with a file) somehow, that
time is 'good'. At least the returncode indicates that:

root@box:~ /usr/sbin/ntpd -q -n -p 1.openwrt.pool.ntp.org
root@box:~ echo $?
0

(it is e.g. 143 when it fails)

so if first timesetting is done, it has to trigger dnsmasq...

bye, bastian
Justin Vallon Sept. 26, 2015, 5:26 a.m. UTC | #7
On 9/22/15 1:52 PM, Bastian Bittorf wrote:
> +maxtime() {
> +	local dir file
> +
> +	find /etc -type d | while read dir; do
> +		file="$dir/$( ls -1t "$dir" | head -n1 )"
> +		[ -e "$file" -a "$file" != '/etc/dnsmasq.time' ] && date -r "$file" +%s
> +	done | sort -nr | head -n1
> +}
It appears that if /etc/dnsmasq.time is the newest file in /etc, it will
"shadow" the next-newest file.  "ls -lt /etc | head -n1" will be
/etc/dnsmasq.time, and no other /etc/* files will be checked.  The new
code will also check the mod time of directories and symlinks, unlike
the previous implementation.

I was experimenting a little.  The fastest is probably "ls -t $(find
/etc -type f) | head -1", but that does not behave well if there are too
many files in /etc (too many args).  Using xargs to split the ls
invocations is possible, but then each xargs invocation needs to take
the max separately.  The fourth sample below does this.

"test" has: test $a -ot $b ($a older than $b).  This makes the script
reasonably straightforward (only checks files, ignores arbitrary paths)
and pretty fast in my test:

local file newest
for file in $( find /etc -type f ! -path /etc/dnsmasq.time ) ; do
    [ -z "$newest" -o "$newest" -ot "$file"] && newest=$file
done
echo $newest

# time sh -c 'find /etc -type f -exec date -r {} +%s \; | sort -nr |
head -n1'
1443239314
real    0m 0.77s
user    0m 0.17s
sys     0m 0.58s
# time sh -c 'date -r $(ls -t $( find /etc -type f ) | head -1) +%s'   
# fails if /etc contains too many files
1443239314
real    0m 0.04s
user    0m 0.02s
sys     0m 0.03s
# time sh -c 'local file newest; for file in $(find /etc -type f ! -path
/etc/dnsmasq.time) ; do [ -z "$newest" -o "$newest" -ot "$file" ] &&
newest=$file ; done ; [ "$newest" ] && date -r "$newest" +%s'
1443239314
real    0m 0.06s
user    0m 0.03s
sys     0m 0.03s
# time sh -c 'find /etc -type f ! -path /etc/dnsmasq.time | xargs sh -c
'\''ls -t "$@" | head -1'\'' - | while read p ; do date -r $p +%s ; done
| sort -nr | head -1'
1443239314
real    0m 0.07s
user    0m 0.02s
sys     0m 0.06s

Feel free to use or ignore.
Yousong Zhou Sept. 30, 2015, 2:22 a.m. UTC | #8
Hi, hope this comment is not too late :)

On 23 September 2015 at 17:12, Steven Barth <cyrus@openwrt.org> wrote:
> Using --dnssec-no-timecheck is impractical since it reacts to SIGHUP which
> is already overloaded and might be triggered by e.g. config changes.
>

Agree.  I did not check the source code, but it's bad design if it is
indeed the case that dnssec time check will be enabled on any
condition on receiving SIGHUP signal which is already there for config
reload.

> Btw. an ntp hotplug infrastructure exists:
> https://dev.openwrt.org/changeset/43421
>
> Please also consider that some devices have an RTC, so disabling timecheck
> indiscriminately at startup might not be ideal either.
>

To be honest, I have little prior experience with the DNSSEC protocol
details.  But considering principles of least privilege and smaller
attack surface, the DNSSEC time check SHOULD be disabled by default
when no reliable time source is available.  The timestamp file is more
like a compromise and compromise is a negative word when talking about
security.

Then how do you guys think about the following proposal

 - An option like "dnssec_time_check" can be provided to let users
switch it on explicitly if they know what's the effect will be and are
okay with it
 - If no option was explicitly specified, then we might check the
availability of rtc in service script and enable the time check if
it's there

Regards,

                yousong

>
>
> Cheers,
>
> Steven
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Kevin Darbyshire-Bryant Sept. 30, 2015, 9:21 a.m. UTC | #9
On 30/09/15 03:22, Yousong Zhou wrote:
> Hi, hope this comment is not too late :)

To be blunt I've given up. There's a 'companion' patch
https://patchwork.ozlabs.org/patch/522968/ which also is mentally in the
same state.

Ultimately if ntpd can be persuaded to set a flag when it considers time
valid, then dnsmasq can be started with '--dnssec-no-timecheck' when
invalid and without '--dnssec-no-timecheck' when it is valid
irrespective of local (correctly?) set RTC.   There's clearly a hole in
the sense that if dnssec is sent SIGHUP whilst -dnssec-no-timecheck is
included AND the time hasn't been set correctly then name resolution
will stop.  Removing the 'SIGHUP' awareness of dnssec-no-timecheck from
what I remember of the code would be a trivial patch.  ntpd should
completely restart dnsmasq to ensure its cache is completely security
validated.

This would also cope with the case where sysfixtime has picked up a
'naughty' file and set the time far in the future, though now I'm
mentally having issues with time going backwards.

I quote pertinent options from the dnsmasq man page for reference:

*--dnssec*
    Validate DNS replies and cache DNSSEC data. When forwarding DNS
    queries, dnsmasq requests the DNSSEC records needed to validate the
    replies. The replies are validated and the result returned as the
    Authenticated Data bit in the DNS packet. In addition the DNSSEC
    records are stored in the cache, making validation by clients more
    efficient. Note that validation by clients is the most secure DNSSEC
    mode, but for clients unable to do validation, use of the AD bit set
    by dnsmasq is useful, provided that the network between the dnsmasq
    server and the client is trusted. Dnsmasq must be compiled with
    HAVE_DNSSEC enabled, and DNSSEC trust anchors provided, see
    *--trust-anchor.* Because the DNSSEC validation process uses the
    cache, it is not permitted to reduce the cache size below the
    default when DNSSEC is enabled. The nameservers upstream of dnsmasq
    must be DNSSEC-capable, ie capable of returning DNSSEC records with
    data. If they are not, then dnsmasq will not be able to determine
    the trusted status of answers. In the default mode, this menas that
    all replies will be marked as untrusted. If
    *--dnssec-check-unsigned* is set and the upstream servers don't
    support DNSSEC, then DNS service will be entirely broken. 

*--dnssec-check-unsigned*
    As a default, dnsmasq does not check that unsigned DNS replies are
    legitimate: they are assumed to be valid and passed on (without the
    "authentic data" bit set, of course). This does not protect against
    an attacker forging unsigned replies for signed DNS zones, but it is
    fast. If this flag is set, dnsmasq will check the zones of unsigned
    replies, to ensure that unsigned replies are allowed in those zones.
    The cost of this is more upstream queries and slower performance.
    See also the warning about upstream servers in the section on
    *--dnssec* 
*--dnssec-no-timecheck*
    DNSSEC signatures are only valid for specified time windows, and
    should be rejected outside those windows. This generates an
    interesting chicken-and-egg problem for machines which don't have a
    hardware real time clock. For these machines to determine the
    correct time typically requires use of NTP and therefore DNS, but
    validating DNS requires that the correct time is already known.
    Setting this flag removes the time-window checks (but not other
    DNSSEC validation.) only until the dnsmasq process receives SIGHUP.
    The intention is that dnsmasq should be started with this flag when
    the platform determines that reliable time is not currently
    available. As soon as reliable time is established, a SIGHUP should
    be sent to dnsmasq, which enables time checking, and purges the
    cache of DNS records which have not been throughly checked. 
*--dnssec-timestamp=<path>*
    Enables an alternative way of checking the validity of the system
    time for DNSSEC (see --dnssec-no-timecheck). In this case, the
    system time is considered to be valid once it becomes later than the
    timestamp on the specified file. The file is created and its
    timestamp set automatically by dnsmasq. The file must be stored on a
    persistent filesystem, so that it and its mtime are carried over
    system restarts. The timestamp file is created after dnsmasq has
    dropped root, so it must be in a location writable by the
    unprivileged user that dnsmasq runs as. 


Note, by default openwrt uses 'dnssec, dnssec-check-unsigned,
dnssec-timestamp'  - The man page arguably doesn't also emphasize enough
that if signature checking is enabled and the current time is incorrect
then resolution will fail (everything marked as bogus)

I await a new patch from a much better coder than me with enthusiasm!
Toke Høiland-Jørgensen Oct. 1, 2015, 8:58 p.m. UTC | #10
Steven Barth <cyrus@openwrt.org> writes:

> Using --dnssec-no-timecheck is impractical since it reacts to SIGHUP which
> is already overloaded and might be triggered by e.g. config changes.

Quite apart from the signaling, using --dnssec-no-timecheck very quickly
turns into an ugly hack. I implemented a startup time sync functionality
for CeroWrt based on this, see
https://github.com/dtaht/cerowrt-3.10/commit/b3a5b704691f1ba1d154dca9c7ab316f92136640

Never even attempted to upstream it because while it does sorta-kinda
work, it is a fairly ugly hack and I don't see any good way to avoid
that.

I definitely consider the timestamp file a cleaner way of solving the
DNSSEC/time sync problem, and will definitely recommend sticking with
that.

As far as whether or not it is a security risk: The whole issue here is
that it is fundamentally impossible to bootstrap DNSSEC securely without
a reliable clock (i.e. real-time clock or GPS or other offline source).
So we're stuck with doing things that minimise the duration of the
vulnerable window.

Also, as far as I can tell, dnsmasq will still read the time off the
file even if it can't write to it. So if the file ownership is the issue
(and I can see how this is at least a theoretical concern), just have
the file be owned as root, and have a suitably privileged process touch
it on shutdown (or periodically? presumably many reboots are going to be
hard power cycles, so no chance to do anything on shutdown?).

-Toke
diff mbox

Patch

diff --git a/package/base-files/files/etc/init.d/sysfixtime b/package/base-files/files/etc/init.d/sysfixtime
index 4010e06..b3e3862 100755
--- a/package/base-files/files/etc/init.d/sysfixtime
+++ b/package/base-files/files/etc/init.d/sysfixtime
@@ -1,11 +1,20 @@ 
 #!/bin/sh /etc/rc.common
-# Copyright (C) 2013-2014 OpenWrt.org
+# Copyright (C) 2013-2015 OpenWrt.org
 
 START=00
 
 boot() {
 	local curtime="$(date +%s)"
-	local maxtime="$(find /etc -type f -exec date -r {} +%s \; | sort -nr | head -n1)"
+	local maxtime="$(maxtime)"
+
 	[ $curtime -lt $maxtime ] && date -s @$maxtime
 }
 
+maxtime() {
+	local dir file
+
+	find /etc -type d | while read dir; do
+		file="$dir/$( ls -1t "$dir" | head -n1 )"
+		[ -e "$file" -a "$file" != '/etc/dnsmasq.time' ] && date -r "$file" +%s
+	done | sort -nr | head -n1
+}