Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
Hi FRIGN, we had this strlcpy pseudo-discussion years ago. And we concluded to avoid adopting strlcpy. The basic reason is, that the claim of strlcpy to be more secure than strncpy is a myth. Roberto has pointed this out already. In either case you should handle the retval for arbitrary source inputs, _unless_ you are knowing what you are doing. In case of dwm, we don't process arbitrary input apart from window titles. And for those we exactly know what we do. Client structures are always zero-allocated and c->name[sizeof c->name - 1] is never touched and hence the last resort string terminator. For all other uses of strcpy or strncpy the buffer sizes will suffice unless you don't purposefully exceed the VERSION macro at compile time to be longer than 251 chars -- and here it wouldn't be about arbitrary input anyways and detected at compile time (if the compiler is smart enough). Also bare in mind, that there is a significant usage difference between strlcpy and strncpy. strlcpy always requires that the size argument contains space for the trailing 0, whereas the size argument of strncpy does not require this. Our code is written to work well with a non-null-terminated strncpy operation (last resort terminator), so there is really no need to introduce strlcpy for pseudo security reasons. I think the same should apply for most suckless stuff as well. BR, Anselm
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
On Mon, Jun 06, 2016 at 08:50:31PM +0200, FRIGN wrote: > On Mon, 6 Jun 2016 20:12:19 +0200 > k...@shike2.com wrote: > >> ... > > Nope, it's not. Keep in mind strlcpy fills the rest > of the memory area with 0's. No it doesn't, but strncpy does that. > Also, I suspect you have not understood strlcpy() > at all. Read the manpage! Roberto wrote compilers before you were a baby. > The size-argument of strlcpy is wrt to the size of > the _destination_, not the _source_. > Your example also hides the fact that "nsrc" has to > be first determined with strlen(). > It depends, strlcpy copies the data regardless if truncation occurs, so it is slower in some cases. It often does not matter though. > > From my point of view the worst thing is that people believe > > that using strlcpy the code magically becomes secure, and this > > is a totally false security sensation. You have to check the > > return code, and it means that the code is totally equivalent > > to an explicit if. Look for example this case: > > > > deluser(strlcpy(dst, "user15", 4)); > > Truncation needs to be checked regardless if strlcpy is used or not, imo the pattern of using strlcpy is nice in alot (but not all) cases. It is usually: if (strlcpy(buf, s, sizeof(buf)) >= sizeof(buf)) ...; > You cannot possibly check these things by yourself. What > strlcpy does is give you an insurance in case there is an > overflow that the program doesn't pass pointers to > unterminated strings around. Of course you have to check > the return value anyway. > I don't know why you are always riding the same horse > here given that: > 1) dwm used strcpy (!) > 2) dwm used _unchecked_ strncpy (!) > so you definitely should calm down a bit here. > Dwm is not insecure in these cases, it is more a style issue. Some of the strncpy cases can be replaced even with strcpy, it can't overflow unless the user screws up his config. -- Kind regards, Hiltjo
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
On Mon, 6 Jun 2016 20:12:19 +0200
k...@shike2.com wrote:
Hey Roberto,
> What happens if you pass an incorrect size to strlcpy?. Please,
> stop of saying stupid things.
well, you don't pass invalid sizes to strlcpy. It's like if I
asked: Robert, what happens if I pass an invalid path to
unlink()? Huh, see, your world is crumbling into pieces.
> if (strlcpy(dst, src, nsrc) >= nsrc)
> error();
>
> is equal to:
>
> if (nsrc >= ndst)
> error();
> memcpy(dst, src, nsrc);
Nope, it's not. Keep in mind strlcpy fills the rest
of the memory area with 0's.
Also, I suspect you have not understood strlcpy()
at all. Read the manpage!
The size-argument of strlcpy is wrt to the size of
the _destination_, not the _source_.
Your example also hides the fact that "nsrc" has to
be first determined with strlen().
> but the code with strlcpy is slower and not portable.
I give you a beer at the next slcon if you can show me
a solution which does the same as strlcpy and is faster.
> There is a reason why after 16 years strlcpy is not in any
> standard, no C11, no POSIX, and it is because it sucks a lot.
Well, this is up for debate. I respect your opinion though.
> From my point of view the worst thing is that people believe
> that using strlcpy the code magically becomes secure, and this
> is a totally false security sensation. You have to check the
> return code, and it means that the code is totally equivalent
> to an explicit if. Look for example this case:
>
> deluser(strlcpy(dst, "user15", 4));
>
> Since you are not checking any return code there you are not
> deleting the correct user, and this kind of attacks can be very
> easy of attack, more easier than stack overflow.
Yes, this is a very good point. However, if you used strncpy,
the string would not even be terminated in case of an overflow.
It's funny to see how people wage holy war between strlcpy and
strncpy only to realize that of the two strlcpy is the safer
one.
Now, of course, you have to check the return value.
> In a previous mail you said that one of the reasons of using
> strlcpy was to avoid problems in the future due to modifications
> in the code. Did you think about it before writing it?. You
> can say that of _ANY_ operation in C, mainly with pointers and
> indexes, but strlcpy can not help at all in a situation like this:
>
>
> #define LENA 5
> #define LENB 6
>
> char sa[LENB];
>
> f(sa);
>
> f(char s[LENB])
> {
> strlcpy(s, "This is a very long string", LENB);
> }
>
> and now you have this patch:
>
> - char sa[LENB];
> + char sa[LENA];
>
> Do you see? strlcpy didn't help at all, and due to the
> false security sensation the programmer didn't dig to
> see all the side effects of changing the size of sa.
> C is a very low level language, and it is a language
> without support for strings, and the only way of writting
> correct code is to be very carefull and before doing any
> change check everything, and look for all the possible
> errors due to your change.
You cannot possibly check these things by yourself. What
strlcpy does is give you an insurance in case there is an
overflow that the program doesn't pass pointers to
unterminated strings around. Of course you have to check
the return value anyway.
I don't know why you are always riding the same horse
here given that:
1) dwm used strcpy (!)
2) dwm used _unchecked_ strncpy (!)
so you definitely should calm down a bit here.
> And of course, strlcpy is also totally useless because
> you can do the same work with snprintf.
I'm not sure if people want to involve the complex
mechanics of the *printf functions to do simple string
copying. You have to agree that on a decision between
strncpy and strlcpy, strncpy wins, no matter the context.
Of course, to use strlcpy safely, you have to check the
return code, because truncation is also yielding garbage.
However, at least this garbage won't blow up in your
face.
> PD: I don't want to begin a flame war, but please, stop
> of being a fan boy and think for yourself, try to find
> the strong points and what is propaganda.
Maybe you should evaluate your position a bit better
before going all grandpa here on this ml.
Your first code example is embarassing and shows that
despite your high skill in C you seem to be fighting
the wrong war here.
Cheers
FRIGN
--
FRIGN
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
> The only reason why strlcpy is "non-portable" is because the > Posix-committee has sticks up their asses (same with the > glibc guys). Why is so difficult to accept that your idea is so bad?. Why people like so much think in obscure conspiracions instead of accepting the true? Regards,
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
> there is also another point here: strlcpy is safer than strcpy
> and strncpy because _if_ there is an overflow the string will
What happens if you pass an incorrect size to strlcpy?. Please,
stop of saying stupid things.
if (strlcpy(dst, src, nsrc) >= nsrc)
error();
is equal to:
if (nsrc >= ndst)
error();
memcpy(dst, src, nsrc);
but the code with strlcpy is slower and not portable.
There is a reason why after 16 years strlcpy is not in any
standard, no C11, no POSIX, and it is because it sucks a lot.
>From my point of view the worst thing is that people believe
that using strlcpy the code magically becomes secure, and this
is a totally false security sensation. You have to check the
return code, and it means that the code is totally equivalent
to an explicit if. Look for example this case:
deluser(strlcpy(dst, "user15", 4));
Since you are not checking any return code there you are not
deleting the correct user, and this kind of attacks can be very
easy of attack, more easier than stack overflow.
In a previous mail you said that one of the reasons of using
strlcpy was to avoid problems in the future due to modifications
in the code. Did you think about it before writing it?. You
can say that of _ANY_ operation in C, mainly with pointers and
indexes, but strlcpy can not help at all in a situation like this:
#define LENA 5
#define LENB 6
char sa[LENB];
f(sa);
f(char s[LENB])
{
strlcpy(s, "This is a very long string", LENB);
}
and now you have this patch:
- char sa[LENB];
+ char sa[LENA];
Do you see? strlcpy didn't help at all, and due to the
false security sensation the programmer didn't dig to
see all the side effects of changing the size of sa.
C is a very low level language, and it is a language
without support for strings, and the only way of writting
correct code is to be very carefull and before doing any
change check everything, and look for all the possible
errors due to your change.
And of course, strlcpy is also totally useless because
you can do the same work with snprintf.
Regards,
PD: I don't want to begin a flame war, but please, stop
of being a fan boy and think for yourself, try to find
the strong points and what is propaganda.
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
On Fri, 3 Jun 2016 17:58:12 +0200 k...@shike2.com wrote: Hey Roberto, > safe?. You are not checking any of the return codes. You are only > moving the problem from some place to another place. Please add > checks and stop using non portable functions. I don't want your > shit, thanks. there is also another point here: strlcpy is safer than strcpy and strncpy because _if_ there is an overflow the string will be 0-terminated. I'm not sure if there even should be an error-out in case for instance we overflow writing the "broken"-state-string to a client-name. Cheers FRIGN -- FRIGN
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
On Fri, 3 Jun 2016 17:58:12 +0200 k...@shike2.com wrote: Hey k0ga, > safe?. You are not checking any of the return codes. You are only > moving the problem from some place to another place. Please add > checks and stop using non portable functions. I don't want your > shit, thanks. I just saw that I forgot to amend this part of the patch to this one here. Will push it later, like the following: if (strlcpy(..., size) >= size) ... The only reason why strlcpy is "non-portable" is because the Posix-committee has sticks up their asses (same with the glibc guys). Cheers FRIGN -- FRIGN
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
On Thu, Jun 02, 2016 at 09:57:01PM +0200, FRIGN wrote: > Hello fellow hackers, > > I'll drop this little patch here so we finally make the switch to the > safe OpenBSD-functions for string copying. A good compromise might be using snprintf(buf, sizeof(buf), "%s", text) this is standard and functionally exactly the same as using strlcpy. I agree the strncpy functions need to go, they don't make sense in this context. Some people forget strncpy has the following properties aswell, from the OpenBSD man page: "If src is less than len characters long, it fills the remaining buffer with '\0' characters". and "strncpy() only NUL terminates the destination string when the length of the source string is less than the length parameter." -- Kind regards, Hiltjo
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
>> I'll drop this little patch here so we finally make the switch to the safe safe?. You are not checking any of the return codes. You are only moving the problem from some place to another place. Please add checks and stop using non portable functions. I don't want your shit, thanks. Regards,
Re: [hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
FRIGN wrote: > I'll drop this little patch here so we finally make the switch to the safe > OpenBSD-functions for string copying. Heyho, please don't forget the drw unification patches from two weeks ago, when merging this. Hiltjo seems to be busy right now and I don't know if anyone else but Hiltjo and Anselm has authority for the dwm repo. --Markus
[hackers] [dwm] [PATCH] Replace str[n]cpy with strlcpy
Hello fellow hackers,
I'll drop this little patch here so we finally make the switch to the
safe OpenBSD-functions for string copying.
Read the patch description for more info.
Cheers
FRIGN
--
FRIGN
>From 849a7cbee0310beb7ea51986bf98aff8d3b7ff26 Mon Sep 17 00:00:00 2001
From: FRIGN
Date: Thu, 2 Jun 2016 21:46:50 +0200
Subject: [PATCH] Replace str[n]cpy with strlcpy
Let's finally use this safe interface here! We've waited long
enough.
Even if a call to strcpy in some cases might be safe
(e.g. writing the broken string to c->name), we can never
assure that there might not be a code change in the future
that breaks this assumption.
Even though you might have had these side-effects in mind the
time you wrote the code, they definitely won't be a few days/
months/years later when the changes are made.
---
dwm.c | 16 +---
util.c | 40
util.h | 3 +++
3 files changed, 52 insertions(+), 7 deletions(-)
diff --git a/dwm.c b/dwm.c
index ff7e096..d45916c 100644
--- a/dwm.c
+++ b/dwm.c
@@ -393,7 +393,7 @@ arrange(Monitor *m)
void
arrangemon(Monitor *m)
{
- strncpy(m->ltsymbol, m->lt[m->sellt]->symbol, sizeof m->ltsymbol);
+ strlcpy(m->ltsymbol, m->lt[m->sellt]->symbol, sizeof(m->ltsymbol));
if (m->lt[m->sellt]->arrange)
m->lt[m->sellt]->arrange(m);
}
@@ -654,7 +654,8 @@ createmon(void)
m->topbar = topbar;
m->lt[0] = &layouts[0];
m->lt[1] = &layouts[1 % LENGTH(layouts)];
- strncpy(m->ltsymbol, layouts[0].symbol, sizeof m->ltsymbol);
+ strlcpy(m->ltsymbol, layouts[0].symbol, sizeof(m->ltsymbol));
+
return m;
}
@@ -931,10 +932,10 @@ gettextprop(Window w, Atom atom, char *text, unsigned int size)
if (!name.nitems)
return 0;
if (name.encoding == XA_STRING)
- strncpy(text, (char *)name.value, size - 1);
+ strlcpy(text, (char *)name.value, size - 1);
else {
if (XmbTextPropertyToTextList(dpy, &name, &list, &n) >= Success && n > 0 && *list) {
- strncpy(text, *list, size - 1);
+ strlcpy(text, *list, size - 1);
XFreeStringList(list);
}
}
@@ -1526,7 +1527,8 @@ setlayout(const Arg *arg)
selmon->sellt ^= 1;
if (arg && arg->v)
selmon->lt[selmon->sellt] = (Layout *)arg->v;
- strncpy(selmon->ltsymbol, selmon->lt[selmon->sellt]->symbol, sizeof selmon->ltsymbol);
+ strlcpy(selmon->ltsymbol, selmon->lt[selmon->sellt]->symbol,
+ sizeof(selmon->ltsymbol));
if (selmon->sel)
arrange(selmon);
else
@@ -1991,14 +1993,14 @@ updatetitle(Client *c)
if (!gettextprop(c->win, netatom[NetWMName], c->name, sizeof c->name))
gettextprop(c->win, XA_WM_NAME, c->name, sizeof c->name);
if (c->name[0] == '\0') /* hack to mark broken clients */
- strcpy(c->name, broken);
+ strlcpy(c->name, broken, sizeof(c->name));
}
void
updatestatus(void)
{
if (!gettextprop(root, XA_WM_NAME, stext, sizeof(stext)))
- strcpy(stext, "dwm-"VERSION);
+ strlcpy(stext, "dwm-"VERSION, sizeof(stext));
drawbar(selmon);
}
diff --git a/util.c b/util.c
index 6b703e9..ac4372f 100644
--- a/util.c
+++ b/util.c
@@ -31,3 +31,43 @@ die(const char *fmt, ...) {
exit(1);
}
+
+/*
+ * Copyright (c) 1998, 2015 Todd C. Miller
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+size_t
+strlcpy(char *dst, const char *src, size_t dsize)
+{
+ const char *osrc = src;
+ size_t nleft = dsize;
+
+ /* Copy as many bytes as will fit. */
+ if (nleft != 0) {
+ while (--nleft != 0) {
+ if ((*dst++ = *src++) == '\0')
+break;
+ }
+ }
+
+ /* Not enough room in dst, add NUL and traverse rest of src. */
+ if (nleft == 0) {
+ if (dsize != 0)
+ *dst = '\0'; /* NUL-terminate dst */
+ while (*src++)
+ ;
+ }
+
+ return(src - osrc - 1); /* count does not include NUL */
+}
diff --git a/util.h b/util.h
index cded043..ce29b77 100644
--- a/util.h
+++ b/util.h
@@ -1,4 +1,5 @@
/* See LICENSE file for copyright and license details. */
+#include
#define MAX(A, B) ((A) > (B) ? (A) : (B))
#define MIN(A, B) ((A) < (B) ? (A) : (B))
@@ -6,3 +7,5 @@
void die(const char *errstr, ...);
void *ecalloc(size_t, size_t);
+#undef strlcpy
+size_t strlcpy(char *, const char *, size_t);
--
2.4.10
