ConnMan network manager
 help / color / Atom feed
* RE: [PATCH] replace strncopy by memcpy
@ 2021-06-18 12:38 VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-06-18 17:53 ` Santtu Lakkala
  0 siblings, 1 reply; 4+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-06-18 12:38 UTC (permalink / raw)
  To: Lukáš Karas, connman

Hello Lukáš

Thank you for your patch, but, even if I have not checked the 
related sources, I really doubt about your fix. Maybe it will not
be a problem, due to the following check:
if ((uptr + ulen) > uncomp_end)
 			goto out;

> memcpy is equivalent to strncpy here as pointed by Santtu.
Please be careful, no more warning does not always imply that
the problem has been fixed. The warning has only disappear here
because you have replaced the call to strncpy by another one.
memcpy and  strncpy are not equivalent.
strncpy is dedicated to string processing, and enables to avoid not
null terminated strings, when the string length is equal to the destination
buffer size.
The good fix is just to replace, in the strncpy call,  ulen by  ulen -1 or
strlen(name).
If you really want to propose a patch, please refer to the commits
(sources code + commit message) I mentioned
(90f0e412569b327d8b25b834623b814f1fddbd19
& 3dbcc963a66d43e4ac51c97de4fe116ec72eb9f6),
it will be a better approach.


Best Regards,

Emmanuel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] replace strncopy by memcpy
  2021-06-18 12:38 [PATCH] replace strncopy by memcpy VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-06-18 17:53 ` Santtu Lakkala
  2021-06-21  7:00   ` Daniel Wagner
  0 siblings, 1 reply; 4+ messages in thread
From: Santtu Lakkala @ 2021-06-18 17:53 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire),
	Lukáš Karas, connman

On 18.6.2021 15.38, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Thank you for your patch, but, even if I have not checked the
> related sources, I really doubt about your fix. Maybe it will not
> be a problem, due to the following check:
> if ((uptr + ulen) > uncomp_end)
>   			goto out;
> 
>> memcpy is equivalent to strncpy here as pointed by Santtu.
> Please be careful, no more warning does not always imply that
> the problem has been fixed. The warning has only disappear here
> because you have replaced the call to strncpy by another one.
> memcpy and  strncpy are not equivalent.
> strncpy is dedicated to string processing, and enables to avoid not
> null terminated strings, when the string length is equal to the destination
> buffer size.
> The good fix is just to replace, in the strncpy call,  ulen by  ulen -1 or
> strlen(name).
> If you really want to propose a patch, please refer to the commits
> (sources code + commit message) I mentioned
> (90f0e412569b327d8b25b834623b814f1fddbd19
> & 3dbcc963a66d43e4ac51c97de4fe116ec72eb9f6),
> it will be a better approach.

This is a different warning, this warning is about using strlen(src) as 
strncpy() length argument. I see a few possible fixes to acknowledge the 
warning:
  1) replace strncpy with memcpy (as done in this patch, no change in 
behavior, only theoretically slightly more performant)
  2) replace strncpy with strcpy (available space is already checked)
  3) replace strncpy length argument with available buffer space (would 
unnecessarily zero the buffer to the end)
  4) change the whole paradigm, and do something like:
     uptr += snprintf(uptr, uncomp_end - uptr, "%s", name);
     if (uptr >= uncomp_end)
         goto out;
  5) do 4 with strlcpy, but we don't have that

-- 
Santtu

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] replace strncopy by memcpy
  2021-06-18 17:53 ` Santtu Lakkala
