Subject: Re: [IPsec] review of draft-pauly-ipsecme-split-dns-01

From: Paul Wouters <[email-masked]>
Date: Thu, 4 Aug 2016 12:57:32 -0400 (EDT)

Daniel,

Thanks for the review. We will pickup the reported nits and textual
clarifications. See comments below on some of your protocol questions.

> > I think the motivation should be better exposed. The reason
> > you need the options are:
> >    - 1) When you have multiple interface to be able to define
> >         for which domain name the DNS query MUST be sent to internal resolver.

I don't think the protocol depends on "interfaces", so I would avoid
adding that now. It is all about "views", and changing the view.

> > Note, that the architecture implies that my resolution goes through
> > the internal resolver. In other words, the mobile cannot have a
> >  resolver that directly queries the authoritative servers. Maybe that
> > would be good the NS are also provided by a INTERNAL_NS_DOMAIN

It can do that if it wants to, by using the supplied resolver. But the
way a mobile works is by adding "forwarders" for certain DNS domains.
I do not see why it would need to insist on talking to the authoritative
internal DNS servers? so unless he can come up with a real use case why
that would be needed, I would not want to add INTERNAL_NS_DOMAIN.

> 3.  Protocol Exchange
> 
> > MGLT: I think the section is missing text that provides an
> > overview of the protocol.
> > The purpose of the information exchanged between the VPN
> > client and the gateway are expected to provide the necessary
> > information so the VPN client be informed properly of the
> > domains that resolve differently internally than externally.
> > In addition, some extra material should be provided so the
> > resolution can appropriately be performed.

I think that belongs in the introduction, and should not be repeated
in Ssection 3 that specifies the protocol.

> > MGLT: My understanding is that INTERNAL_IP4_DNS and INTERNAL_IP6_DNS
> > only concerns the resolution. As resolution could be performed through
> > the resolvers as well as by the client I would treat this aspect and
> > the recommendations a separate section. The resolution could be for
> > example performed by a resolver on the client in which case NS
> > records may be requested by the client.

See above. The protocol hands over all the information needed to resolve
securely. If the client wants to satisfy its curiosity about where the
authoritative DNS servers are, it can ask the provided forwarding DNS
server.

> > MGLT: Maybe that would be good to have a INTERNAL_NS attribute.

Again, I dont see a use case for it.

> > Also one domain name may have multiple TA, I think it would be
> > wiser to be able to have multiple TA payload associated to one
> > domain.

The TA format includes the domain it applies for already, so there is
no reason to complicate things by creating more associations or
dependencies between the payload options.

> > MGLT: I think that is better to use the wire format than the string format.

As explains in another thread on the list, that just complicates sending
this information down to a locally running DNS server. None of the
current ones take DNS wireformat for on-the-fly reconfiguration.
However, they do take DNS presentation format. Also, for instance if
you look at the libreswan implementation, this data is passed into
the connection handler via environment variables, so libreswan would
have to convert the DNS wire format to DNS presentation format before
giving it to the handler for DNS reconfiguration.

> > I do not understand why the TTL is ignored. I also think that
> > would be important in case of KSK roll-over.

The TTL is ignored because this is an administrative reconfiguration.
Just like a DNS server loading trust anchors from a file, there is no
TTL to that file-based information either.

If the zone does a KSK roll without updating the IPsec server, it is
an administrative error. Possibly, the IPsec server could have code
that reads the KSK's hourly and auto-updates what it gives to its
clients, but that would all be implementation specific and I don't
think this document needs to specify that.

> > Maybe some text could mention that multiple TA can be added
> > especially during the key roll over.

Sure, if that is not obvious we can do that.

> > In addition, I think additional text shoudl be provided to clarify
> > what ar ethe advanatges of using a DS reccord. In fact providing
> > the KSK in stead of the DS prevent teh client to request the KSK....
> > so it seems the KSK will anyway be transmitted.

