From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=FLhGO/kb; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 793695A026F for ; Sat, 01 Feb 2025 14:13:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1738415618; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XiUBwvm/YiydQsfz5tMZfLz5coTgd07666QOr5qi4m0=; b=FLhGO/kbsnz1ULOb6o4Bz1vdrXzzDFn/2UuF0PKxmoTH8UF6wzMnZWPaffjjZs/Hw29aNr J387sI37rb3Z/fXBcGLL3VmiszGecQGH3HBpeMkRwGWRSPW/m4nrPaxVxurvU5d3er8ySI CPlPJhHmBcrBTxuDiwrP3L3HAxPZ0Kg= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-662-L2GBOK4APbu4L3QVRXsotw-1; Sat, 01 Feb 2025 08:13:36 -0500 X-MC-Unique: L2GBOK4APbu4L3QVRXsotw-1 X-Mimecast-MFC-AGG-ID: L2GBOK4APbu4L3QVRXsotw Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-436289a570eso25371885e9.0 for ; Sat, 01 Feb 2025 05:13:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738415614; x=1739020414; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=XiUBwvm/YiydQsfz5tMZfLz5coTgd07666QOr5qi4m0=; b=Jm205Y7fhrFR6C5tcYInluMrMyeMzEVNmEwOO4oWLFhydQE8MEFEsrM3rVqBu2R5VB sUJe0Thzys14lrrqpowvcl/Rvad1uq4YAT8cfwFcHzE+/RjbmHMvX7ePKPkOxA9hQmqw 0TXJL4VIJ9XjHS6UazkOr7hATD+Rj+h0754S0UBqATXe5wbQU2Of0iCyKsHcnepTToze D/7epa/lCrATHGMXC4Grds9XzfSdI9fE+C2za4Wc2cQV17TGiHkNYzD/YT4xe1lKMB2J 1qu6kWK3guCbwjUEwqc0ewq0Id+/CygDv00MubsSlwJQJLXs0N8X3c/iu/Juh+VRz5Yz ZKdA== X-Gm-Message-State: AOJu0Yw/5tAf35doW3p75uOgP2IKb39F4m47f73BkJGjaUkIfZ5v1x5G LuRar88OsYvxADsafc9KrX9r0OVvnYhI1GNDlK/DhSn9LhCZo1gb18YhlF7KVaNcnlEQwGW/FID l5j7BjHrHLjWajeQIA1D/yNOuW834elIfbwP9VD9l/YXWAu1gU2lBnaYczIf1i5sJS7LOUTsnmT aw+cjcby2u0ql6dNRr6+P6Gj9r38THbQde X-Gm-Gg: ASbGncvo3s0UNxELJIF1vH6waEiPsqc+WHOyF9CxHofLeKPAs4bvn5Ha1GXZb8HqX2C Ki9v6uC8c6bmSX6G6+DXeGA7FcamNEeviWxTaJ3B6Tom0Tmtd4V/tqnCexN1MHRdVUD8RTmI6tW IvL1NUtKG1siAP2a4M3IpgWTd63YZ2NxripylJPGb5Fb7h0IUqXYM7y3EVntO5/Qyc5IhOI+Zii j8sqfzMIHlFUq7V2yQBeu9zHMWJYz8ftScTPuIYFErE1tjPltFqTCPseNBej72IBrEfXCsQJa18 o5ApvcsStHJ9s2JiyRd2yxZRrqn9DLSt+Q== X-Received: by 2002:a05:600c:4713:b0:434:a852:ba77 with SMTP id 5b1f17b1804b1-438dc3cadf9mr163162495e9.15.1738415614406; Sat, 01 Feb 2025 05:13:34 -0800 (PST) X-Google-Smtp-Source: AGHT+IGuIcIuuHtHJ7FeAs228swJVcVyPddvgLjMV3VOoSzviz/ilNBQsrSdH43Buwgy/Bd5NnILrw== X-Received: by 2002:a05:600c:4713:b0:434:a852:ba77 with SMTP id 5b1f17b1804b1-438dc3cadf9mr163162275e9.15.1738415613924; Sat, 01 Feb 2025 05:13:33 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-438dcc2e3a6sm126969515e9.20.2025.02.01.05.13.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 01 Feb 2025 05:13:33 -0800 (PST) Date: Sat, 1 Feb 2025 14:13:30 +0100 From: Stefano Brivio To: Enrique Llorente Subject: Re: [PATCH] dhcp: Don't re-use request message for reply Message-ID: <20250201141330.59af1324@elisabeth> In-Reply-To: <20250131145329.1835558-1-ellorent@redhat.com> References: <20250131145329.1835558-1-ellorent@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: iuNZuMn6K9u673dNDX3VJHOjLgM_mkJb1Vdmh-yV3CE_1738415615 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: W7FVHKWI3VGSYEKKRLH2STBNT6QFZ5F6 X-Message-ID-Hash: W7FVHKWI3VGSYEKKRLH2STBNT6QFZ5F6 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, 31 Jan 2025 15:53:29 +0100 Enrique Llorente wrote: > The logic composing the DHCP reply message is reusing the request > message to compose the it, this kind be problematic from a security Does "be problematic" imply "would be ... once we add longer options"? > context and may break the functionality. Which one? This is important to know for distribution maintainers and, ultimately, users. As far as I know it's all fine until now, the problem would arise in your next patch, so perhaps state that. The real reason why we need this is that we want to have a given, fixed size for the option field (308) so that we can easily check we don't exceed it once we start writing the FQDN in it. > This change create a new reply message and fill it in with proper fields > from request adding on top the generated opetions. s/opetions/options/ > > Signed-off-by: Enrique Llorente > --- > dhcp.c | 55 ++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 38 insertions(+), 17 deletions(-) > > diff --git a/dhcp.c b/dhcp.c > index d8515aa..d8ff330 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -142,17 +142,36 @@ static void fill_one(struct msg *m, int o, int *offset) > } > > /** > - * fill() - Fill options in message > - * @m: Message to fill > + * fill() - Fill fields and options in response message On one hand, this fits with a function that's called "fill()". On the other hand, I would have a slight preference to keep this in dhcp() because dhcp() is a comprehensive summary (albeit a bit long) of what we're doing. Is there a particular reason why you moved non-option field assignments to here? > + * @c: Execution context to copy from > + * @req: Request message to copy from > + * @resp: Response Message to write to s/Message/message/ > * > * Return: current size of options field > */ > -static int fill(struct msg *m) > +static int fill(const struct ctx *c, struct msg const* req, Coding style (assuming you need to change this). > + struct msg *resp) > { > int i, o, offset = 0; > > - m->op = BOOTREPLY; > - m->secs = 0; > + resp->op = BOOTREPLY; > + resp->secs = 0; > + resp->hops = 0; // We are not a RELAY agent Coding style. Inconsistency between detail of messages. By this metric, the one above should also say "We reply FAST" and the one below "This is OLD". I don't think these comments really add anything. > + memset(&resp->sname, 0, sizeof(resp->sname)); > + memset(&resp->file, 0, sizeof(resp->file)); > + resp->yiaddr = c->ip4.addr; > + > + Excess newline. > + /* Copy these fields from request */ Also rather obvious, plus it would be problematic along with my suggestion below about having the fields in order. > + memcpy(&resp->chaddr, req->chaddr, sizeof(resp->chaddr)); > + resp->htype = req->htype; These look like a table, the 'resp' assignments above don't. > + resp->hlen = req->hlen; > + resp->xid = req->xid; > + resp->flags = req->flags; > + resp->ciaddr = req->ciaddr; > + resp->siaddr = req->siaddr; /* TODO server ip ? */ This needs to be 0. The issue is pre-existing, but as you're already doing this... > + resp->giaddr = req->giaddr; > + resp->magic = req->magic; Could we have *all* those in order? If one is familiar with the standard (or is reading it), it's easier to follow and find things. > for (o = 0; o < 255; o++) > opts[o].sent = 0; > @@ -162,24 +181,24 @@ 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); > + fill_one(resp, 53, &offset); > > for (i = 0; i < opts[55].clen; i++) { > o = opts[55].c[i]; > if (opts[o].slen != -1) > - fill_one(m, o, &offset); > + fill_one(resp, o, &offset); > } > > for (o = 0; o < 255; o++) { > if (opts[o].slen != -1 && !opts[o].sent) > - fill_one(m, o, &offset); > + fill_one(resp, o, &offset); > } > > - m->o[offset++] = 255; > - m->o[offset++] = 0; > + resp->o[offset++] = 255; > + resp->o[offset++] = 0; > > if (offset < OPT_MIN) { > - memset(&m->o[offset], 0, OPT_MIN - offset); > + memset(&resp->o[offset], 0, OPT_MIN - offset); > offset = OPT_MIN; > } > > @@ -291,8 +310,9 @@ int dhcp(const struct ctx *c, const struct pool *p) > const struct ethhdr *eh; > const struct iphdr *iph; > const struct udphdr *uh; > + struct msg const *m; Look at the line just above. > + struct msg resp; > unsigned int i; > - struct msg *m; > > eh = packet_get(p, 0, offset, sizeof(*eh), NULL); > offset += sizeof(*eh); > @@ -321,6 +341,7 @@ int dhcp(const struct ctx *c, const struct pool *p) > m->op != BOOTREQUEST) > return -1; > > + Stray change. > offset += offsetof(struct msg, o); > > for (i = 0; i < ARRAY_SIZE(opts); i++) > @@ -364,7 +385,6 @@ int dhcp(const struct ctx *c, const struct pool *p) > > info(" from %s", eth_ntop(m->chaddr, macstr, sizeof(macstr))); > > - m->yiaddr = c->ip4.addr; > mask.s_addr = htonl(0xffffffff << (32 - c->ip4.prefix_len)); > memcpy(opts[1].s, &mask, sizeof(mask)); > memcpy(opts[3].s, &c->ip4.guest_gw, sizeof(c->ip4.guest_gw)); > @@ -399,16 +419,17 @@ int dhcp(const struct ctx *c, const struct pool *p) > opts[6].slen = -1; > > if (!c->no_dhcp_dns_search) > - opt_set_dns_search(c, sizeof(m->o)); > + opt_set_dns_search(c, sizeof(resp.o)); > + Stray extra newline. > > - dlen = offsetof(struct msg, o) + fill(m); > + dlen = offsetof(struct msg, o) + fill(c, m, &resp); > > if (m->flags & FLAG_BROADCAST) > dst = in4addr_broadcast; > else > dst = c->ip4.addr; > - Stray change. > - tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen); > + tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, &resp, dlen); > > return 1; > } > + Same here. -- Stefano