* [musl] [PATCH] dns: check length field in tcp response message @ 2023-03-22 12:29 Alexey Kodanev 2023-03-22 13:16 ` Rich Felker 0 siblings, 1 reply; 7+ messages in thread From: Alexey Kodanev @ 2023-03-22 12:29 UTC (permalink / raw) To: musl; +Cc: Alexey Kodanev The received length field in the message may be greater than the size of the 'answer' buffer in which the message resides. Currently, ABUF_SIZE is 768. And if we get a larger 'alen', it will result in an out-of-bounds reading during parsing, because 'alen' will be passed to __dns_parse() later: __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); To fix this, limit 'alen' to the size of the received buffer. --- src/network/res_msend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/network/res_msend.c b/src/network/res_msend.c index fef7e3a2..291853de 100644 --- a/src/network/res_msend.c +++ b/src/network/res_msend.c @@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries, int rcode = answers[i][3] & 15; if (rcode != 0 && rcode != 3) goto out; + if (alen > asize) alen = asize; /* Storing the length here commits the accepted answer. * Immediately close TCP socket so as not to consume -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [musl] [PATCH] dns: check length field in tcp response message 2023-03-22 12:29 [musl] [PATCH] dns: check length field in tcp response message Alexey Kodanev @ 2023-03-22 13:16 ` Rich Felker 2023-03-22 13:48 ` Alexey Kodanev 2023-05-08 16:25 ` Alexey Izbyshev 0 siblings, 2 replies; 7+ messages in thread From: Rich Felker @ 2023-03-22 13:16 UTC (permalink / raw) To: Alexey Kodanev; +Cc: musl On Wed, Mar 22, 2023 at 03:29:16PM +0300, Alexey Kodanev wrote: > The received length field in the message may be greater than the > size of the 'answer' buffer in which the message resides. Currently, > ABUF_SIZE is 768. And if we get a larger 'alen', it will result > in an out-of-bounds reading during parsing, because 'alen' will > be passed to __dns_parse() later: > > __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); > > To fix this, limit 'alen' to the size of the received buffer. > --- > src/network/res_msend.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/network/res_msend.c b/src/network/res_msend.c > index fef7e3a2..291853de 100644 > --- a/src/network/res_msend.c > +++ b/src/network/res_msend.c > @@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries, > int rcode = answers[i][3] & 15; > if (rcode != 0 && rcode != 3) > goto out; > + if (alen > asize) alen = asize; > > /* Storing the length here commits the accepted answer. > * Immediately close TCP socket so as not to consume > -- > 2.25.1 This is incorrect. It breaks res_send, whose contract is to return the full answer length even if it did not fit, so that the caller can retry with the appropriate size. Instead, name_from_dns just needs to clamp the value before passing it to __dns_parse. Rich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [musl] [PATCH] dns: check length field in tcp response message 2023-03-22 13:16 ` Rich Felker @ 2023-03-22 13:48 ` Alexey Kodanev 2023-03-22 14:07 ` Rich Felker 2023-05-08 16:25 ` Alexey Izbyshev 1 sibling, 1 reply; 7+ messages in thread From: Alexey Kodanev @ 2023-03-22 13:48 UTC (permalink / raw) To: Rich Felker; +Cc: musl Hi Rich, On 22.03.2023 16:16, Rich Felker wrote: > On Wed, Mar 22, 2023 at 03:29:16PM +0300, Alexey Kodanev wrote: >> The received length field in the message may be greater than the >> size of the 'answer' buffer in which the message resides. Currently, >> ABUF_SIZE is 768. And if we get a larger 'alen', it will result >> in an out-of-bounds reading during parsing, because 'alen' will >> be passed to __dns_parse() later: >> >> __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); >> >> To fix this, limit 'alen' to the size of the received buffer. >> --- >> src/network/res_msend.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/network/res_msend.c b/src/network/res_msend.c >> index fef7e3a2..291853de 100644 >> --- a/src/network/res_msend.c >> +++ b/src/network/res_msend.c >> @@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries, >> int rcode = answers[i][3] & 15; >> if (rcode != 0 && rcode != 3) >> goto out; >> + if (alen > asize) alen = asize; >> >> /* Storing the length here commits the accepted answer. >> * Immediately close TCP socket so as not to consume >> -- >> 2.25.1 > > This is incorrect. It breaks res_send, whose contract is to return the > full answer length even if it did not fit, so that the caller can > retry with the appropriate size. > > Instead, name_from_dns just needs to clamp the value before passing it > to __dns_parse. > OK, I see, something like this or better with sizeof *abuf? diff --git a/src/network/lookup_name.c b/src/network/lookup_name.c index 5f6867cb..65b3e8fb 100644 --- a/src/network/lookup_name.c +++ b/src/network/lookup_name.c @@ -175,6 +175,7 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static if (alens[i] < 4 || (abuf[i][3] & 15) == 2) return EAI_AGAIN; if ((abuf[i][3] & 15) == 3) return 0; if ((abuf[i][3] & 15) != 0) return EAI_FAIL; + if (alens[i] > ABUF_SIZE) alens[i] = ABUF_SIZE; } for (i=nq-1; i>=0; i--) { ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [musl] [PATCH] dns: check length field in tcp response message 2023-03-22 13:48 ` Alexey Kodanev @ 2023-03-22 14:07 ` Rich Felker 0 siblings, 0 replies; 7+ messages in thread From: Rich Felker @ 2023-03-22 14:07 UTC (permalink / raw) To: Alexey Kodanev; +Cc: musl On Wed, Mar 22, 2023 at 04:48:00PM +0300, Alexey Kodanev wrote: > Hi Rich, > On 22.03.2023 16:16, Rich Felker wrote: > > On Wed, Mar 22, 2023 at 03:29:16PM +0300, Alexey Kodanev wrote: > >> The received length field in the message may be greater than the > >> size of the 'answer' buffer in which the message resides. Currently, > >> ABUF_SIZE is 768. And if we get a larger 'alen', it will result > >> in an out-of-bounds reading during parsing, because 'alen' will > >> be passed to __dns_parse() later: > >> > >> __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); > >> > >> To fix this, limit 'alen' to the size of the received buffer. > >> --- > >> src/network/res_msend.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/src/network/res_msend.c b/src/network/res_msend.c > >> index fef7e3a2..291853de 100644 > >> --- a/src/network/res_msend.c > >> +++ b/src/network/res_msend.c > >> @@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries, > >> int rcode = answers[i][3] & 15; > >> if (rcode != 0 && rcode != 3) > >> goto out; > >> + if (alen > asize) alen = asize; > >> > >> /* Storing the length here commits the accepted answer. > >> * Immediately close TCP socket so as not to consume > >> -- > >> 2.25.1 > > > > This is incorrect. It breaks res_send, whose contract is to return the > > full answer length even if it did not fit, so that the caller can > > retry with the appropriate size. > > > > Instead, name_from_dns just needs to clamp the value before passing it > > to __dns_parse. > > > > OK, I see, something like this or better with sizeof *abuf? > > diff --git a/src/network/lookup_name.c b/src/network/lookup_name.c > index 5f6867cb..65b3e8fb 100644 > --- a/src/network/lookup_name.c > +++ b/src/network/lookup_name.c > @@ -175,6 +175,7 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static > if (alens[i] < 4 || (abuf[i][3] & 15) == 2) return EAI_AGAIN; > if ((abuf[i][3] & 15) == 3) return 0; > if ((abuf[i][3] & 15) != 0) return EAI_FAIL; > + if (alens[i] > ABUF_SIZE) alens[i] = ABUF_SIZE; > } > > for (i=nq-1; i>=0; i--) { I don't think it matters a whole lot which you use. sizeof abuf[i] might be the most clear. But I would move it down to the next loop that's about use of the results rather than the error-checking loop. Rich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [musl] [PATCH] dns: check length field in tcp response message 2023-03-22 13:16 ` Rich Felker 2023-03-22 13:48 ` Alexey Kodanev @ 2023-05-08 16:25 ` Alexey Izbyshev 2024-02-29 10:49 ` Alexey Izbyshev 1 sibling, 1 reply; 7+ messages in thread From: Alexey Izbyshev @ 2023-05-08 16:25 UTC (permalink / raw) To: musl; +Cc: Alexey Kodanev [-- Attachment #1: Type: text/plain, Size: 1528 bytes --] On 2023-03-22 16:16, Rich Felker wrote: > On Wed, Mar 22, 2023 at 03:29:16PM +0300, Alexey Kodanev wrote: >> The received length field in the message may be greater than the >> size of the 'answer' buffer in which the message resides. Currently, >> ABUF_SIZE is 768. And if we get a larger 'alen', it will result >> in an out-of-bounds reading during parsing, because 'alen' will >> be passed to __dns_parse() later: >> >> __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); >> >> To fix this, limit 'alen' to the size of the received buffer. >> --- >> src/network/res_msend.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/network/res_msend.c b/src/network/res_msend.c >> index fef7e3a2..291853de 100644 >> --- a/src/network/res_msend.c >> +++ b/src/network/res_msend.c >> @@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const unsigned >> char *const *queries, >> int rcode = answers[i][3] & 15; >> if (rcode != 0 && rcode != 3) >> goto out; >> + if (alen > asize) alen = asize; >> >> /* Storing the length here commits the accepted answer. >> * Immediately close TCP socket so as not to consume >> -- >> 2.25.1 > > This is incorrect. It breaks res_send, whose contract is to return the > full answer length even if it did not fit, so that the caller can > retry with the appropriate size. > > Instead, name_from_dns just needs to clamp the value before passing it > to __dns_parse. > Not only name_from_dns, but also getnameinfo. The patch is attached. Alexey [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-getnameinfo-fix-calling-__dns_parse-with-potentially.patch --] [-- Type: text/x-diff; name=0001-getnameinfo-fix-calling-__dns_parse-with-potentially.patch, Size: 1238 bytes --] From 3073d26361bcb9e9f1e9ab998440ae3b321fe830 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev <izbyshev@ispras.ru> Date: Mon, 8 May 2023 19:03:46 +0300 Subject: [PATCH] getnameinfo: fix calling __dns_parse with potentially too large rlen Mail-Followup-To: musl@lists.openwall.com __res_send returns the full answer length even if it didn't fit the buffer, but __dns_parse expects the length of the filled part of the buffer. This is analogous to commit 77327ed064bd57b0e1865cd0e0364057ff4a53b4, which fixed the only other __dns_parse call site. --- src/network/getnameinfo.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/network/getnameinfo.c b/src/network/getnameinfo.c index 7abe0fa9..133c15b3 100644 --- a/src/network/getnameinfo.c +++ b/src/network/getnameinfo.c @@ -162,8 +162,10 @@ int getnameinfo(const struct sockaddr *restrict sa, socklen_t sl, query[3] = 0; /* don't need AD flag */ int rlen = __res_send(query, qlen, reply, sizeof reply); buf[0] = 0; - if (rlen > 0) + if (rlen > 0) { + if (rlen > sizeof reply) rlen = sizeof reply; __dns_parse(reply, rlen, dns_parse_callback, buf); + } } if (!*buf) { if (flags & NI_NAMEREQD) return EAI_NONAME; -- 2.39.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [musl] [PATCH] dns: check length field in tcp response message 2023-05-08 16:25 ` Alexey Izbyshev @ 2024-02-29 10:49 ` Alexey Izbyshev 2024-02-29 15:36 ` Rich Felker 0 siblings, 1 reply; 7+ messages in thread From: Alexey Izbyshev @ 2024-02-29 10:49 UTC (permalink / raw) To: musl; +Cc: Alexey Kodanev On 2023-05-08 19:25, Alexey Izbyshev wrote: > On 2023-03-22 16:16, Rich Felker wrote: >> On Wed, Mar 22, 2023 at 03:29:16PM +0300, Alexey Kodanev wrote: >>> The received length field in the message may be greater than the >>> size of the 'answer' buffer in which the message resides. Currently, >>> ABUF_SIZE is 768. And if we get a larger 'alen', it will result >>> in an out-of-bounds reading during parsing, because 'alen' will >>> be passed to __dns_parse() later: >>> >>> __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); >>> >>> To fix this, limit 'alen' to the size of the received buffer. >>> --- >>> src/network/res_msend.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/src/network/res_msend.c b/src/network/res_msend.c >>> index fef7e3a2..291853de 100644 >>> --- a/src/network/res_msend.c >>> +++ b/src/network/res_msend.c >>> @@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const unsigned >>> char *const *queries, >>> int rcode = answers[i][3] & 15; >>> if (rcode != 0 && rcode != 3) >>> goto out; >>> + if (alen > asize) alen = asize; >>> >>> /* Storing the length here commits the accepted answer. >>> * Immediately close TCP socket so as not to consume >>> -- >>> 2.25.1 >> >> This is incorrect. It breaks res_send, whose contract is to return the >> full answer length even if it did not fit, so that the caller can >> retry with the appropriate size. >> >> Instead, name_from_dns just needs to clamp the value before passing it >> to __dns_parse. >> > Not only name_from_dns, but also getnameinfo. The patch is attached. > Pinging due to the approaching release. Did the patch fall through cracks? Thanks, Alexey ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [musl] [PATCH] dns: check length field in tcp response message 2024-02-29 10:49 ` Alexey Izbyshev @ 2024-02-29 15:36 ` Rich Felker 0 siblings, 0 replies; 7+ messages in thread From: Rich Felker @ 2024-02-29 15:36 UTC (permalink / raw) To: musl On Thu, Feb 29, 2024 at 01:49:53PM +0300, Alexey Izbyshev wrote: > On 2023-05-08 19:25, Alexey Izbyshev wrote: > >On 2023-03-22 16:16, Rich Felker wrote: > >>On Wed, Mar 22, 2023 at 03:29:16PM +0300, Alexey Kodanev wrote: > >>>The received length field in the message may be greater than the > >>>size of the 'answer' buffer in which the message resides. Currently, > >>>ABUF_SIZE is 768. And if we get a larger 'alen', it will result > >>>in an out-of-bounds reading during parsing, because 'alen' will > >>>be passed to __dns_parse() later: > >>> > >>> __dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx); > >>> > >>>To fix this, limit 'alen' to the size of the received buffer. > >>>--- > >>> src/network/res_msend.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>>diff --git a/src/network/res_msend.c b/src/network/res_msend.c > >>>index fef7e3a2..291853de 100644 > >>>--- a/src/network/res_msend.c > >>>+++ b/src/network/res_msend.c > >>>@@ -297,6 +297,7 @@ int __res_msend_rc(int nqueries, const > >>>unsigned char *const *queries, > >>> int rcode = answers[i][3] & 15; > >>> if (rcode != 0 && rcode != 3) > >>> goto out; > >>>+ if (alen > asize) alen = asize; > >>> > >>> /* Storing the length here commits the accepted answer. > >>> * Immediately close TCP socket so as not to consume > >>>-- > >>>2.25.1 > >> > >>This is incorrect. It breaks res_send, whose contract is to return the > >>full answer length even if it did not fit, so that the caller can > >>retry with the appropriate size. > >> > >>Instead, name_from_dns just needs to clamp the value before passing it > >>to __dns_parse. > >> > >Not only name_from_dns, but also getnameinfo. The patch is attached. > > > Pinging due to the approaching release. Did the patch fall through > cracks? > > Thanks, > Alexey Thanks, applying! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-29 15:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-22 12:29 [musl] [PATCH] dns: check length field in tcp response message Alexey Kodanev 2023-03-22 13:16 ` Rich Felker 2023-03-22 13:48 ` Alexey Kodanev 2023-03-22 14:07 ` Rich Felker 2023-05-08 16:25 ` Alexey Izbyshev 2024-02-29 10:49 ` Alexey Izbyshev 2024-02-29 15:36 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).