On Tue, 14 Jan 2025 15:01:33 +0100
Enrique Llorente Pastora
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