diff mbox

[OpenWrt-Devel] dnsmasq: remove dnssec timecheck enable on SIGHUP

Message ID 1443694799-5306-1-git-send-email-kevin@darbyshire-bryant.me.uk
State Rejected, archived
Headers show

Commit Message

Kevin Darbyshire-Bryant Oct. 1, 2015, 10:19 a.m. UTC
This patch stops SIGHUP from enabling dnssec timechecks if disabled by
use of --dnssec-no-timecheck option.  --dnssec-timestamp continues to
work correctly.

Enabling dnssec timechecks now requires restarting dnsmasq without
the --dnssec-no-timecheck configuration option and closes a
potential denial of service exploit by sending SIGHUP when system
time does not correspond with Internet time.

This change may be useful for future ntpd/dnsmasq hotplug integration.


Signed-off-by: Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
---
 .../dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch  | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch

Comments

Etienne Champetier Oct. 1, 2015, 10:37 a.m. UTC | #1
Hi,

2015-10-01 12:19 GMT+02:00 Kevin Darbyshire-Bryant <
kevin@darbyshire-bryant.me.uk>:

> This patch stops SIGHUP from enabling dnssec timechecks if disabled by
> use of --dnssec-no-timecheck option.  --dnssec-timestamp continues to
> work correctly.
>

I haven't really followed the previous discusion,
but maybe you can just use another signal?


>
> Enabling dnssec timechecks now requires restarting dnsmasq without
> the --dnssec-no-timecheck configuration option and closes a
> potential denial of service exploit by sending SIGHUP when system
> time does not correspond with Internet time.
>


>
> This change may be useful for future ntpd/dnsmasq hotplug integration.
>
>
> Signed-off-by: Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
> ---
>  .../dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch  | 13
> +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644
> package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch
>
>
Kevin Darbyshire-Bryant Oct. 1, 2015, 11:21 a.m. UTC | #2
On 01/10/15 11:37, Etienne Champetier wrote:
> Hi,
>
> 2015-10-01 12:19 GMT+02:00 Kevin Darbyshire-Bryant
> <kevin@darbyshire-bryant.me.uk <mailto:kevin@darbyshire-bryant.me.uk>>:
>
>     This patch stops SIGHUP from enabling dnssec timechecks if disabled by
>     use of --dnssec-no-timecheck option.  --dnssec-timestamp continues to
>     work correctly.
>
>
> I haven't really followed the previous discusion,
> but maybe you can just use another signal?
The user defined signals USR1 & USR2 are already occupied by dnsmasq
with debug/info dump type functions.  Maybe one of the SIGTT* signals
could be repurposed but I don't know how valid a solution that is.

However even if that were done it still doesn't stop a malicious
user/process from sending that new signal and potentially disabling dns
resolution (assuming dnssec is being used & the system time is incorrect)

Ideally some evaluation of threat presented by 'sysfixtime', 'dnssec
timestamp files', 'dnssec no timecheck' and the multi-function
'overloading' of SIGHUP into dnsmasq in the context of dnssec &
correct/incorrect system time should take place and an appropriate,
considered response and solution proposed/implemented.  That person
isn't me ;-)

I personally think that sysfixtime is a necessary evil, but at the very
least at the present moment until a more correct solution is
implemented, it should not be using dnsmasq's timestamp file as a source
time reference on boot.

  
>  
>
>
>     Enabling dnssec timechecks now requires restarting dnsmasq without
>     the --dnssec-no-timecheck configuration option and closes a
>     potential denial of service exploit by sending SIGHUP when system
>     time does not correspond with Internet time.
>
>  
>
>
>     This change may be useful for future ntpd/dnsmasq hotplug integration.
>
>
>     Signed-off-by: Kevin Darbyshire-Bryant
>     <kevin@darbyshire-bryant.me.uk <mailto:kevin@darbyshire-bryant.me.uk>>
>     ---
>      .../dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch  | 13
>     +++++++++++++
>      1 file changed, 13 insertions(+)
>      create mode 100644
>     package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch
>
>
Etienne Champetier Oct. 1, 2015, 11:56 a.m. UTC | #3
2015-10-01 13:21 GMT+02:00 Kevin Darbyshire-Bryant <
kevin@darbyshire-bryant.me.uk>:

>
>
> On 01/10/15 11:37, Etienne Champetier wrote:
> > Hi,
> >
> > 2015-10-01 12:19 GMT+02:00 Kevin Darbyshire-Bryant
> > <kevin@darbyshire-bryant.me.uk <mailto:kevin@darbyshire-bryant.me.uk>>:
> >
> >     This patch stops SIGHUP from enabling dnssec timechecks if disabled
> by
> >     use of --dnssec-no-timecheck option.  --dnssec-timestamp continues to
> >     work correctly.
> >
> >
> > I haven't really followed the previous discusion,
> > but maybe you can just use another signal?
> The user defined signals USR1 & USR2 are already occupied by dnsmasq
> with debug/info dump type functions.  Maybe one of the SIGTT* signals
> could be repurposed but I don't know how valid a solution that is.
>
> However even if that were done it still doesn't stop a malicious
> user/process from sending that new signal and potentially disabling dns
> resolution (assuming dnssec is being used & the system time is incorrect)
>

