Re: [PATCH 6/7] cifs.upcall: use ip address passed by kernel to get server's hostname

On Sun, Aug 16, 2009 at 4:23 AM, Jeff Layton<[email protected]> wrote:
> On Sun, 16 Aug 2009 02:59:24 +0400
> "Q (Igor Mammedov)" <[email protected]> wrote:
>
>> Jeff,
>> I'm sorry for a late response.
>>
>> I see two possible issues with resolving ip to hostname and using it
>> as service principal:
>> 1. In case of spoofed revers lookup or local dns cache or
>> compromised dns server or router between client and dns
>> server, we could be connected to a fake server without even
>> noticing it.
>
> I have a hard time understanding this one. I don't think that we gain
> any security by not trusing reverse hostname resolution.
>
> Why? Because you still have to trust the forward resolve to give you
> the correct address. You're just as likely to end up on a compromised
> server that way.

No, we are not trusting anyone, may be except KDC a little ;)

>
>> If we leave code as it is now, i.e. use hostname
>> supplied by user at mount time, we will get service ticket for
>> server we intended to connect to.
>
> Only if you use the canonical hostname of the server when mounting.
>
>> In this case we at least will
>> get error message in logs  "Unexpected SMB signature".
>> As I can  remember, the current cifs code only checks signature
>> but doesn't tears session with wrong signature on packets
>> (transport.c:789) and doesn't do mutual authentication of the peer.
>>
>
> That should probably be fixed sometime, but again, I don't see how
> you're protected by not trusting reverse hostname resolution. With the
> previous scheme you *had* to mount with a hostname anyway, so you
> more or less have to trust your forward resolution anyway. I don't
> see how trusting reverse resolution increases your security exposure.

I'll try explain more clearly, what I've meant.

Providing that we could have compromised dns resolving, we could not
trust forward nor reverse resolution, but the goal of attacker is in
redirecting us his server.
Case 1: using host name  A.B.C supplied at mount time
   1.1 we have an IP of a  fake server because forward resolution was spoofed
   1.2 we get a valid session key from KDC for A.B.C
   Result: we can detect that packets a coming from a fake server, because
  of an incorrect signature.

Case 2: using host name F.A.K.E from revers resolution
   2.1 the same attack vector 1.1,  i.e. connect to a fake server's
IP, and doing
         revers resolution we are getting host name of a fake server.
  2.2 we get a valid session key from KDC for F.A.K.E because of fake server
        is a member of our domain.
  Result: attacker forced us to use his server, nobody will even notice it.

I prefer using 'case 1' for my files. And if cifs is fixed to drop a session
to a fake server, I will like it even more.

Case 2 are safe only in a hardened  network environment or with a secure
dns setup.

> There's a huge problem with the current scheme however. You need to
> know the canonical name of the host in the service ticket and then
> mount that. This is a huge source of confusion for users. Suppose I
> have a host:
>
>        server.example.com
>
> ...and I have a principal that I need to get in order to mount it:
>
>        cifs/[email protected]
>
> ...now, suppose I try to mount up a share from that host, but use a
> non-canonical hostname in the server section of the UNC:
>
>        //server/share
>
> ...it doesn't work. The upcall will try to assemble a principal name
> like:
>
>        cifs/[email protected]

I haven't noticed this case because in AD domain, a machine
account is registered with a fqdn and a hostname principals.
(At least it is so in my company)
So I were able to connect to //server/share

> ...similar problem if I try to mount a CNAME for the server. This is
> also a problem when you want to mix krb5 and DFS. I don't believe the
> current code sets the hostname.

The same thing, I someone wishes to provide access to a server via
CNAMEs, he/she should create corresponding principals on KDC.
This way it will not reduce security.

>
> The patch below fixes this (assuming you have your DNS and service
> principals set up correctly).
>
>> 2.  It will break working now code In environments where there
>> is no dns or revers lookup doesn't match with a supplied
>> server name. And user will not even suspect why cifs doesn't
>> work anymore.
>>
>
> What about those people who are broken now? I consider the fact that
> you can't mount using an alternate server hostname a bug.
>
> I don't think it's unreasonable to expect that people with a functional
> krb5 environment also have functioning hostname resolution. I think it's
> more reasonable to expect that host resolution work than to insist that
> people use the canonical hostname of the server when mounting. A user
> may not even have any idea what that hostname actually *is*.
>
> We should however probably note that properly functioning reverse host
> resolution is required in the manpage however (the manpage is due for
> some work anyway).