Trust anchors for DNS servers tend to be specified using DS records, or
"DS or DNSKEY" records. So DS was the common denominator. Additionally,
a DS blob is much smaller than a DNSKEY blob. I had asked at the last
IETF if DNS vendors prefered one of the other, since getting a DS means
they need to fetch the DNSKEY (insecurely) and then confirm it matches
the DS record supplied, and they said DS was fine for them. (I talked to
unbound, knot and bind people)

>    For each INTERNAL_DNS_DOMAIN entry in a CFG_REPLY payload, the client
>    SHOULD use the provided INTERNAL_IP4_DNS or INTERNAL_IP6_DNS DNS
>    servers as the only resolvers for the listed domains and its sub-
>    domains and it SHOULD NOT attempt to resolve the provided DNS domains
>    using its external DNS servers.
>   
> > MGLT: This may come with some privacy issues. So I would rather
> > let the client chose if the domain is in relation with an internal
> > resolution.

That can use some rewording. Note there are different use cases.

Obviously, we don't want VPN provider FOO to ask for all google.com
queries when the VPN client connects to it only for FOO services.
But we also want to give university BAR a chance to hand out a list
of their own custom domains that the VPN client does not know about
beforehand.

>    If a CFG_REPLY contains one or more INTERNAL_DNS_DOMAIN attributes,
>    the client SHOULD configure its DNS resolver to resolve those domains
>    and all their subdomains using only the DNS resolver(s) listed in
>    that CFG_REPLY message.  If those resolvers fail, those names SHOULD
>    NOT be resolved using any other DNS resolvers.  All other domain
> 
>    names MUST be resolved using some other external DNS resolver(s),
>    configured independently, and SHOULD NOT be sent to the internal DNS
>    resolver(s) listed in that CFG_REPLY message.  For example, if the
>    INTERNAL_DNS_DOMAIN attribute specifies "example.com", then
>    "example.com", "www.example.com" and "mail.eng.example.com" MUST be
>    resolved using the internal DNS resolver(s), but "anotherexample.com"
>    and "ample.com" MUST be resolved using the system's external DNS
>    resolver(s).
> 
> > MGLT: Unless i am missing some point I would rather mention this as a
> > SHOULD. This appears to me more a policy.

Well, the idea here is that you will not be able to resolve that
resource unless you accept DNS server for it. If you say SHOULD instead
of MUST, it means you are going to fail to resolve the internal
resource. As for using the internal DNS for more than advertised, if the
internal DNS server is okay with resolving everything, it could send
the value ".". We could clarify that explicitly.

One risk of using an internal DNS for a public DNS name is that the
client can be tricked into revealing that it has a VPN connection option
to VPN provider FOO by giving it a unique tracking DNS entry to lookup.
So unless the VPN provider instructs clients to use it for ALL DNS, I
think the advise of MUST is good.

Paul

---

From: Tero Kivinen <[email-masked]>
Date: Mon, 8 Aug 2016 15:39:33 +0300

[not waring any hats]

> giving it to the handler for DNS reconfiguration.

We are specifying the IKEv2 protocol extension, not really specifying
how to pass the information forward. If some implementation is using
environment variables, or command line options to pass this
information forward I think some kind of binary format is better in
IKEv2 than string format.

Ifwe use string format the IKEv2 implementations might easily just
pass the string forward without any checking, and this might cause
security vulnerabilities like we had with shellshock or similar.

If the input format is some kind of binary format, and the IKEv2
library then constructs the string format out of that it will remove
ability to do that kind of attacks.

