Re: [PATCH v9] dhcp, dhcpv6: Add hostname and client fqdn ops

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 22 Jan 2025 13:09:07 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Thu, Jan 16, 2025 at 1:31 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > Thanks for sticking with this. The general approach and almost the
> > whole patch looks good to me now, but there are still a couple of
> > issues with the options:
> >
> > On Wed, 15 Jan 2025 09:18:44 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >  
> > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > have the VirtualMachine name as the guest hostname.
> > >
> > > This change add the following arguments:
> > >  - -H --hostname NAME to configure the hostname DHCPv4 option(12)
> > >  - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
> > >    DHCPv6(39)
> > >
> > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > ---
> > >  conf.c           | 20 +++++++++--
> > >  dhcp.c           | 60 ++++++++++++++++++++++++++-----
> > >  dhcpv6.c         | 93 ++++++++++++++++++++++++++++++++++++++++--------
> > >  passt.1          | 10 ++++++
> > >  passt.h          |  5 +++
> > >  pasta.c          | 17 ++++++---
> > >  test/lib/setup   | 10 +++---
> > >  test/passt.mbuto |  6 ++--
> > >  test/passt/dhcp  | 15 +++++++-
> > >  util.c           | 22 ++++++++++++
> > >  util.h           |  6 ++++
> > >  11 files changed, 226 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/conf.c b/conf.c
> > > index df2b016..0cbd625 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
> > >               FPRINTF(f, "    default: use addresses from /etc/resolv.conf\n");
> > >       FPRINTF(f,
> > >               "  -S, --search LIST    Space-separated list, search domains\n"
> > > -             "    a single, empty option disables the DNS search list\n");
> > > +             "    a single, empty option disables the DNS search list\n"
> > > +             "  -H, --hostname NAME  Hostname to configure client with\n"
> > > +             "  --fqdn NAME          FQDN to configure client with\n");
> > >       if (strstr(name, "pasta"))
> > >               FPRINTF(f, "    default: don't use any search list\n");
> > >       else
> > > @@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               {"outbound",    required_argument,      NULL,           'o' },
> > >               {"dns",         required_argument,      NULL,           'D' },
> > >               {"search",      required_argument,      NULL,           'S' },
> > > +             {"hostname",    required_argument,      NULL,           'H' },
> > >               {"no-tcp",      no_argument,            &c->no_tcp,     1 },
> > >               {"no-udp",      no_argument,            &c->no_udp,     1 },
> > >               {"no-icmp",     no_argument,            &c->no_icmp,    1 },
> > > @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               /* vhost-user backend program convention */
> > >               {"print-capabilities", no_argument,     NULL,           26 },
> > >               {"socket-path", required_argument,      NULL,           's' },
> > > +             {"fqdn",        required_argument,      NULL,           27 },
> > >               { 0 },
> > >       };
> > >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > @@ -1379,9 +1383,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > >       if (c->mode == MODE_PASTA) {
> > >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > >               fwd_default = FWD_AUTO;
> > > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
> > >       } else {
> > > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
> > >       }
> > >
> > >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               case 26:
> > >                       vu_print_capabilities();
> > >                       break;
> > > +             case 27:
> > > +                     if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > > +                                        "%s", optarg))
> > > +                             die("Invalid FQDN: %s", optarg);
> > > +                     break;
> > >               case 'd':
> > >                       c->debug = 1;
> > >                       c->quiet = 0;
> > > @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv)
> > >
> > >                       die("Cannot use DNS search domain %s", optarg);
> > >                       break;
> > > +             case 'H':
> > > +                     if (snprintf_check(c->hostname, PASST_MAXDNAME,
> > > +                                        "%s", optarg))
> > > +                             die("Invalid hostname: %s", optarg);
> > > +                     break;
> > >               case '4':
> > >                       v4_only = true;
> > >                       v6_only = false;
> > > diff --git a/dhcp.c b/dhcp.c
> > > index d8515aa..ba66f47 100644
> > > --- a/dhcp.c
> > > +++ b/dhcp.c
> > > @@ -63,6 +63,11 @@ static struct opt opts[255];
> > >
> > >  #define OPT_MIN              60 /* RFC 951 */
> > >
> > > +/* Total option size (excluding end option) is 576 (RFC 2131), minus
> > > + * offset of options (268), minus end option and its length (2).
> > > + */
> > > +#define OPT_MAX              306
> > > +
> > >  /**
> > >   * dhcp_init() - Initialise DHCP options
> > >   */
> > > @@ -122,7 +127,7 @@ struct msg {
> > >       uint8_t sname[64];
> > >       uint8_t file[128];
> > >       uint32_t magic;
> > > -     uint8_t o[308];
> > > +     uint8_t o[OPT_MAX + 2 /* End option and its length */ ];
> > >  } __attribute__((__packed__));
> > >
> > >  /**
> > > @@ -130,15 +135,28 @@ struct msg {
> > >   * @m:               Message to fill
> > >   * @o:               Option number
> > >   * @offset:  Current offset within options field, updated on insertion
> > > + *
> > > + * Return: false if m has space to write the option, true otherwise
> > >   */
> > > -static void fill_one(struct msg *m, int o, int *offset)
> > > +static bool fill_one(struct msg *m, int o, int *offset)
> > >  {
> > > +     size_t slen = opts[o].slen;
> > > +
> > > +     /* If we don't have space to write the option, then just skip */
> > > +     if (*offset + 1 /* length of option */ + slen > OPT_MAX)
> > > +             return true;
> > > +
> > >       m->o[*offset] = o;
> > > -     m->o[*offset + 1] = opts[o].slen;
> > > -     memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > +     m->o[*offset + 1] = slen;
> > > +
> > > +     /* Move to option */
> > > +     *offset += 2;
> > > +
> > > +     memcpy(&m->o[*offset], opts[o].s, slen);
> > >
> > >       opts[o].sent = 1;
> > > -     *offset += 2 + opts[o].slen;
> > > +     *offset += slen;
> > > +     return false;
> > >  }
> > >
> > >  /**
> > > @@ -162,17 +180,20 @@ static int fill(struct msg *m)
> > >        * Put it there explicitly, unless requested via option 55.
> > >        */
> > >       if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
> > > -             fill_one(m, 53, &offset);
> > > +             if (fill_one(m, 53, &offset))
> > > +                      debug("DHCP: skipping option 55");  
> >
> > We're skipping option 53 in this case, not 55.
> >  
> > >
> > >       for (i = 0; i < opts[55].clen; i++) {
> > >               o = opts[55].c[i];
> > >               if (opts[o].slen != -1)
> > > -                     fill_one(m, o, &offset);
> > > +                     if (fill_one(m, o, &offset))
> > > +                             debug("DHCP: skipping option %i", o);
> > >       }
> > >
> > >       for (o = 0; o < 255; o++) {
> > >               if (opts[o].slen != -1 && !opts[o].sent)
> > > -                     fill_one(m, o, &offset);
> > > +                     if (fill_one(m, o, &offset))
> > > +                             debug("DHCP: skipping option %i", o);
> > >       }
> > >
> > >       m->o[offset++] = 255;
> > > @@ -398,6 +419,29 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > >       if (!opts[6].slen)
> > >               opts[6].slen = -1;
> > >
> > > +     opt_len = strlen(c->hostname);
> > > +     if (opt_len > 0) {
> > > +             opts[12].slen = opt_len;
> > > +             memcpy(opts[12].s, &c->hostname, opt_len);
> > > +     }  
> >
> > So I tried (note the comma):
> >
> > $ ./passt -d -f --hostname thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one, -p fqdn.pcap
> >
> > $ ./passt -d -f --hostname , -p fqdn.pcap
> >
> > $ ./passt -d -f --hostname "" -p fqdn.pcap
> >
> > the hostname and the DHCP reply are fine, but we get an empty FQDN
> > option in the DHCPv6 reply (only).
> >
> > Note that RFC 2132 (3.14) says, about option 12:
> >
> >   See RFC 1035 for character set restrictions.
> >
> > but I'm not sure if we should validate the user input any further. I
> > wouldn't. One might legitimately use passt to test the robustness of a
> > DHCP client.
> >  
> 
> I understand that we are talking about two different things:
> - The fact that adding "," to option 12 is not ok

