[PATCH] Remove strcpy() from most Team-controlled code.
Jeremy Allison
jra at samba.org
Thu Mar 17 20:03:24 UTC 2016
On Thu, Mar 17, 2016 at 12:53:27PM -0700, Jeremy Allison wrote:
> Removes uses of strcpy() (even though these were
> safe) from most Team code. I'd like to excise all
> strcpy() instances even from test and example code
> as good security policy.
>
> Had to leave the use inside examples/validchars/validchr.c
> as that's a DOS-end-of-line CR/LF file and I couldn't
> figure out how to create a git-am patch that would
> cleanly apply to a checked out branch without causing
> a "inconsistent EOL" error in the patch process.
>
> There are still some in ctdb I haven't gotten to
> yet.
>
> Passes local make test.
>
> Please review and push if happy !
Bah. Sent the previous version of the patch (sorry).
One character change and this is the correct one
(doubt if anyone'll spot it, but I want this to
be correct :-).
Here's the correct patch !
Jeremy.
-------------- next part --------------
From 3878373af5b33b6f84835d3b6e768aec9bdeb7ae Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 16 Mar 2016 09:37:42 -0700
Subject: [PATCH 1/5] nsswitch: linux: Remove use of strcpy().
The previous use was safe, but having *any* use of strcpy inside
our code sets off security flags. Replace with an explicit length
calculation and memcpy.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
nsswitch/winbind_nss_linux.c | 44 ++++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/nsswitch/winbind_nss_linux.c b/nsswitch/winbind_nss_linux.c
index 2b31bac..b5c50ef 100644
--- a/nsswitch/winbind_nss_linux.c
+++ b/nsswitch/winbind_nss_linux.c
@@ -190,29 +190,33 @@ static NSS_STATUS fill_pwent(struct passwd *result,
struct winbindd_pw *pw,
char **buffer, size_t *buflen)
{
+ size_t len;
+
/* User name */
+ len = strlen(pw->pw_name) + 1;
if ((result->pw_name =
- get_static(buffer, buflen, strlen(pw->pw_name) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->pw_name, pw->pw_name);
+ memcpy(result->pw_name, pw->pw_name, len);
/* Password */
+ len = strlen(pw->pw_passwd) + 1;
if ((result->pw_passwd =
- get_static(buffer, buflen, strlen(pw->pw_passwd) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->pw_passwd, pw->pw_passwd);
+ memcpy(result->pw_passwd, pw->pw_passwd, len);
/* [ug]id */
@@ -220,40 +224,43 @@ static NSS_STATUS fill_pwent(struct passwd *result,
result->pw_gid = pw->pw_gid;
/* GECOS */
+ len = strlen(pw->pw_gecos) + 1;
if ((result->pw_gecos =
- get_static(buffer, buflen, strlen(pw->pw_gecos) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->pw_gecos, pw->pw_gecos);
+ memcpy(result->pw_gecos, pw->pw_gecos, len);
/* Home directory */
+ len = strlen(pw->pw_dir) + 1;
if ((result->pw_dir =
- get_static(buffer, buflen, strlen(pw->pw_dir) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->pw_dir, pw->pw_dir);
+ memcpy(result->pw_dir, pw->pw_dir, len);
/* Logon shell */
+ len = strlen(pw->pw_shell) + 1;
if ((result->pw_shell =
- get_static(buffer, buflen, strlen(pw->pw_shell) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->pw_shell, pw->pw_shell);
+ memcpy(result->pw_shell, pw->pw_shell, len);
/* The struct passwd for Solaris has some extra fields which must
be initialised or nscd crashes. */
@@ -279,29 +286,32 @@ static NSS_STATUS fill_grent(struct group *result, struct winbindd_gr *gr,
char *name;
int i;
char *tst;
+ size_t len;
/* Group name */
+ len = strlen(gr->gr_name) + 1;
if ((result->gr_name =
- get_static(buffer, buflen, strlen(gr->gr_name) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->gr_name, gr->gr_name);
+ memcpy(result->gr_name, gr->gr_name, len);
/* Password */
+ len = strlen(gr->gr_passwd) + 1;
if ((result->gr_passwd =
- get_static(buffer, buflen, strlen(gr->gr_passwd) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy(result->gr_passwd, gr->gr_passwd);
+ memcpy(result->gr_passwd, gr->gr_passwd, len);
/* gid */
@@ -342,13 +352,15 @@ static NSS_STATUS fill_grent(struct group *result, struct winbindd_gr *gr,
while(next_token_alloc((const char **)&gr_mem, &name, ",")) {
/* Allocate space for member */
+ len = strlen(name) + 1;
+
if (((result->gr_mem)[i] =
- get_static(buffer, buflen, strlen(name) + 1)) == NULL) {
+ get_static(buffer, buflen, len)) == NULL) {
free(name);
/* Out of memory */
return NSS_STATUS_TRYAGAIN;
}
- strcpy((result->gr_mem)[i], name);
+ memcpy((result->gr_mem)[i], name, len);
free(name);
i++;
}
--
2.7.0.rc3.207.g0ac5344
From 23321e8f434e4a28f36b4bc397bb316b818e2489 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 16 Mar 2016 15:09:12 -0700
Subject: [PATCH 2/5] examples: Remove all uses of strcpy in examples (except
for validchr.c).
I can't figure out how to make git handle the CR/LF differences
in this file.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
examples/libsmbclient/testacl.c | 3 ++-
examples/libsmbclient/testbrowse2.c | 12 ++++++++----
examples/libsmbclient/testsmbc.c | 2 +-
examples/libsmbclient/testtruncate.c | 2 +-
examples/libsmbclient/testwrite.c | 2 +-
5 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/examples/libsmbclient/testacl.c b/examples/libsmbclient/testacl.c
index 99a6d13..e3e06b3 100644
--- a/examples/libsmbclient/testacl.c
+++ b/examples/libsmbclient/testacl.c
@@ -139,7 +139,8 @@ int main(int argc, const char *argv[])
return 1;
}
- strcpy(path, poptGetArg(pc));
+ strncpy(path, poptGetArg(pc), sizeof(path));
+ path[sizeof(path)-1] = '\0';
if (smbc_init(get_auth_data_fn, debug) != 0)
{
diff --git a/examples/libsmbclient/testbrowse2.c b/examples/libsmbclient/testbrowse2.c
index 123660f..ac2063d 100644
--- a/examples/libsmbclient/testbrowse2.c
+++ b/examples/libsmbclient/testbrowse2.c
@@ -68,16 +68,18 @@ static smbitem* get_smbitem_list(SMBCCTX *ctx, char *smb_path){
if ((fd = smbc_getFunctionOpendir(ctx)(ctx, smb_path)) == NULL)
return NULL;
while((dirent = smbc_getFunctionReaddir(ctx)(ctx, fd)) != NULL){
+ size_t slen;
if (strcmp(dirent->name, "") == 0) continue;
if (strcmp(dirent->name, ".") == 0) continue;
if (strcmp(dirent->name, "..") == 0) continue;
- if ((item = malloc(sizeof(smbitem) + strlen(dirent->name))) == NULL)
+ slen = strlen(dirent->name)+1;
+ if ((item = malloc(sizeof(smbitem) + slen)) == NULL)
continue;
item->next = list;
item->type = dirent->smbc_type;
- strcpy(item->name, dirent->name);
+ memcpy(item->name, dirent->name, slen);
list = item;
}
smbc_getFunctionClose(ctx)(ctx, fd);
@@ -113,7 +115,8 @@ static void recurse(SMBCCTX *ctx, const char *smb_group, char *smb_path, int max
else print_smb_path(smb_group, list->name);
if (maxlen < 7 + strlen(list->name)) break;
- strcpy(smb_path + 6, list->name);
+ strncpy(smb_path + 6, list->name, maxlen - 6);
+ smb_path[maxlen-1] = '\0';
if ((ctx1 = create_smbctx()) != NULL){
recurse(ctx1, smb_group, smb_path, maxlen);
delete_smbctx(ctx1);
@@ -128,7 +131,8 @@ static void recurse(SMBCCTX *ctx, const char *smb_group, char *smb_path, int max
if (maxlen < len + strlen(list->name) + 2) break;
smb_path[len] = '/';
- strcpy(smb_path + len + 1, list->name);
+ strncpy(smb_path + len + 1, list->name, maxlen - len - 1);
+ smb_path[maxlen-1] = '\0';
print_smb_path(smb_group, smb_path + 6);
if (list->type != SMBC_FILE){
recurse(ctx, smb_group, smb_path, maxlen);
diff --git a/examples/libsmbclient/testsmbc.c b/examples/libsmbclient/testsmbc.c
index 1f98c3a..3c9aa56 100644
--- a/examples/libsmbclient/testsmbc.c
+++ b/examples/libsmbclient/testsmbc.c
@@ -115,7 +115,7 @@ int main(int argc, char *argv[])
/* Now, write some date to the file ... */
memset(buff, '\0', sizeof(buff));
- strcpy(buff, "Some test data for the moment ...");
+ snprintf(buff, sizeof(buff), "%s", "Some test data for the moment ...");
err = smbc_write(fd, buff, sizeof(buff));
diff --git a/examples/libsmbclient/testtruncate.c b/examples/libsmbclient/testtruncate.c
index 3e29ad2..1b4298d 100644
--- a/examples/libsmbclient/testtruncate.c
+++ b/examples/libsmbclient/testtruncate.c
@@ -32,7 +32,7 @@ int main(int argc, char * argv[])
return 1;
}
- strcpy(buffer, "Hello world.\nThis is a test.\n");
+ snprintf(buffer, sizeof(buffer), "%s", "Hello world.\nThis is a test.\n");
ret = smbc_write(fd, buffer, strlen(buffer));
savedErrno = errno;
diff --git a/examples/libsmbclient/testwrite.c b/examples/libsmbclient/testwrite.c
index b641a08..636cb20 100644
--- a/examples/libsmbclient/testwrite.c
+++ b/examples/libsmbclient/testwrite.c
@@ -47,7 +47,7 @@ int main(int argc, char * argv[])
continue;
}
- strcpy(buffer, "Hello world\n");
+ snprintf(buffer, sizeof(buffer), "%s", "Hello world\n");
ret = smbc_write(fd, buffer, strlen(buffer));
savedErrno = errno;
--
2.7.0.rc3.207.g0ac5344
From 5cd412d3d1b2a578b2ae34c6f957f6f112274431 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 16 Mar 2016 13:55:31 -0700
Subject: [PATCH 3/5] lib:tdb: Remove use of strcpy in tdb test.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/tdb/test/external-agent.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/tdb/test/external-agent.c b/lib/tdb/test/external-agent.c
index 443d382..3c59c06 100644
--- a/lib/tdb/test/external-agent.c
+++ b/lib/tdb/test/external-agent.c
@@ -184,7 +184,8 @@ enum agent_return external_agent_operation(struct agent *agent,
string = malloc(len);
string[0] = op;
- strcpy(string+1, name);
+ strncpy(string+1, name, len - 1);
+ string[len-1] = '\0';
if (write(agent->cmdfd, string, len) != len
|| read(agent->responsefd, &res, sizeof(res)) != sizeof(res))
--
2.7.0.rc3.207.g0ac5344
From b14f9a2f13004fd32cec3e4b4ced4da57e42de14 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 16 Mar 2016 14:04:34 -0700
Subject: [PATCH 4/5] nsswitch: winbind_nss_aix: Remove all uses of strcpy.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
nsswitch/winbind_nss_aix.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/nsswitch/winbind_nss_aix.c b/nsswitch/winbind_nss_aix.c
index c5c223f..dc44db4 100644
--- a/nsswitch/winbind_nss_aix.c
+++ b/nsswitch/winbind_nss_aix.c
@@ -85,13 +85,15 @@ static void logit(const char *format, ...)
#define STRCPY_RET(dest, src) \
do { \
if (strlen(src)+1 > sizeof(dest)) { errno = EINVAL; return -1; } \
- strcpy(dest, src); \
+ strncpy(dest, src, sizeof(dest)); \
+ dest[sizeof(dest)-1] = '\0'; \
} while (0)
#define STRCPY_RETNULL(dest, src) \
do { \
if (strlen(src)+1 > sizeof(dest)) { errno = EINVAL; return NULL; } \
- strcpy(dest, src); \
+ strncpy(dest, src, sizeof(dest)); \
+ dest[sizeof(dest)-1] = '\0'; \
} while (0)
@@ -578,18 +580,21 @@ static attrval_t pwd_to_groupsids(struct passwd *pwd)
{
attrval_t r;
char *s, *p;
+ size_t mlen;
if ( (s = wb_aix_getgrset(pwd->pw_name)) == NULL ) {
r.attr_flag = EINVAL;
return r;
}
- if ( (p = malloc(strlen(s)+2)) == NULL ) {
+ mlen = strlen(s)+2;
+ if ( (p = malloc(mlen)) == NULL ) {
r.attr_flag = ENOMEM;
return r;
}
- strcpy(p, s);
+ strncpy(p, s, mlen);
+ p[mlen-1] = '\0';
replace_commas(p);
free(s);
@@ -855,7 +860,8 @@ static int wb_aix_normalize(char *longname, char *shortname)
/* automatically cope with AIX 5.3 with longer usernames
when it comes out */
if (S_NAMELEN > strlen(longname)) {
- strcpy(shortname, longname);
+ strncpy(shortname, longname, S_NAMELEN);
+ shortname[S_NAMELEN-1] = '\0';
return 1;
}
--
2.7.0.rc3.207.g0ac5344
From 8bfb665c890a246682d79a5a056105ca2b042cf9 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 16 Mar 2016 14:19:57 -0700
Subject: [PATCH 5/5] nsswitch: winbind_nss_solaris.c: Remove unused macro
containing strcpy.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
nsswitch/winbind_nss_solaris.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/nsswitch/winbind_nss_solaris.c b/nsswitch/winbind_nss_solaris.c
index dfb87e3..eb1ddb0 100644
--- a/nsswitch/winbind_nss_solaris.c
+++ b/nsswitch/winbind_nss_solaris.c
@@ -77,17 +77,6 @@ struct nss_groupsbymem {
#endif /* HPUX */
-#define make_pwent_str(dest, src) \
-{ \
- if((dest = get_static(buffer, buflen, strlen(src)+1)) == NULL) \
- { \
- *errnop = ERANGE; \
- NSS_DEBUG("ERANGE error"); \
- return NSS_STATUS_TRYAGAIN; \
- } \
- strcpy(dest, src); \
-}
-
static NSS_STATUS _nss_winbind_setpwent_solwrap (nss_backend_t* be, void* args)
{
NSS_DEBUG("_nss_winbind_setpwent_solwrap");
--
2.7.0.rc3.207.g0ac5344
More information about the samba-technical
mailing list