@ 2021-06-21  7:00   ` Daniel Wagner
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Wagner @ 2021-06-21  7:00 UTC (permalink / raw)
  To: Santtu Lakkala
  Cc: VAUTRIN Emmanuel (Canal Plus Prestataire),
	Lukáš Karas, connman

Hi,

On Fri, Jun 18, 2021 at 08:53:25PM +0300, Santtu Lakkala wrote:
> On 18.6.2021 15.38, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > Thank you for your patch, but, even if I have not checked the
> > related sources, I really doubt about your fix. Maybe it will not
> > be a problem, due to the following check:
> > if ((uptr + ulen) > uncomp_end)
> >   			goto out;
> > 
> > > memcpy is equivalent to strncpy here as pointed by Santtu.
> > Please be careful, no more warning does not always imply that
> > the problem has been fixed. The warning has only disappear here
> > because you have replaced the call to strncpy by another one.
> > memcpy and  strncpy are not equivalent.
> > strncpy is dedicated to string processing, and enables to avoid not
> > null terminated strings, when the string length is equal to the destination
> > buffer size.
> > The good fix is just to replace, in the strncpy call,  ulen by  ulen -1 or
> > strlen(name).
> > If you really want to propose a patch, please refer to the commits
> > (sources code + commit message) I mentioned
> > (90f0e412569b327d8b25b834623b814f1fddbd19
> > & 3dbcc963a66d43e4ac51c97de4fe116ec72eb9f6),
> > it will be a better approach.
> 
> This is a different warning, this warning is about using strlen(src) as
> strncpy() length argument. I see a few possible fixes to acknowledge the
> warning:
>  1) replace strncpy with memcpy (as done in this patch, no change in
> behavior, only theoretically slightly more performant)
>  2) replace strncpy with strcpy (available space is already checked)
>  3) replace strncpy length argument with available buffer space (would
> unnecessarily zero the buffer to the end)
>  4) change the whole paradigm, and do something like:
>     uptr += snprintf(uptr, uncomp_end - uptr, "%s", name);
>     if (uptr >= uncomp_end)
>         goto out;
>  5) do 4 with strlcpy, but we don't have that

I've applied the patch. While option 4) is probably a bit nicer to read,
the rest of the function uses memcpy. So let's use memcpy for the first
operation as well at least for the time being.

If someone wants to rewrite the dnsproxy code to be more robust please
give it a go. We had at least 3 CVE in dnsproxy and I woulnd't be
surprised if there are more sever bugs hidden.

Thanks a lot!
Daniel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] replace strncopy by memcpy
@ 2021-06-18  9:27 Lukáš Karas
  0 siblings, 0 replies; 4+ messages in thread
From: Lukáš Karas @ 2021-06-18  9:27 UTC (permalink / raw)
  To: connman

[-- Attachment #1.1: Type: text/plain, Size: 390 bytes --]

This patch fixes build with GCC 10.3.0, that fails with warning/error:

  '__builtin_strncpy' specified bound depends on the length
  of the source argument [-Werror=stringop-overflow=]

Destination buffer size is is checked properly,
memcpy is equivalent to strncpy here as pointed by Santtu.

This patch replaces my previous one:
"dnsproxy: Suppress GCC warning stringop-overflow"

Lukas

[-- Attachment #1.2: 0001-replace-strncopy-by-memcpy.patch --]
[-- Type: text/x-patch, Size: 956 bytes --]

From 3dca8ec863d60a3bfd81baeb56255742c242ad93 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Karas?= <lukas.karas@centrum.cz>
Date: Fri, 18 Jun 2021 11:09:04 +0200
Subject: [PATCH] replace strncopy by memcpy

Destination buffer size is is checked properly,
but GCC 10.3.0 analysis fails to detect it and prints stringop-overflow warning.
Memcpy is equivalent to strncpy here, because ulen = strlen(name) + 1.
---
 src/dnsproxy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 38dbdd71..4b65b379 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1791,7 +1791,7 @@ static char *uncompress(int16_t field_count, char *start, char *end,
 		ulen = strlen(name) + 1;
 		if ((uptr + ulen) > uncomp_end)
 			goto out;
-		strncpy(uptr, name, ulen);
+		memcpy(uptr, name, ulen);
 
 		debug("pos %d ulen %d left %d name %s", pos, ulen,
 			(int)(uncomp_end - (uptr + ulen)), uptr);
-- 
2.27.0


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-18 12:38 [PATCH] replace strncopy by memcpy VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-06-18 17:53 ` Santtu Lakkala
2021-06-21  7:00   ` Daniel Wagner
  -- strict thread matches above, loose matches on Subject: below --
2021-06-18  9:27 Lukáš Karas

ConnMan network manager

Archives are clonable:
	git clone --mirror https://lore.kernel.org/connman/0 connman/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 connman connman/ https://lore.kernel.org/connman \
		connman@lists.linux.dev
	public-inbox-index connman

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/dev.linux.lists.connman


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git