No, I think it's actually fine, I just wanted to make you aware of that.

> - And DHCPv6 is broken if we don't pass --fqdn because it adds a "."
> field that breaks it.

Right, yes.

> I will fix the DHCPv6 FQDN client option, not sure about validation
> input, but it's true that more or less
> we are going to send virtual machine names at hostnames and by
> kubernetes standard those should be a fqdn,
> so they are implicitly validated.

Well, this is completely independent of what Kubernetes might do with
it one day. But still, I would *not* validate it because passt is also
quite useful (and used) to test this kind of stuff.

> > > +
> > > +     opt_len = strlen(c->fqdn);
> > > +     if (opt_len > 0) {
> > > +             opt_len += 3 /* flags */
> > > +                     + 2; /* Length byte for first label, and terminator */
> > > +
> > > +             if (sizeof(opts[81].s) < opt_len)
> > > +                     debug("DHCP: client FQDN option doesn't fit, skipping");  
> >
> > For this one, I think you actually need to maintain the check in the
> > caller of encode_domain_name() (that is, here), because of the three
> > bytes for the flags.
> 
> Right, I am not "skipping", I will fix that.
> 
> > Otherwise, with this:
> >
> >   $ ./passt -d -f --fqdn
> >   thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one, -p fqdn.pcap
> >
> > this message is printed, but we end up with an option 81 with length
> > 2, and the FQDN ends up in the padding (try and see capture).
> >
> > With this:
> >
> >   $ ./passt -d -f --fqdn
> >  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_252_with_this_one -p fqdn.pcap
> >
> > length becomes 1, and the FQDN is still in the padding.
> >
> > With this:
> >
> >   $ ./passt -d -f --fqdn
> >  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.make_that_251_with_this_one -p fqdn.pcap
> >
> > length becomes 0, end option is missing, and we have an option 4 (time
> > server) instead.
> 
> After the fix I see just proper padding with 0 at the end, is this fine ?

Probably. We just shouldn't have unnecessary padding.

By the way, this change should depend on another change *not* reusing
the original message as buffer for the reply, as discussed.

> > With this:
> >
> >   $ ./passt -d -f --fqdn
> >  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_its_250_with_this_one -p fqdn.pcap
> >
> > things are almost fine. We get one byte of padding, which is probably
> > not needed, but I didn't look into it.
> 
> And at this case I see proper option 81 configured with length 255
> (250 + 3 /* flags */ + 2 /* first length and terminator */)
> 
> So with the fix now it looks good, I am super worried that we don't
> have this at the tests, but we will go back to it when the proper
> testing framework lands.

I'm not, in the sense that this isn't really security relevant *unless
we leak host memory into the guest*. The rest is a functional issue,
and it's not easy to break this "by mistake".

Sure, ideally we should have tests for those, but... they don't break
by themselves. This code is pretty much isolated and stand-alone.

-- 
Stefano