you can only signal yourself
http://stackoverflow.com/a/13335054/3768051


>
> Ideally some evaluation of threat presented by 'sysfixtime', 'dnssec
> timestamp files', 'dnssec no timecheck' and the multi-function
> 'overloading' of SIGHUP into dnsmasq in the context of dnssec &
> correct/incorrect system time should take place and an appropriate,
> considered response and solution proposed/implemented.  That person
> isn't me ;-)
>
> I personally think that sysfixtime is a necessary evil, but at the very
> least at the present moment until a more correct solution is
> implemented, it should not be using dnsmasq's timestamp file as a source
> time reference on boot.
>
>
> >
> >
> >
> >     Enabling dnssec timechecks now requires restarting dnsmasq without
> >     the --dnssec-no-timecheck configuration option and closes a
> >     potential denial of service exploit by sending SIGHUP when system
> >     time does not correspond with Internet time.
> >
> >
> >
> >
> >     This change may be useful for future ntpd/dnsmasq hotplug
> integration.
> >
> >
> >     Signed-off-by: Kevin Darbyshire-Bryant
> >     <kevin@darbyshire-bryant.me.uk <mailto:kevin@darbyshire-bryant.me.uk
> >>
> >     ---
> >      .../dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch  | 13
> >     +++++++++++++
> >      1 file changed, 13 insertions(+)
> >      create mode 100644
> >
>  package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch
> >
> >
>
>
>
Kevin Darbyshire-Bryant Oct. 1, 2015, 12:12 p.m. UTC | #4
On 01/10/15 12:56, Etienne Champetier wrote:
>
>
> 2015-10-01 13:21 GMT+02:00 Kevin Darbyshire-Bryant
> <kevin@darbyshire-bryant.me.uk <mailto:kevin@darbyshire-bryant.me.uk>>:
>
>
>
>     On 01/10/15 11:37, Etienne Champetier wrote:
>     > Hi,
>     >
>     > 2015-10-01 12:19 GMT+02:00 Kevin Darbyshire-Bryant
>     > <kevin@darbyshire-bryant.me.uk
>     <mailto:kevin@darbyshire-bryant.me.uk>
>     <mailto:kevin@darbyshire-bryant.me.uk
>     <mailto:kevin@darbyshire-bryant.me.uk>>>:
>     >
>     >     This patch stops SIGHUP from enabling dnssec timechecks if
>     disabled by
>     >     use of --dnssec-no-timecheck option.  --dnssec-timestamp
>     continues to
>     >     work correctly.
>     >
>     >
>     > I haven't really followed the previous discusion,
>     > but maybe you can just use another signal?
>     The user defined signals USR1 & USR2 are already occupied by dnsmasq
>     with debug/info dump type functions.  Maybe one of the SIGTT* signals
>     could be repurposed but I don't know how valid a solution that is.
>
>     However even if that were done it still doesn't stop a malicious
>     user/process from sending that new signal and potentially
>     disabling dns
>     resolution (assuming dnssec is being used & the system time is
>     incorrect)
>
>
> you can only signal yourself
> http://stackoverflow.com/a/13335054/3768051

It runs as nobody.  So do other processes.  I didn't raise the security
flag ;-)

>  
>
>
>     Ideally some evaluation of threat presented by 'sysfixtime', 'dnssec
>     timestamp files', 'dnssec no timecheck' and the multi-function
>     'overloading' of SIGHUP into dnsmasq in the context of dnssec &
>     correct/incorrect system time should take place and an appropriate,
>     considered response and solution proposed/implemented.  That person
>     isn't me ;-)
>
That statement still stands.

>     I personally think that sysfixtime is a necessary evil, but at the
>     very
>     least at the present moment until a more correct solution is
>     implemented, it should not be using dnsmasq's timestamp file as a
>     source
>     time reference on boot.
>
>
>     >
>     >
>     >
>     >     Enabling dnssec timechecks now requires restarting dnsmasq
>     without
>     >     the --dnssec-no-timecheck configuration option and closes a
>     >     potential denial of service exploit by sending SIGHUP when
>     system
>     >     time does not correspond with Internet time.
>     >
>     >
>     >
>     >
>     >     This change may be useful for future ntpd/dnsmasq hotplug
>     integration.
>     >
>     >
>     >     Signed-off-by: Kevin Darbyshire-Bryant
>     >     <kevin@darbyshire-bryant.me.uk
>     <mailto:kevin@darbyshire-bryant.me.uk>
>     <mailto:kevin@darbyshire-bryant.me.uk
>     <mailto:kevin@darbyshire-bryant.me.uk>>>
>     >     ---
>     >      .../dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch 
>     | 13
>     >     +++++++++++++
>     >      1 file changed, 13 insertions(+)
>     >      create mode 100644
>     >   
>      package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch
>     >
>     >
>
>
>
Toke Høiland-Jørgensen Oct. 1, 2015, 3:20 p.m. UTC | #5
Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> This patch stops SIGHUP from enabling dnssec timechecks if disabled by
> use of --dnssec-no-timecheck option.  --dnssec-timestamp continues to
> work correctly.