And maybe force those who hasn't dns at all, for whatever reason, to use it.

>
>> PS:
>> Theoretically we could have uninitialized memory read of
>> hostbuf if there wasn't ipv6 or ipv4 parameter in the upcall
>> request.
>>
>
> I don't think so. The patch below also changes:
>
> #define DKD_MUSTHAVE_SET (DKD_HAVE_IP|DKD_HAVE_VERSION|DKD_HAVE_SEC)
>
> ...or am I missing something?

It just caught my eyes.
Anyway I would initialized it, just in case.

>
>> On Fri, Aug 7, 2009 at 11:43 PM, Jeff Layton<[email protected]> wrote:
>> > Instead of using the hostname given by the upcall to get the server's
>> > principal, take the IP address given in the upcall and reverse resolve
>> > it to a hostname.
>> >
>> > Signed-off-by: Jeff Layton <[email protected]>
>> > ---
>> >  client/cifs.upcall.c |   68 +++++++++++++++++++++++++++++++++++++++++---------
>> >  1 files changed, 56 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/client/cifs.upcall.c b/client/cifs.upcall.c
>> > index 38be876..1e58503 100644
>> > --- a/client/cifs.upcall.c
>> > +++ b/client/cifs.upcall.c
>> > @@ -150,15 +150,15 @@ handle_krb5_mech(const char *oid, const char *principal, DATA_BLOB *secblob,
>> >  #define DKD_HAVE_HOSTNAME      0x1
>> >  #define DKD_HAVE_VERSION       0x2
>> >  #define DKD_HAVE_SEC           0x4
>> > -#define DKD_HAVE_IPV4          0x8
>> > -#define DKD_HAVE_IPV6          0x10
>> > -#define DKD_HAVE_UID           0x20
>> > -#define DKD_HAVE_PID           0x40
>> > -#define DKD_MUSTHAVE_SET (DKD_HAVE_HOSTNAME|DKD_HAVE_VERSION|DKD_HAVE_SEC)
>> > +#define DKD_HAVE_IP            0x8
>> > +#define DKD_HAVE_UID           0x10
>> > +#define DKD_HAVE_PID           0x20
>> > +#define DKD_MUSTHAVE_SET (DKD_HAVE_IP|DKD_HAVE_VERSION|DKD_HAVE_SEC)
>> >
>> >  static struct decoded_args {
>> >        int             ver;
>> >        char            *hostname;
>> > +       char            *ip;
>> >        uid_t           uid;
>> >        pid_t           pid;
>> >        sectype_t       sec;
>> > @@ -167,6 +167,7 @@ static struct decoded_args {
>> >  static unsigned int
>> >  decode_key_description(const char *desc, struct decoded_args *arg)
>> >  {
>> > +       int len;
>> >        int retval = 0;
>> >        char *pos;
>> >        const char *tkn = desc;
>> > @@ -174,7 +175,6 @@ decode_key_description(const char *desc, struct decoded_args *arg)
>> >        do {
>> >                pos = index(tkn, ';');
>> >                if (strncmp(tkn, "host=", 5) == 0) {
>> > -                       int len;
>> >
>> >                        if (pos == NULL)
>> >                                len = strlen(tkn);
>> > @@ -186,10 +186,18 @@ decode_key_description(const char *desc, struct decoded_args *arg)
>> >                        arg->hostname = SMB_XMALLOC_ARRAY(char, len);
>> >                        strlcpy(arg->hostname, tkn + 5, len);
>> >                        retval |= DKD_HAVE_HOSTNAME;
>> > -               } else if (strncmp(tkn, "ipv4=", 5) == 0) {
>> > -                       /* BB: do we need it if we have hostname already? */
>> > -               } else if (strncmp(tkn, "ipv6=", 5) == 0) {
>> > -                       /* BB: do we need it if we have hostname already? */
>> > +               } else if (!strncmp(tkn, "ip4=", 4) ||
>> > +                          !strncmp(tkn, "ip6=", 4)) {
>> > +                       if (pos == NULL)
>> > +                               len = strlen(tkn);
>> > +                       else
>> > +                               len = pos - tkn;
>> > +
>> > +                       len -= 3;
>> > +                       SAFE_FREE(arg->ip);
>> > +                       arg->ip = SMB_XMALLOC_ARRAY(char, len);
>> > +                       strlcpy(arg->ip, tkn + 4, len);
>> > +                       retval |= DKD_HAVE_IP;
>> >                } else if (strncmp(tkn, "pid=", 4) == 0) {
>> >                        errno = 0;
>> >                        arg->pid = strtol(tkn + 4, NULL, 0);
>> > @@ -288,6 +296,35 @@ cifs_resolver(const key_serial_t key, const char *key_descr)
>> >        return 0;
>> >  }
>> >
>> > +static int
>> > +ip_to_fqdn(const char *ipaddr, char *host, size_t hostlen)
>> > +{
>> > +       int rc;
>> > +       struct addrinfo hints = { .ai_flags = AI_NUMERICHOST };
>> > +       struct addrinfo *res;
>> > +
>> > +       rc = getaddrinfo(ipaddr, NULL, &hints, &res);
>> > +       if (rc) {
>> > +               syslog(LOG_DEBUG, "%s: failed to resolve %s to ipaddr: %s",
>> > +                       __func__, ipaddr,
>> > +                       rc == EAI_SYSTEM ? strerror(errno) : gai_strerror(rc));
>> > +               return rc;
>> > +       }
>> > +
>> > +       rc = getnameinfo(res->ai_addr, res->ai_addrlen, host, hostlen,
>> > +                        NULL, 0, NI_NAMEREQD);
>> > +       freeaddrinfo(res);
>> > +       if (rc) {
>> > +               syslog(LOG_DEBUG, "%s: failed to resolve %s to fqdn: %s",
>> > +                       __func__, ipaddr,
>> > +                       rc == EAI_SYSTEM ? strerror(errno) : gai_strerror(rc));
>> > +               return rc;
>> > +       }
>> > +
>> > +       syslog(LOG_DEBUG, "%s: resolved %s to %s", __func__, ipaddr, host);
>> > +       return 0;
>> > +}
>> > +
>> >  static void
>> >  usage(void)
>> >  {
>> > @@ -306,6 +343,7 @@ int main(const int argc, char *const argv[])
>> >        long rc = 1;
>> >        int c;
>> >        char *buf, *princ, *ccname = NULL;
>> > +       char hostbuf[NI_MAXHOST];
>> >        struct decoded_args arg = { };
>> >        const char *oid;
>> >
>> > @@ -383,12 +421,18 @@ int main(const int argc, char *const argv[])
>> >                }
>> >        }
>> >
>> > +       if (have & DKD_HAVE_IP) {
>> > +               rc = ip_to_fqdn(arg.ip, hostbuf, sizeof(hostbuf));
>> > +               if (rc)
>> > +                       goto out;
>> > +       }
>> > +
>> >        // do mech specific authorization
>> >        switch (arg.sec) {
>> >        case MS_KRB5:
>> >        case KRB5:
>> >                /* for "cifs/" service name + terminating 0 */
>> > -               datalen = strlen(arg.hostname) + 5 + 1;
>> > +               datalen = strnlen(hostbuf, sizeof(hostbuf)) + 5 + 1;
>> >                princ = SMB_XMALLOC_ARRAY(char, datalen);
>> >                if (!princ) {
>> >                        rc = 1;
>> > @@ -405,7 +449,7 @@ int main(const int argc, char *const argv[])
>> >                 * getting a host/ principal if that doesn't work.
>> >                 */
>> >                strlcpy(princ, "cifs/", datalen);
>> > -               strlcpy(princ + 5, arg.hostname, datalen - 5);
>> > +               strlcpy(princ + 5, hostbuf, datalen - 5);
>> >                rc = handle_krb5_mech(oid, princ, &secblob, &sess_key, ccname);
>> >                if (rc) {
>> >                        memcpy(princ, "host/", 5);
>> > --
>> > 1.6.0.6
>> >
>> > _______________________________________________
>> > linux-cifs-client mailing list
>> > [email protected]
>> > https://lists.samba.org/mailman/listinfo/linux-cifs-client
>> >
>
>
> --
> Jeff Layton <[email protected]>
>
_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client