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

14 Jan 2025


      On Tue, 14 Jan 2025 15:01:33 +0100
Enrique Llorente Pastora  wrote:
...
On Sat, Jan 11, 2025 at 12:53 AM Stefano Brivio  wrote:
...
On Fri, 10 Jan 2025 11:26:26 +0100
Enrique Llorente  wrote:
...
@@ -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);
+             offset = fill_one(m, 53, offset);
Now suppose that somebody adds a "long" option before this block, or...
...
for (i = 0; i < opts[55].clen; i++) {
              o = opts[55].c[i];
              if (opts[o].slen != -1)
-                     fill_one(m, o, &offset);
+                     offset = fill_one(m, o, offset);
      }
before this one, and fill_one() returns -1. Then...
...
for (o = 0; o < 255; o++) {
-             if (opts[o].slen != -1 && !opts[o].sent)
-                     fill_one(m, o, &offset);
+             if (opts[o].slen != -1 && !opts[o].sent) {
+                     offset = fill_one(m, o, offset);
you use offset as -1 here, and boom. If not, see directly below).
...
+                     if (offset == -1)
Or maybe the domain name is actually too long, and offset is -1 here...
...
+                             debug("DHCP: skipping option %i", o);
+             }
      }
m->o[offset++] = 255;
...and boom. This sets the last byte of "magic" (depending on the
architecture) to 0xff, making the whole message invalid.
well, I was expecting the only problematic options being at > 55
(didn't check the standard)
but I agree that we have to be consistent on handling the fill_in result.
Sure, that's the (reasonable) expectation, but note that you would have
the same problem even with a domain name that's too long for fill_one().

-- 
Stefano

    