[nfs4-acl-tools PATCH 0/3] nfs4-acl-tools hardening fixes

[nfs4-acl-tools PATCH 0/3] nfs4-acl-tools hardening fixes

[Roberto Bergantinos Corpas]

- Escape special characters in printed filenames to prevent injection
- Fix bound checking around NFS4_MAX_PRINCIPALSIZE
- Use lsetxattr to avoid following symlinks during recursive walks

Roberto Bergantinos Corpas (3):
  nfs4_getfacl/nfs4_setfacl: escape newline+cr from filenames
  libnfs4acl: fix various bound checkings against NFS4_MAX_PRINCIPALSIZE
  nfs4_setacl: do not follow symlinks on
    do_apply_action/nfs4_setacl_byname

 include/libacl_nfs4.h             |  1 +
 libnfs4acl/acl_nfs4_set_who.c     |  2 +-
 libnfs4acl/acl_nfs4_xattr_load.c  |  2 +-
 libnfs4acl/nfs4_ace_from_string.c |  2 +-
 libnfs4acl/nfs4_print_acl.c       | 13 +++++++++++++
 libnfs4acl/nfs4_setacl.c          |  2 +-
 nfs4_getfacl/nfs4_getfacl.c       |  7 +++++--
 nfs4_setfacl/nfs4_setfacl.c       |  6 ++++--
 8 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.55.0

[nfs4-acl-tools PATCH 1/3] nfs4_getfacl/nfs4_setfacl: escape newline+cr from filenames

[Roberto Bergantinos Corpas]

Filenames can be crafted to contain these special chars, as a result
they can be interpreted as ACE payload on the file that was not
explicitely set. Escape these chars from the path/filename.

Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
 include/libacl_nfs4.h       |  1 +
 libnfs4acl/nfs4_print_acl.c | 13 +++++++++++++
 nfs4_getfacl/nfs4_getfacl.c |  7 +++++--
 nfs4_setfacl/nfs4_setfacl.c |  6 ++++--
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/libacl_nfs4.h b/include/libacl_nfs4.h
index a486390..76c7e5e 100644
--- a/include/libacl_nfs4.h
+++ b/include/libacl_nfs4.h
@@ -217,6 +217,7 @@ extern int			nfs4_print_ace_verbose(struct nfs4_ace * ace, u32 isdir);
 extern char*			nfs4_get_ace_type(struct nfs4_ace*, char*, int);
 extern char*			nfs4_get_ace_flags(struct nfs4_ace*, char*);
 extern char*			nfs4_get_ace_access(struct nfs4_ace*, char*, int);
+extern void			nfs4_print_escaped_path(FILE *fp, const char *path);
 
 
 /** misc **/
diff --git a/libnfs4acl/nfs4_print_acl.c b/libnfs4acl/nfs4_print_acl.c
index 61f4d95..5afd682 100644
--- a/libnfs4acl/nfs4_print_acl.c
+++ b/libnfs4acl/nfs4_print_acl.c
@@ -83,3 +83,16 @@ unexp_failed:
 	fprintf(stderr, "An unexpected failure has occurred. \n");
 	return;
 }
+
+void nfs4_print_escaped_path(FILE *fp, const char *path)
+{
+	const char *s;
+	for (s = path; *s; s++) {
+		if (*s == '\n')
+			fputs("\\n", fp);
+		else if (*s == '\r')
+			fputs("\\r", fp);
+		else
+			fputc(*s, fp);
+	}
+}
diff --git a/nfs4_getfacl/nfs4_getfacl.c b/nfs4_getfacl/nfs4_getfacl.c
index ddb3005..be4652c 100644
--- a/nfs4_getfacl/nfs4_getfacl.c
+++ b/nfs4_getfacl/nfs4_getfacl.c
@@ -163,8 +163,11 @@ static void print_acl_from_path(const char *fpath, enum acl_type type)
 	}
 
 	if (acl != NULL) {
-		if (ignore_comment == 0)
-			printf("# file: %s\n", fpath);
+		if (ignore_comment == 0) {
+			fputs("# file: ", stdout);
+			nfs4_print_escaped_path(stdout, fpath);
+			fputc('\n', stdout);
+		}
 		nfs4_print_acl(stdout, acl);
 		printf("\n");
 		nfs4_free_acl(acl);
diff --git a/nfs4_setfacl/nfs4_setfacl.c b/nfs4_setfacl/nfs4_setfacl.c
index d10e073..ab4a3d8 100644
--- a/nfs4_setfacl/nfs4_setfacl.c
+++ b/nfs4_setfacl/nfs4_setfacl.c
@@ -545,9 +545,11 @@ static struct nfs4_acl* edit_ACL(struct nfs4_acl *acl, const char *path, const s
 	}
 
 	if (stat->st_mode & S_IFDIR)
-		fprintf(tmp_fp, "## Editing NFSv4 ACL for directory: %s\n", path);
+		fputs("## Editing NFSv4 ACL for directory: ", tmp_fp);
 	else
-		fprintf(tmp_fp, "## Editing NFSv4 ACL for file: %s\n", path);
+		fputs("## Editing NFSv4 ACL for file: ", tmp_fp);
+	nfs4_print_escaped_path(tmp_fp, path);
+	fputc('\n', tmp_fp);
 	nfs4_print_acl(tmp_fp, acl);
 	rewind(tmp_fp);
 
-- 
2.55.0

[nfs4-acl-tools PATCH 2/3] libnfs4acl: fix various bound checkings against NFS4_MAX_PRINCIPALSIZE

[Roberto Bergantinos Corpas]

We have some missing bound checkings against NFS4_MAX_PRINCIPALSIZE
around wholen/who field from payload that may cause oob or off-by-one
read/writes. Add/fix these bound checks.

Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
 libnfs4acl/acl_nfs4_set_who.c     | 2 +-
 libnfs4acl/acl_nfs4_xattr_load.c  | 2 +-
 libnfs4acl/nfs4_ace_from_string.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libnfs4acl/acl_nfs4_set_who.c b/libnfs4acl/acl_nfs4_set_who.c
index 07b73de..53ec343 100644
--- a/libnfs4acl/acl_nfs4_set_who.c
+++ b/libnfs4acl/acl_nfs4_set_who.c
@@ -72,7 +72,7 @@ int acl_nfs4_set_who(struct nfs4_ace* ace, int type, char* who)
 	}
 
 	wholen = strlen(iwho);
