[PATCH] Remove strcpy() from most Team-controlled code.
Jeremy Allison
jra at samba.org
Thu Mar 17 19:53:27 UTC 2016
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 !
Cheers,
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)-1);
+ 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