I tried to compare libslirp's DHCP reply with the one of passt to find out why passt doesn't work for me. The only obvious difference I can see so far is that passt doesn't set siaddr, whereas slirp does. If you tell me how can I change the code to set the siaddr, I can try that and see if this is the problem or not.
Also slirp properly clears the received fields. like eg flags. And passt simply copies them back as received, w/o any clearing. This is quite sloppy.
(In reply to stsp from comment #0) > I tried to compare libslirp's DHCP reply > with the one of passt to find out why passt > doesn't work for me. > The only obvious difference I can see so far > is that passt doesn't set siaddr, whereas > slirp does. We shouldn't, because RFC 2131 says: DHCP clarifies the interpretation of the 'siaddr' field as the address of the server to use in the next step of the client's bootstrap process. A DHCP server may return its own address in the 'siaddr' field, if the server is prepared to supply the next bootstrap service (e.g., delivery of an operating system executable image). but... yes, a BOOTP client might expect something there, so: > If you tell me how can I change the code to > set the siaddr, I can try that and see if this > is the problem or not. you can simply use yiaddr assignments as example. In line 338 of dhcp.c you have: m->yiaddr = c->ip4.addr; and you could try placing "our" address in siaddr just after that: m->siaddr = c->ip4.gw; (In reply to stsp from comment #1) > Also slirp properly clears the received fields. > like eg flags. And passt simply copies them back > as received, w/o any clearing. This is quite sloppy. The main reasoning is that in general we want to "accept" anything the client might request -- this works for a number of options, and fix up whatever we need for compatibility. Other than that, we want to keep this as simple as we reasonably can (cf. CVE-2021-3592). By the way, I couldn't find a moment to look into https://bugs.passt.top/show_bug.cgi?id=72 yet, sorry for that.
> m->siaddr = c->ip4.gw; You mean: m->siaddr = c->ip4.gw.s_addr; ? That's what I already tried, no luck. :( So it seems like I checked all option fields and all fixed fields. Probably the next thing is to inspect the UDP packet, ipv4 packet, ethernet frame... libslirp replies to broadcast btw, whereas passt replies with the mac of a destination.
OK, victory!!! This works: diff --git a/dhcp.c b/dhcp.c index 46c258e..4f8404c 100644 --- a/dhcp.c +++ b/dhcp.c @@ -149,6 +149,12 @@ static int fill(struct msg *m) for (o = 0; o < 255; o++) opts[o].sent = 0; + o = 53; + if (opts[o].slen && !opts[o].sent) + fill_one(m, o, &offset); + o = 54; + if (opts[o].slen && !opts[o].sent) + fill_one(m, o, &offset); for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; if (opts[o].slen) I thought the order slirp arranges the optional fields, is not important... but it is!
Smaller version of the patch: diff --git a/dhcp.c b/dhcp.c index 46c258e..da1cbff 100644 --- a/dhcp.c +++ b/dhcp.c @@ -149,6 +149,8 @@ static int fill(struct msg *m) for (o = 0; o < 255; o++) opts[o].sent = 0; + fill_one(m, 53, &offset); + fill_one(m, 54, &offset); for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; if (opts[o].slen) Extra checks seems unneeded as 53 and 54 should always present.
Btw slirp uses defines, rather than the raw option numbers. ;)
(In reply to stsp from comment #5) > Smaller version of the patch: > > diff --git a/dhcp.c b/dhcp.c > index 46c258e..da1cbff 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -149,6 +149,8 @@ static int fill(struct msg *m) > for (o = 0; o < 255; o++) > opts[o].sent = 0; > > + fill_one(m, 53, &offset); > + fill_one(m, 54, &offset); > for (i = 0; i < opts[55].clen; i++) { > o = opts[55].c[i]; > if (opts[o].slen) So, strictly speaking, this is not entirely correct because RFC 2132 says: The client MAY list the options in order of preference. The DHCP server is not required to return the options in the requested order, but MUST try to insert the requested options in the order requested by the client. which is now fixed with commit c09069211a97 ("dhcp: Actually note down the length of options received by the client"), and nothing actually prevents the client from requesting options 53 and 54 (even though it makes little sense as they must be sent anyway). On the other hand, a reasonable interpretation of "requested options" would also permit adding 53 and 54 first. If it fixes things for somebody, great, let's go for it. Could you just check first if, maybe, it's just option 53 that needs to be inserted before the rest? > Extra checks seems unneeded as > 53 and 54 should always present. Right. (In reply to stsp from comment #6) > Btw slirp uses defines, rather than > the raw option numbers. ;) I kept option numbers on purpose because, at least to me, "option 53" is more meaningful than "message type", and "option 121" is shorter than "Classless Static Route Option" (or some contraction thereof that I would keep misspelling...).
> but MUST try to insert the requested options in the order requested > by the client. Oh! That seem to make sense. The client requests in this order: 53, 61, 57, 55. Will you prepare a bigger fix to remember the client's order and answer exactly that way? > Could you just check first if, maybe, it's just option 53 that needs > to be inserted before the rest? Yes, this is sufficient. And that can also be seen from the aforementioned client's request.
(In reply to stsp from comment #8) > > but MUST try to insert the requested options in the order requested > > by the client. > > Oh! That seem to make sense. > The client requests in this order: > 53, 61, 57, 55. > Will you prepare a bigger fix to > remember the client's order and > answer exactly that way? Gosh, no, wait, this should be already fixed. Do you have commit c09069211a97 ("dhcp: Actually note down the length of options received by the client") in your build?
> Gosh, no, wait, this should be already fixed. Do you have commit c09069211a97 > ("dhcp: Actually note down the length of options received by the client") in > your build? Of course I do. I was waiting for that commit to settle, before starting the final debugging attempt. I don't get your point here though. The fix only makes sure the requested options are replied. But it still doesn't enforce the ordering. So even with the fix the option 53 is not the first one, even though the client puts it as a first one. Much bigger fix is needed to keep the ordering correctly.
I think the quote from RFC says 2 things: - Server must "patch in" the options requested by the client, without changing their order (the same way you do not overwrite flags etc - you patch reply into the client's data frame) - Server can add more options (not the ones requested), and for them the order doesn't matter, but they should be at the end.
(In reply to stsp from comment #10) > > Gosh, no, wait, this should be already fixed. Do you have commit c09069211a97 > > ("dhcp: Actually note down the length of options received by the client") in > > your build? > > Of course I do. > I was waiting for that commit to > settle, before starting the final > debugging attempt. > I don't get your point here though. > The fix only makes sure the requested > options are replied. But it still > doesn't enforce the ordering. It *should* enforce the ordering. First we go through the list in option 55: for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; if (opts[o].slen) fill_one(m, o, &offset); } and add the options (for which we have a value) from that list, in that order. Then we add the rest: for (o = 0; o < 255; o++) { if (opts[o].slen && !opts[o].sent) fill_one(m, o, &offset); } I understand it's buggy, but I'm don't know where the issue would be.
> It *should* enforce the ordering. How is so? fill_one() function just gets an offset as an external parameter, and advances it to the next free space: *offset += 2 + opts[o].slen; Then that offset is used for a subsequent option. The loop you pointed, sorts them in a numerical order: options with the smaller number go first. Sorry but I don't see a single code line that suggests the ordering is enforced. And all I see from the frame dump, is a numerical order of options, just as expected from the code when I read it.
(In reply to stsp from comment #13) > > It *should* enforce the ordering. > > How is so? By doing this: > for (i = 0; i < opts[55].clen; i++) { For every byte in the option 55 sent by the client, > o = opts[55].c[i]; take that number, and > if (opts[o].slen) if it matches the option number of an option for which we have a value to send, > fill_one(m, o, &offset); add that option > fill_one() function just gets > an offset as an external parameter, > and advances it to the next free > space: > *offset += 2 + opts[o].slen; > Then that offset is used for a > subsequent option. > The loop you pointed, sorts them in > a numerical order: options with the > smaller number go first. No, we should go through single bytes in option 55 first. Every byte represents an option number, and we go through those bytes (and numbers) in order. > Sorry but I don't see a single code > line that suggests the ordering is > enforced. And all I see from the > frame dump, is a numerical order of > options, just as expected from the > code when I read it. Those are probably from the second loop. Unless there's another issue I'm missing.
> Those are probably from the second loop. Unless there's another issue I'm > missing. Yes, from second loop. All options I see sorted, including 53, are from second loop. I am not sure how 55 plays here because libslirp doesn't even fill 55 at all.
(In reply to stsp from comment #15) > > Those are probably from the second loop. Unless there's another issue I'm > > missing. > > Yes, from second loop. > All options I see sorted, including 53, > are from second loop. > I am not sure how 55 plays here because > libslirp doesn't even fill 55 at all. Option 55 is filled *by the client* -- that's the "Parameter Request List" we should follow the order from.
> Option 55 is filled *by the client* -- that's the "Parameter Request List" > we should follow the order from. I think we read this part differently: The client MAY list the options in order of preference. The DHCP server is not required to return the options in the requested order, but MUST try to insert the requested options in the order requested by the client. To me (and obviously also to the author of the dhcp client I use) this sounds like server may not follow an order of 55 because the order in which the client filled an options in its request, is more important.
Also the client doesn't even do an "overlap": what it puts into 55 is NOT PRESENT as an option in his request. So the author was very pedantic on reading this part of an RFC. So you obviously first need to deal with client-passed options, then with 55-passed options, then with the rest.
(In reply to stsp from comment #18) > Also the client doesn't even do an > "overlap": what it puts into 55 is > NOT PRESENT as an option in his request. This is totally normal and expected: in general we do *not* need to echo back options sent from the client. See RFC 2131, 3.5: The client can inform the server which configuration parameters the client is interested in by including the 'parameter request list' option. The data portion of this option explicitly lists the options requested by tag number. and that's it -- for the rest, we follow the description from RFC 2131 4.3.1, starting from: o Parameters requested by the client, according to the following rules: [...] > So the author was very pedantic on > reading this part of an RFC. > So you obviously first need to deal > with client-passed options, then with > 55-passed options, then with the rest. "Deal with client-passed options" however doesn't mean that we need to echo back options -- that's not specified anywhere and *generally* not done. In your specific case, what's listed by the client in option 55, and what options is passt currently sending back?
> in general we do *not* need to echo back options sent from the client. Of course, but sometimes you do. Like in the case of 53, or some other mandatory option, or some option from 55 that was also sent by client. And when you do, you MUST use the client-provided place-holder. > "Deal with client-passed options" however doesn't mean that we need to > echo back options -- that's not specified anywhere and *generally* not done. But sometimes it is done. These cases are handled wrongly now. > In your specific case, what's listed by the client in option 55 37 0c 01 03 06 08 09 0c 0f 17 1a 23 24 25 (sorry for hex, this is how I see dumps) > and what options is passt currently sending back? passt is sending back the correct set of options (adds 53, 54 etc), but it doesn't use the client-provided place-holder when one exists. This is a bug.
(In reply to stsp from comment #20) > > in general we do *not* need to echo back options sent from the client. > > Of course, but sometimes you do. > Like in the case of 53, or some other > mandatory option, or some option > from 55 that was also sent by client. > And when you do, you MUST use the > client-provided place-holder. Why? I can't find any reference to any "placeholder" in any standard. > > "Deal with client-passed options" however doesn't mean that we need to > > echo back options -- that's not specified anywhere and *generally* not done. > > But sometimes it is done. > These cases are handled wrongly now. > > > In your specific case, what's listed by the client in option 55 > > 37 0c 01 03 06 08 09 0c 0f 17 1a 23 24 25 > (sorry for hex, this is how I see dumps) > > > and what options is passt currently sending back? > > passt is sending back the correct > set of options (adds 53, 54 etc), but > it doesn't use the client-provided > place-holder when one exists. > This is a bug. I don't think it is, because again, there's no such thing as a client placeholder. However, to fix compatibility with your client, you saw that we need to add (at most) options 53 and 54 before other options. Now, option 54 is surely not sent by the client. So I guess it just boils down to inserting option 53 before the others? Option 54 too? Both are okay, except that if they are requested by the client (that makes little sense, but it's not forbidden), then we MUST try to insert them in the order specified by option 55. So I think the code should look like this: /* Workaround for client ...: add options 53 and 54 before anything else, * but not if they were requested via option 55: respect the sequence from * option 55 in that case. */ for (i = 0; i < opts[55].clen && opts[55].c[i] != 53; i++); if (i == opts[55].clen) fill_one(m, 53, &offset); for (i = 0; i < opts[55].clen && opts[55].c[i] != 54; i++); if (i == opts[55].clen) fill_one(m, 54, &offset); for (i = 0; i < opts[55].clen; i++) { [...]
> Why? I can't find any reference to any "placeholder" in any standard. The client MAY list the options in order of preference. The DHCP server is not required to return the options in the requested order, but MUST try to insert the requested options _in the order requested by the client_. No, I do not agree with what you write above. This quote from RFC is very clear on the fact that the options are returned in an order requested by the client. And the first part of the quote says that the order of 55 is not as important as the order of options specified by client.
> Option 54 too? No, as in this case client have not provided it in the request. But it did for 53, and that needs to be respected.
> I can't find any reference to any "placeholder" But you can see the "insert the requested options" sentence. In that particular case by "insert" they most likely meant "use the existing place-holder rather than to allocate from free space".
(In reply to stsp from comment #22) > > Why? I can't find any reference to any "placeholder" in any standard. > > The client MAY list the options in order of preference. The DHCP > server is not required to return the options in the requested order, > but MUST try to insert the requested options _in the order requested > by the client_. > > No, I do not agree with what you > write above. This quote from RFC > is very clear on the fact that the > options are returned in an order > requested by the client. This quote is about Option 55 (Parameter Request List). I quoted this from RFC 2132, section 9.8. There's no other way for the client to request options (see RFC 2131, 4.3.1). The options sent by the client are *client* options, they have little to do with server options. > And the > first part of the quote says that > the order of 55 is not as important > as the order of options specified > by client. That quote refers to the list included in option 55 (only), because it describes option 55. Please have a look at where I took that quote from: https://datatracker.ietf.org/doc/html/rfc2132#section-9.8
> There's no other way for the client to request options (see RFC 2131, 4.3.1). I fully agree. But there appears to be a way for client to specify a place-holder for this or that option IFF the server is going to return that option. If it is not returning that option, then place-holder is ignored. > That quote refers to the list included in option 55 (only), because it > describes option 55. Please have a look at where I took that quote from: Yes, I know where that quote comes from. It says: The client MAY list the options in order of preference. This means 55. The DHCP server is not required to return the options in the requested order, This means the server can ignore the order of 55 in some cases. but MUST try to insert the requested options _in the order requested by the client_. This means the server CAN'T ignore the place-holder, if one exists. It must INSERT the option there. The word "try" here means that if there is no place-holder, there there is no where to insert, so we then allocate from the free space.
"insert" means write to an existing location. If RFC says "you try to insert", then it means: if the place-holder exists you insert, otherwise append.
(In reply to stsp from comment #26) > > There's no other way for the client to request options (see RFC 2131, 4.3.1). > > I fully agree. > But there appears to be a way for > client to specify a place-holder > for this or that option IFF the server > is going to return that option. Where, why, how? Can you point to a reference? As far as I can see there's no concept of "placeholder" option whatsoever. The server is expected to build a new set of options from scratch. We reuse the buffer for convenience, but I haven't actually seen any other DHCP server doing that (not even the one from libslirp -- it allocates a new buffer). Please don't get confused by the somewhat particular implementation I used in passt. > If it > is not returning that option, then > place-holder is ignored. > > > > That quote refers to the list included in option 55 (only), because it > > describes option 55. Please have a look at where I took that quote from: > > Yes, I know where that quote comes from. > It says: > > The client MAY list the options in order of preference. > This means 55. > > The DHCP > server is not required to return the options in the requested order, > This means the server can ignore the > order of 55 in some cases. > > but MUST try to insert the requested options _in the order requested > by the client_. > This means the server CAN'T ignore the > place-holder, if one exists. It must INSERT > the option there. The word "try" here means > that if there is no place-holder, there there > is no where to insert, so we then allocate > from the free space. Option 55 is used by the client to request options, that's it. A client including an option is *not* a way to request it back! Clients might include for example options 61 (Client-identifier), option 55 itself, and option 57 (Maximum DHCP Message Size), but the server MUST NOT (see Table 3, RFC 2131) include them in the reply. The word "try" here simply refers to possible implementation issues, for example if you have pointers to aligned data on memory-constrained systems and you can't conveniently copy it.
> Where, why, how? For example if client provided option 53, then you MUST write option 53 to the exactly same location. > Can you point to a reference? Only the RFC quote we discuss. I maintain it says what I see there, and obviously at least some dhcp authors are also reading it the same way as myself. > A client including an option is *not* a way to request it back! Sure thing! I never claimed the opposite. Only IFF you are going to return that option anyway (not because it was sent by the client!), then you must use the same offset. So this is not a way to request option! This is a way to request the offset for an option. > The word "try" here simply refers to possible implementation issues How would you in this case interpret The DHCP server is not required to return the options in the requested order, but MUST try to insert the requested options _in the order requested by the client_. Not required, but MUST try to insert? In your interpretation this part will most likely lead to a contradiction. But in my (and also dhcp client's author) intepretation, its perfectly clear and does not lead to contradictions. DHCP server is not required to follow the order of 55, but every option MUST be tried to inserted to where client wrote it, and only if there is none, then append. Please provide any other interpretation that has ho self-contradictions.
The DHCP server is not required to return the options in the requested order, but MUST try to insert the requested options _in the order requested by the client_. I mean, "not required to return the options in the requested order" in your interp would contradict with "MUST try to insert the requested options _in the order requested by the client_"
The only way to avoid the contradiction, is to admit that "in the order requested by the client" nolonger belongs to 55, but rather to the client's request. Everything else leads to a contradiction of "not required" vs "MUST provide in the order requested by client".
(In reply to stsp from comment #29) > > Where, why, how? > > For example if client provided option > 53, then you MUST write option 53 to > the exactly same location. I understand what you mean, but that's not the case. I'm attaching two captures showing the behaviour of two different DHCP servers, one is the one included in Android, the other one is ISC's DHCP server. The client is NetworkManager in both cases. Look at the offsets of option 12 in client request and server responses -- it never matches. Option 53 happens to match because both implementations add it at the very beginning, but that's an implementation detail. If somebody relies on that behaviour, I'm happy to change the order, but it has nothing to do with following whatever placeholder, because there's no such concept in any normative reference. > > Can you point to a reference? > > Only the RFC quote we discuss. ...which describes option 55. > I maintain it says what I see there, > and obviously at least some dhcp authors > are also reading it the same way as myself. That's not an indication. Expecting option 53 as first option might be reasonable because that option describes the message type, but that's not true for any other option or "placeholder". > > A client including an option is *not* a way to request it back! > > Sure thing! > I never claimed the opposite. > Only IFF you are going to return that > option anyway (not because it was sent > by the client!), then you must use the > same offset. This is not required anywhere -- until you can show me a reference, or a common server implementing that. > So this is not a way to request option! > This is a way to request the offset for > an option. > > > The word "try" here simply refers to possible implementation issues > > How would you in this case interpret > The DHCP > server is not required to return the options in the requested order, > but MUST try to insert the requested options _in the order requested > by the client_. > > Not required, but MUST try to insert? > In your interpretation this part will > most likely lead to a contradiction. Why? If it's possible in my implementation (and, perhaps more relevantly, in case of DHCP Option Overload, where you need use split areas -- think of fitting e.g. domain names), I must try to insert options according to the order they were requested by option 55. If it's not possible, they will be in another order. There's no contradiction whatsoever. > But in my (and also dhcp client's author) > intepretation, its perfectly clear and > does not lead to contradictions. > DHCP server is not required to follow > the order of 55 It is, if possible, because: - option 55 is used by the client to request options - options should be inserted in the order requested by the client hence, options should be inserted in the order requested by the client by option 55.
Created attachment 30 [details] Behaviour of Android's DHCP server
Created attachment 31 [details] Behaviour of ISC's DHCP server
(In reply to stsp from comment #31) > The only way to avoid the contradiction, > is to admit that "in the order requested by the client" Again, it seems to me that you're missing the fact that the *only* way the client has to request options is to list them in option 55. Nothing else is documented, nothing else is requested, nothing else is even typical (see the captures).
> Why? If it's possible in my implementation Because in your impl (and any other I can think of) 55 list is linearly processed. In what case you "insert"? You don't, you process 55 linearly, and only _append_ any new option seen on the 55 list. In what case would you ever "insert"? So I can't correlate the "insert" operation with the linear list processing. In my interpretation, however, "insert" makes perfect sense because you insert to where client put it in, and only otherwise append. > Look at the offsets of option 12 in client request and server responses -- > it never matches. OK so definitely not all authors read it the same way as I do. :) But I still don't understand why "insert" is needed in your interpretation. > Again, it seems to me that you're missing the fact that the *only* way > the client has to request options is to list them in option 55. No, I am not missing that fact, why do you think so? I am only speaking about an "insert" operarion and of the way to reserve the location for insertions.
Anyway, if other DHCP servers are not doing it that way, then, no matter if my interpretation is good or bad, it doesn't match the reality. It a bit upsets me to consider such client as "buggy", whereas in my eyes it uses the most pedantic interpretation of all possible. But if real-world DHCP servers makes it buggy, then so be it. So just add the 53 at the beginning then?
Maybe it would be more fair to not call it "buggy" in comments, but say that it interprets the spec differently. That would make me calm. :)
So this then? diff --git a/dhcp.c b/dhcp.c index 46c258e..b99b621 100644 --- a/dhcp.c +++ b/dhcp.c @@ -149,6 +149,8 @@ static int fill(struct msg *m) for (o = 0; o < 255; o++) opts[o].sent = 0; + if (!memchr(opts[55].c, 53, opts[55].clen)) + fill_one(m, 53, &offset); for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; if (opts[o].slen)
Or this? diff --git a/dhcp.c b/dhcp.c index 46c258e..ecb4674 100644 --- a/dhcp.c +++ b/dhcp.c @@ -46,6 +46,7 @@ struct opt { uint8_t s[255]; int clen; uint8_t c[255]; + size_t opt_off; }; static struct opt opts[255]; @@ -67,14 +68,14 @@ static struct opt opts[255]; */ void dhcp_init(void) { - opts[1] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, }; /* Mask */ - opts[3] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, }; /* Router */ + opts[1] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, 0 }; /* Mask */ + opts[3] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, 0 }; /* Router */ opts[51] = (struct opt) { 0, 4, { 0xff, 0xff, 0xff, - 0xff }, 0, { 0 }, }; /* Lease time */ - opts[53] = (struct opt) { 0, 1, { 0 }, 0, { 0 }, }; /* Type */ - opts[54] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, }; /* Server ID */ + 0xff }, 0, { 0 }, 0 }; /* Lease time */ + opts[53] = (struct opt) { 0, 1, { 0 }, 0, { 0 }, 0 }; /* Type */ + opts[54] = (struct opt) { 0, 4, { 0 }, 0, { 0 }, 0 }; /* Server ID */ } /** @@ -149,6 +150,9 @@ static int fill(struct msg *m) for (o = 0; o < 255; o++) opts[o].sent = 0; + if (!memchr(opts[55].c, 53, opts[55].clen) && opts[53].clen + && !opts[53].opt_off) + fill_one(m, 53, &offset); for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; if (opts[o].slen) @@ -318,6 +322,7 @@ int dhcp(const struct ctx *c, const struct pool *p) memcpy(&opts[*type].c, val, *olen); opts[*type].clen = *olen; + opts[*type].opt_off = opt_off; opt_off += *olen + 2; }
(In reply to stsp from comment #36) > > Why? If it's possible in my implementation > > Because in your impl (and any other > I can think of) 55 list is linearly > processed. In what case you "insert"? I think "insert" is not used in that sense: it rather means to "insert in the message" (see also the description for option 52 -- that would make little sense otherwise). That can mean append (insert at the end), prefix (insert at the beginning), or simply insert somewhere that's neither the end nor the beginning. In English, and many other languages, one might use synonyms to avoid repetitions. That's why RFC 2132 continuously alternates between "return", "insert", "use" -- but they don't have such a specific meaning. > You don't, you process 55 linearly, > and only _append_ any new option seen > on the 55 list. > In what case would you ever "insert"? In that specific sense you mean: never. They're not talking about a buffer, they're talking about a message. To "insert" something in a message might simply mean adding to it. > So I can't correlate the "insert" > operation with the linear list processing. > In my interpretation, however, "insert" > makes perfect sense because you insert > to where client put it in, and only > otherwise append. ...but again, those are two separate messages, and the standards (RFC 2131 in particular) is actually pretty clear about how you form the (separate!) response message. > > Look at the offsets of option 12 in client request and server responses -- > > it never matches. > > OK so definitely not all authors read > it the same way as I do. :) But I still > don't understand why "insert" is needed > in your interpretation. It's not needed! It's just a rather free choice of words. It's less accurate, but not wrong. (In reply to stsp from comment #37) > It a bit upsets me to consider such > client as "buggy", whereas in my eyes > it uses the most pedantic interpretation > of all possible. ...on the other hand nowhere the standard seems to allow ignoring an option just because it's not in the (supposedly -- again, not in my interpretation) expected position. That's what makes it buggy to me. But I don't really care about that -- I would just say in a comment that this is for compatibility with the given client (just mentioning the name). (In reply to stsp from comment #39) > So this then? > > diff --git a/dhcp.c b/dhcp.c > index 46c258e..b99b621 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -149,6 +149,8 @@ static int fill(struct msg *m) > for (o = 0; o < 255; o++) > opts[o].sent = 0; > > + if (!memchr(opts[55].c, 53, opts[55].clen)) > + fill_one(m, 53, &offset); > for (i = 0; i < opts[55].clen; i++) { > o = opts[55].c[i]; > if (opts[o].slen) I would rather go with this, yes, thanks, I didn't think of using memchr(). The other implementation looks a bit too much to me, and unnecessarily so. By the way, while we validate 'clen' to avoid exceeding the data passed by the client in the request, and while its maximum value is 255 (which is the size of our static buffer, so we can't overrun it), this *might* (it's a bit complicated to prove) give a reliable way for a client to know if a previous client had '53' in its option 55. There's no disclosure of anything useful, but for general hygiene, we should perhaps zero out the rest of the buffers before copying option values to them. I can take care of this in a separate patch.
> I would rather go with this, yes, thanks, I didn't think of using memchr(). > The other implementation looks a bit too much to me, and unnecessarily so. The reason is to minimize the impact on other clients. We can go for the minimal fix, but I think this will break your interpretation completely, because in that case we will never actually follow the 55 order. Some googling reveals that mandatory options (like 53) are not put into 55, so our memchr() band-aid isn't really effective. With that change we will always put 53 in front, making 55 rather useless. Note: I think 55 is actually quite useless anyway. libslirp doesn't even check it. So in my second patch I enforce the band-aid to only my particular client. So if you prefer the minimal fix... maybe then just ignore 55 completely? I don't see how it would still be useful. Also one question that still bugs me, is how could this client ever work at all in the world of your interpretation? If it did then certainly some other readings were in effect sometime...
(In reply to stsp from comment #42) > > I would rather go with this, yes, thanks, I didn't think of using memchr(). > > The other implementation looks a bit too much to me, and unnecessarily so. > > The reason is to minimize the impact > on other clients. We can go for the > minimal fix, but I think this will > break your interpretation completely, > because in that case we will never > actually follow the 55 order. Some > googling reveals that mandatory options > (like 53) are not put into 55, so our > memchr() band-aid isn't really effective. > With that change we will always put > 53 in front, making 55 rather useless. Well, wait, we put 53 in front if the client didn't request it via option 55 (and thus asking that it's in a specific order -- which is the only case we need to cover with the memchr()). Let's say a client requests options 1 and 6 -- then we'll reply with options 53, 1, and 6, in this order, which still complies with RFC 2132 -- because the *requested options* are in order. Option 53 wasn't requested. Of course it's unlikely that a client would request option 53, because it's mandatory in responses. But... it's not forbidden. > Note: I think 55 is actually quite > useless anyway. libslirp doesn't even > check it. It's helpful if you have a very constrained implementation for the client where storing the reply is expensive or complicated to implement (think of a microcontroller where for whatever reason compiling isn't practical, so you'd write that in assembly, or where you really don't have 100 bytes easily available). Let's say you need option 1 before option 121, because configuration of static routes needs to be done after configuring an address, but you parse a few bytes at a time and you can't store option 121. > So in my second patch I enforce the > band-aid to only my particular client. > > So if you prefer the minimal fix... > maybe then just ignore 55 completely? > I don't see how it would still be useful. I agree it's rarely useful, but standard compliance is still important because we have no idea what client might talk to us. > Also one question that still bugs me, > is how could this client ever work at > all in the world of your interpretation? > If it did then certainly some other > readings were in effect sometime... Well, as you've seen, most DHCP servers answer with option 53 first, anyway. It looks like passt is an exception.
> Well, wait, we put 53 in front if the client didn't request it via option 55 ... which is always the case. > Of course it's unlikely that a client would request option 53, because it's > mandatory in responses. But... it's not forbidden. It will not start doing so because of our change. Such change would affect most, if not all, current clients. > I agree it's rarely useful, but standard compliance is still important > because we have no idea what client might talk to us. What kind of standard compliance remain once you violated 55 and put 53 in the front? My second patch retains the standard compliance, in your interp at least. > Well, as you've seen, most DHCP servers answer with option 53 first, anyway. What is the logic there? I've only seen that in libslirp, but it ignores 55, so the logic is simple. What do those who do support 55?
(In reply to stsp from comment #44) > > Well, wait, we put 53 in front if the client didn't request it via option 55 > > ... which is always the case. ...as far as you googled around, yes. > > Of course it's unlikely that a client would request option 53, because it's > > mandatory in responses. But... it's not forbidden. > > It will not start doing so because > of our change. Such change would affect > most, if not all, current clients. Most, right. > > I agree it's rarely useful, but standard compliance is still important > > because we have no idea what client might talk to us. > > What kind of standard compliance remain > once you violated 55 and put 53 in the > front? Adding 53 in front doesn't violate the requirement, as long as 53 is not requested, because the *requested options* would still be in the requested order. Example: given the sequence A := "1, 6, 121", the sequence B := "53, 1, 6, 121" includes sequence A in order. > My second patch retains the > standard compliance, in your interp at > least. It introduces a dependency between order of options in the request (which is a different *type* of message) and order of options in the reply, which is something not documented and not requested anywhere, as I keep repeating... > > Well, as you've seen, most DHCP servers answer with option 53 first, anyway. > > What is the logic there? > I've only seen that in libslirp, but it > ignores 55, so the logic is simple. What > do those who do support 55? I don't know exactly, ISC's DHCP server seems to actually order some requested options in the order given by option 55, but I'm not terribly interested -- there's a well established standard and the implementation is 1. trivial 2. already there.
> because the *requested options* would still be in the requested order. Well, this is a very, very interesting interpretation of an RFC. :) I don't think its a useful interpretation though, as some client may (and mine obviously does) use the fixed offsets to get the options data, after requesting the particular order with 55. We would break such clients, even if in your interpretation the order is kinda still preserved. And this is the ONLY reason I proposed the second version of the patch. If you think there are no (more than 1 I've found) clients that use fixed offsets, then lets apply the simple change. But me worries. Any client that was implemented in an asm, would likely seek the way to use fixed offsets, as in asm that saves quite a code chunk. > It introduces a dependency between order of options in the request > (which is a different *type* of message) and order of options in the reply Not really. Its basically a heuristic to detect my particular client, and not to affect any other one. The heuristic is: - client have not listed 53 in 55 (almost always the case) - client put 53 as the first option (dunno, if this fingerpring is also not too specific, then such heuristic is useless)
(In reply to stsp from comment #46) > > because the *requested options* would still be in the requested order. > > Well, this is a very, very interesting > interpretation of an RFC. :) It's really the plain meaning of, quote, "the requested options in the order requested by the client". > I don't > think its a useful interpretation though, See comment #43 for an explanation of why it's useful (I have myself implemented a DHCP client like that in the past, I couldn't conveniently store parsed values, so 121 needed to be after option 1 and a few others). > as some client may (and mine obviously > does) use the fixed offsets to get the > options data, after requesting the particular > order with 55. We would break such clients, > even if in your interpretation the order is > kinda still preserved. You won't break anything known, because the only buggy behaviour from a client we know about is to expect 53 in a fixed position, and that's what we can work around here. > And this is the ONLY reason I proposed > the second version of the patch. > If you think there are no (more than 1 > I've found) clients that use fixed offsets, > then lets apply the simple change. > But me worries. Any client that was > implemented in an asm, would likely seek > the way to use fixed offsets, as in asm > that saves quite a code chunk. Sure, but that's a totally unreasonable assumption. Option 53 is somewhat special because it indicates the type of message, so the interpretation of other options depend on it. If somebody else uses a fixed offset for, say, option 54 (a bit less reasonably), we'll find out and work around it too. > > > It introduces a dependency between order of options in the request > > (which is a different *type* of message) and order of options in the reply > > Not really. > Its basically a heuristic to detect my > particular client, and not to affect any > other one. The heuristic is: > - client have not listed 53 in 55 (almost always the case) > - client put 53 as the first option (dunno, if this fingerpring is also not > too specific, then such heuristic is useless) Most clients seem to do that anyway. It shouldn't be taken as an indication of anything specific.
> It's really the plain meaning of, quote, "the requested options in the > order requested by the client". Its quite bad that RFC seems to be not telling anything about a "mandatory" options, so everyone deals with them on his own... Its not even clear which exactly options are mandatory and which are not. > Sure, but that's a totally unreasonable assumption. Option 53 is somewhat > special because it indicates the type of message, so the interpretation > of other options depend on it. It shouldn't have been an option then... I suppose all we know is that an RFC is quite broken. > If somebody else uses a fixed offset for, say, option 54 (a bit less > reasonably), we'll find out and work around it too. OK, then lets apply a simple fix.
(In reply to stsp from comment #48) > > It's really the plain meaning of, quote, "the requested options in the > > order requested by the client". > > Its quite bad that RFC seems to be > not telling anything about a "mandatory" > options Well, some are clearly marked as such, but yes: > > Sure, but that's a totally unreasonable assumption. Option 53 is somewhat > > special because it indicates the type of message, so the interpretation > > of other options depend on it. > > It shouldn't have been an option then... ...the RFC 951 legacy makes it much more complicated than what it could be. > I suppose all we know is that an RFC is > quite broken. > > > If somebody else uses a fixed offset for, say, option 54 (a bit less > > reasonably), we'll find out and work around it too. > > OK, then lets apply a simple fix. Will you post a patch, or should I? It would be good to indicate what DHCP client this is for, in a comment. I suppose it all comes from mTCP, and this check in APPS/DHCP/DHCP.CPP: if ( resp->options[0] != 53 ) { TRACE_WARN(( "Dhcp: first option was not a Dhcp msg type\n" )); return; } correct?
> correct? No, seems to be wattcp-32, with this code: /** * Return TRUE if DHCP message is an ACK. */ static int DHCP_is_ack (void) { const BYTE *opt = (const BYTE*) &dhcp_in.dh_opt[4]; return (opt[0] == DHCP_OPT_MSG_TYPE && opt[1] == 1 && opt[2] == DHCP_ACK); } /** * Return TRUE if DHCP message is a NACK. */ static int DHCP_is_nack (void) { const BYTE *opt = (const BYTE*) &dhcp_in.dh_opt[4]; return (opt[0] == DHCP_OPT_MSG_TYPE && opt[1] == 1 && opt[2] == DHCP_NAK); } Will post a patch later, ok.
Patch sent. It might be a good idea to add where to send patches, to the README.md.
(In reply to stsp from comment #51) > Patch sent. Thanks, it looks good to me, I'll give it some time for reviews before applying. > It might be a good idea to add > where to send patches, to the > README.md. It's actually written in the README.md, but missing in README.plain.md. This would be solved by: https://archives.passt.top/passt-dev/20230628140727.15750-1-kuhnchris+git@kuhnchris.eu/#Z31makedocs.pl ...but nobody had the time to give another spin to that patch yet.
Fixed in 2023_10_04.f851084.