-	if (wholen < 1)
+	if (wholen < 1 || wholen >= NFS4_MAX_PRINCIPALSIZE)
 		goto inval_failed;
 
 	memset(ace->who, '\0', NFS4_MAX_PRINCIPALSIZE);
diff --git a/libnfs4acl/acl_nfs4_xattr_load.c b/libnfs4acl/acl_nfs4_xattr_load.c
index c747e8d..638b9e2 100644
--- a/libnfs4acl/acl_nfs4_xattr_load.c
+++ b/libnfs4acl/acl_nfs4_xattr_load.c
@@ -141,7 +141,7 @@ struct nfs4_acl *acl_nfs41_xattr_load(char *xattr_v, int xattr_size, u32 is_dir,
 		bufs -= d_ptr;
 
 		/* Get the who string */
-		if (bufs <= 0) {
+		if (bufs <= 0 || wholen > (u32)bufs || wholen >= NFS4_MAX_PRINCIPALSIZE) {
 			errno = EINVAL;
 			goto err1;
 		}
diff --git a/libnfs4acl/nfs4_ace_from_string.c b/libnfs4acl/nfs4_ace_from_string.c
index 7f13154..8e0e887 100644
--- a/libnfs4acl/nfs4_ace_from_string.c
+++ b/libnfs4acl/nfs4_ace_from_string.c
@@ -158,7 +158,7 @@ struct nfs4_ace * nfs4_ace_from_string(char *ace_buf, int is_dir)
 	if (ret < 0) {
 		fprintf(stderr,"Scanning ACE string '%s' failed.\n", ace_buf);
 		goto out;
-	} else if (strlen(fields[WHO_INDEX]) > NFS4_MAX_PRINCIPALSIZE) {
+	} else if (strlen(fields[WHO_INDEX]) >= NFS4_MAX_PRINCIPALSIZE) {
 		fprintf(stderr,"Principal \'%s\' is too large.\n",fields[WHO_INDEX]);
 		goto out_free;
 	}
-- 
2.55.0

[nfs4-acl-tools PATCH 3/3] nfs4_setacl: do not follow symlinks on do_apply_action/nfs4_setacl_byname

[Roberto Bergantinos Corpas]

We use nftw+FTW_PHYS on nfs4_setacl to avoid following symlinks, however
during do_apply_action/nfs4_setacl_byname we use setxattr that may
follow symlinks. As a consequence, there is a race window where if a
file turned into a symlink we may end up applying, during recursive walks,
an ACL to an unintended part of the tree.

Use lsetxattr instead of setxattr on nfs4_setacl_byname to avoid this
chance.

Signed-off-by: Roberto Bergantinos Corpas <rbergant@redhat.com>
---
 libnfs4acl/nfs4_setacl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libnfs4acl/nfs4_setacl.c b/libnfs4acl/nfs4_setacl.c
index d684502..8a6cae2 100644
--- a/libnfs4acl/nfs4_setacl.c
+++ b/libnfs4acl/nfs4_setacl.c
@@ -30,7 +30,7 @@ static int nfs4_setacl_byname(const char *path, const char *xattr_name,
 
 	ret = acl_nfs41_xattr_pack(acl, &xdrbuf, type);
 	if (ret != -1)
-		ret = setxattr(path, xattr_name, xdrbuf, ret, XATTR_REPLACE);
+		ret = lsetxattr(path, xattr_name, xdrbuf, ret, XATTR_REPLACE);
 	free(xdrbuf);
 	return ret;
 }
-- 
2.55.0