I'd argue that patching dnsmasq in this way is the wrong way to fix
this. If you're worried about that DOS vector, don't use
--dnssec-no-timecheck but rather use --dnssec-timestamp.

Also, in a scenario where --dnssec-no-timecheck is used, the expectation
is that the time will be fixed in fairly short order (i.e. as soon as
NTP syncs up), so the potential for this being a DOS vector is rather
small I would say... And if you can SIGHUP the process you can also
SIGKILL it.

-Toke
Kevin Darbyshire-Bryant Oct. 1, 2015, 3:37 p.m. UTC | #6
On 01/10/15 16:20, Toke Høiland-Jørgensen wrote:
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>
>> This patch stops SIGHUP from enabling dnssec timechecks if disabled by
>> use of --dnssec-no-timecheck option.  --dnssec-timestamp continues to
>> work correctly.
> I'd argue that patching dnsmasq in this way is the wrong way to fix
> this. If you're worried about that DOS vector, don't use
> --dnssec-no-timecheck but rather use --dnssec-timestamp.
Hi Toke, small world :-)

Could I kindly ask you to read
https://patchwork.ozlabs.org/patch/521344/ particularly with regards to
Yousong's comments.  You'll hopefully appreciate the irony of your
suggestion and how things (by which I mean 'I') have been sent on a bit
of a merry-go-round of late.


>
> Also, in a scenario where --dnssec-no-timecheck is used, the expectation
> is that the time will be fixed in fairly short order (i.e. as soon as
> NTP syncs up), so the potential for this being a DOS vector is rather
> small I would say... And if you can SIGHUP the process you can also
> SIGKILL it.
>
> -Toke
I couldn't agree more.  But in order to cover one's backside AND because
SIGHUP is used for multiple things, e.g. poking dnsmasq to get it to
re-read some files, there's a possibility that it could be poked to
re-read config files (good) but with a side effect of checking dnssec
timestamps at exactly the wrong moment ie. before time is made correct (bad)

I think I'm actually trying to be helpful but I'm stepping off now
before I .....well I'm not sure what before, but just before ;-)   Back
to sqm.

Kevin
Toke Høiland-Jørgensen Oct. 1, 2015, 4:15 p.m. UTC | #7
Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> Could I kindly ask you to read
> https://patchwork.ozlabs.org/patch/521344/ particularly with regards to
> Yousong's comments.  You'll hopefully appreciate the irony of your
> suggestion and how things (by which I mean 'I') have been sent on a bit
> of a merry-go-round of late.

Ah, completely missed that go by. Will go have a look at that.

> I think I'm actually trying to be helpful but I'm stepping off now
> before I .....well I'm not sure what before, but just before ;-) Back
> to sqm.

Yeah, feel your pain. This whole dnssec/time thing is a royal PITA. I
did a hackish implementation of the reload logic based on NTP state in
Cerowrt:
https://github.com/dtaht/cerowrt-3.10/blob/master/package/network/services/dnsmasq/files/check_ntpd.sh

...which was hackish but did kinda-sorta work. This was before the
timestamp file feature went in, though, and actually was under the
impression that that feature fixed it in a cleaner way. But of course if
sysfixtime is messing with that, well.... I'll go reply to the other
thread I guess.

-Toke
diff mbox

Patch

diff --git a/package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch b/package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch
new file mode 100644
index 0000000..2ea1ee8
--- /dev/null
+++ b/package/network/services/dnsmasq/patches/220-dnssec-disable-timecheck-hup.patch
@@ -0,0 +1,13 @@ 
+Index: dnsmasq-2.75/src/dnsmasq.c
+===================================================================
+--- dnsmasq-2.75.orig/src/dnsmasq.c	2015-07-30 20:59:07.000000000 +0100
++++ dnsmasq-2.75/src/dnsmasq.c	2015-10-01 10:47:38.832034041 +0100
+@@ -1054,7 +1054,7 @@
+       int event, errsave = errno;
+       
+       if (sig == SIGHUP)
+-	event = EVENT_RELOAD;
++	event = EVENT_INIT;
+       else if (sig == SIGCHLD)
+ 	event = EVENT_CHILD;
+       else if (sig == SIGALRM)