With string formats we need to make sure that there is nothing
dangerous in there ("$INCLUDE /etc/passwd" or "; `sh -c
/sbin/reboot`") and we have to specify what kind of checking
implementations should do on the format.

Saying that the contents is DS RDATA wire format as specified in the
RFC4034 section 5.1 would be much easier, and converting that to
string representation is quite easy, as it is just 1 16-bit field, two
8-bit field, and then the digest:

                        1 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |           Key Tag             |  Algorithm    |  Digest Type  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   /                                                               /
   /                            Digest                             /
   /                                                               /
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

There would not be need for the TTL, so that is not an issue, but on
the other hand, we need the domain name, so perhaps the
INTERNAL_DNS_DOMAIN would need to be as follows:

                       1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |R|         Attribute Type      |            Length             |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  Name length  |                                               |
   +-+-+-+-+-+-+-+-+                                               |
   ~                          Domain Name                          ~
   |                                                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |           Key Tag             |  Algorithm    |  Digest Type  |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                                                               |
   ~                            Digest                             ~
   |                                                               |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Where the Name length tells the lengt of the domain name, and after
that there is optional DS record information for that domain. Whether
the Key Tag / Algorith / Digest Type is there after the Domain name is
seen from the Length field and the Key Tag is followed immediately
after Domain Name without any padding, and without the trailing dot,
and the Domain Name is not null-terminated.

If there is multiple DS records, then there would be multiple
INTERNAL_DNS_DOMAIN records, with same or different domain name.

I.e., in that case the example would be :

   CP(CFG_REQUEST) =
     INTERNAL_IP4_ADDRESS()
     INTERNAL_IP4_DNS()
     INTERNAL_DNS_DOMAIN(example.com)
     INTERNAL_DNS_DOMAIN(other.com)

   CP(CFG_REPLY) =
     INTERNAL_IP4_ADDRESS(198.51.100.234)
     INTERNAL_IP4_DNS(198.51.100.2)
     INTERNAL_IP4_DNS(198.51.100.4)
     INTERNAL_DNS_DOMAIN(example.com,31406,8,2,
       0xF78CF3344F72137235098ECBBD08947C2C9001C7F6A085A17F518B5D8F6B916D)
     INTERNAL_DNS_DOMAIN(example.com,31589,8,1,
       0x3490A6806D47F17A34C29E2CE80E8A999FFBE4BE)
     INTERNAL_DNS_DOMAIN(example.com,43547,8,1,
       0xB6225AB2CC613E0DCA7962BDC2342EA4F1B56083)
     INTERNAL_DNS_DOMAIN(example.com,31406,8,1,
       0x189968811E6EBA862DD6C209F75623D8D9ED9142)
     INTERNAL_DNS_DOMAIN(example.com,31589,8,2,
       0xCDE0D742D6998AA554A92D890F8184C698CFAC8A26FA59875A990C03E576343C)
     INTERNAL_DNS_DOMAIN(example.com,43547,8,2,
       0x615A64233543F66F44D68933625B17497C89A70E858ED76A2145997EDF96A918)
     INTERNAL_DNS_DOMAIN(city.other.com)

with DS records given for example.com, but without DS records for
city.other.com. 

> think this document needs to specify that.

It is not enough for the IPsec server to fetch KSK, as the client
might have fetched the TA information two months ago, when it
connected to the SGW. It might have done several IKEv2 rekeys after
that, but as it does configuration payloads only when it initially
connects it will not get new configuration information ever.

This might not be something that we actually need to fix here, it
might be enough just to add some words about that, and say that if
internal domain does KSK roll overs then the server might want to
request clients to do reauthentication (RFC4478) while doing roll over
so they will get new configuration information before the old one is
removed. 

> the value ".". We could clarify that explicitly.

Internal server might be willing to resolve everything, and even
provide completely new DS for "." so you can do everything securely,
but the user might be bit suprised to learn that his facebook.com
requests go to the company dns server, and dnssec is happy with the
faked results pointing to the companys internal facebook proxy that
checks they do not post anything nasty about company to the
facebook...

So this is also security issue, i.e., how much configuration you
should trust from the server. Bad server can send you bad
configuration data, and client might want to limit the damage done by
the bad server.
-- 
[email-masked]

---

From: Paul Wouters <[email-masked]>
Date: Mon, 8 Aug 2016 09:12:55 -0400 (EDT)

> IKEv2 than string format.

Well, we have been passing domain names to forward via IKEv1 XAUTH for
10+ years and I've never heard of a problem with passing domain names
as strings. I find this "security software authors might do dangerous
string passing" a rather unconvincing argument.

> security vulnerabilities like we had with shellshock or similar.

We are talking about dedicated security software here, not /bin/bash. If
your security software can't do string passing, you already are using
a timb bomb.

> ability to do that kind of attacks.

Why? You don't think attackers can encode weird characters in binary
which would be "blindly" decoded into ascii with the same dangerous
characters inside?

> implementations should do on the format.

Same for binary format? We either have to write our own routines (more
likely) or link against a DNS library that has this functionality.
Like using ldns, which would drag in so much code people more likely
will copy the wire conversation functions from some dns library into
their code and never update that code again, possibly leaving in
security vulnerabilities.

> 8-bit field, and then the digest:

And now you have to understand DNS wire format and all their possible
changes, use of compression, sneaking in multiple records, etc etc.

>       0xF78CF3344F72137235098ECBBD08947C2C9001C7F6A085A17F518B5D8F6B916D)

I understand you can do this, but it does add a string2wire() and
wire2string() operation for what I believe is a red-herring for security
reasons.

> connects it will not get new configuration information ever.

Perhaps that needs clarification, but my idea was that you purge the
trust anchor and the zone data underneath it when you tear down the
IPsec SA. The VPN tunnel is not meant to be a replacement for RFC-5011
style trust anchor updating. The split-DNS view can be changed at any
time by the server/internal network and clients MUST NOT store this
information as a permanent update. I will add language to clarify that.

> so they will get new configuration information before the old one is

Any internal KSK roll should be done slowly enough (days or weeks) so
that any VPN client connecting can always pull a working DNSKEY RRset.
Perhaps a re-key or re-auth event should allow to request/send a new
CFG set to keep updated for very long lived tunnels?

> checks they do not post anything nasty about company to the

I am not sure if you are agreeing with the SHOULD or the MUST here? You
seem to be arguing to leave it as MUST?

> configuration data, and client might want to limit the damage done by

Which is why the client can send the list of domains it is willing to
have overridden, or it can interpret the received list. And it can
ignore "." if that is not in its locally accepted list.

Paul

---

From: Tero Kivinen <[email-masked]>
Date: Mon, 8 Aug 2016 19:13:00 +0300

> string passing" a rather unconvincing argument.

Simple string is not an issue. Regex to check whether domain name is
leagl is quite simple, and even then people do that wrong.

Checking whether string is valid textual representation of the dns
record is harder, and you have to think things like, are comments
allowed, are newlines allowed. Also if you want to verify that the DS
record is actually for the real domain name, and not something else
you need to parse the string to get the first entry of it, and you
might need to know what $ORIGIN is to be able to know which suffix you
need to add to it if it does not have dot in the end etc...

I do not even know whether that format has some way of escaping
special characters, i.e., some kind of \x2e format or so.

Where is the formal specification for that text representation?
RFC4034 just specifies DS RR specific stuff, but it does not give any
indication for the generic format. For example where is it said that
"; key id = 60485" is comment?

I tried quickly look that up, but could not find it, so it would be
better to add direct reference to the rfc specifying the generic text
representation format of DNS RR records, not just the DS RR specific
parts which are in RFC4034. 

> your security software can't do string passing, you already are using

You said that someone uses environment variables, and some uses it as
command line arguments. Those all might have issues with bash parsing
environment variables in specific ways, meaning that the security
software needs to make sure the things it passes to them are safe.

Yes, the easist way to do string passing that format is to first parse
it in IKEv2 from text to binary, and then reformat it back to text
format which I know cannot have anything funny in it, but before I can
do that I need to know the format I am about to parse, and for now I
do not have that information. 

> which would be "blindly" decoded into ascii with the same dangerous

No they cannot. If I encode the 8-bit number as 8-bit binary, when I
parse it, I will get 8-bit number, which I can then print with "%d" to
ascii if needed. 

> Same for binary format?

We do not need to specify our binary format to include any of those
special things. There is no need for our binary format to allow
comments, or including other files or anything like that. We do not
need to specify binary format that includes all features the text
format has, only the features we need, which in our case is only to
encode the DS RR record, which consists exactly 4 fields. 

> We either have to write our own routines (more likely) or link
> against a DNS library that has this functionality.

You can link against DNS library, I would write:

  ret = ssh_decode_buffer(buffer,
			  SSH_DECODE_UINT8_STR(&name, &name_len),
			  SSH_DECODE_UINT16(&key_tag), 
			  SSH_DECODE_CHAR(&alg), 
			  SSH_DECODE_CHAR(&digest_type),
			  SSH_FORMAT_END);
  if (ret == 0) { return SSH_IKEV2_INVALID_SYNTAX; }
  digest = ssh_buffer_ptr(buffer) + ret;
  digest_len = ssh_buffer_len(buffer) - ret;

which is most likely easier, than to find out the suitable location in
dns library where to call to get the rr-record decoded. 

> security vulnerabilities.

Yes. And as we are talking about 10 lines of code, I think they should
write it specifically for this job, and not use huge library. 

> changes, use of compression, sneaking in multiple records, etc etc.

I was talking DS RDATA wire format as specifice in 5.1, and I do not
think there is anything like compression there, or multiple records,
as it is just one record. But if you like, we can just copy the fields
and their sizes to this specification and not use the RDATA format if
that makes you happy. 

> wire2string() operation for what I believe is a red-herring for security

I disagree with you. I think it will add security, will make the
implementations easier to write, and smaller. Especially if you do
have your own resolver you are going to configure, and which might not
use textual format at all. In that case you suddenly need to implement
full textual format parser just to parse this one line of text. 

> IPsec SA.

IPsec SA? I assume you are meaning IKEv2 SA, not IPsec SA (Child SA),
as configuration parameters passed inside the configuration paylads
are tied to the lifetime of the IKEv2 SA, not Child SA.

> The VPN tunnel is not meant to be a replacement for RFC-5011

But you do store it for few years, i.e. as long as your IKEv2 SA
happens to stay up... Not all IKEv2 SAs are short lived. Some of them
might stay up for months, or even years. 

> Any internal KSK roll should be done slowly enough (days or weeks)
> so that any VPN client connecting can always pull a working DNSKEY
> RRset.

Why would client reconnect that often? I mean if I have IKEv2 up and
running, and I do rekeys every 24 hours, why should I need to
reconnect it ever unless the network between devices break down. 

> CFG set to keep updated for very long lived tunnels?

IKEv2 rekey does not do that, as old configuration parameters are
inherited by new IKEv2 SA. Re-authentication (i.e. tearing down the
IKEv2 SA and creating new one) do make new configuration payload
exchange, thus might get new IP-address and will get new trust anchors
for DNSSEC.

And yes, the server can just send delete notification to the IKEv2 SA
when the old trust anchor is about to expire, thus forcing the client
to do reauthentication, and this will work even if the client does not
support RFC4478.

This might of course cause issues, as user might not be there to type
in the credentials need to reconnect, thus causing the connection to
stay down...

> seem to be arguing to leave it as MUST?

Not really, I am saying we should have warning saying that rogue
or badly configure server can cause all kind of issues especially by
sending the DS records for ".". The security considerations section do
have text saying that if vpn gateway A sends "example.com" and vpn
gateway B does the same then the IKE connection is terminated, but it
does not say that vpn gateway a might send "." as INTERNAL_DNS_DOMAIN
and give new TA for "." too, which will forward all DNS requests to
the vpn...

Also if one VPN gateway sends "example.com" and another sends "com"
which one of them is used? The one which the dns resolver you use
happens to use, or do we want to specifcy that resolvers should then
always use longest match when matching against the list of
resolvers... 

> ignore "." if that is not in its locally accepted list.

But adding reasoning for that to the security considerations section
is needed... 
-- 
[email-masked]