Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBSD: Teach libarchive about the system extended attribute namespace #1409

Merged
merged 4 commits into from Oct 14, 2020

Conversation

lattera
Copy link
Contributor

@lattera lattera commented Jul 3, 2020

In order to teach HardenedBSD's packaging infrastructure how to support
HardenedBSD's method of exploit mitigation toggling, teach libarchive
how to handle the system filesystem extended attribute namespace.

Signed-off-by: Shawn Webb shawn.webb@hardenedbsd.org

@lattera
Copy link
Contributor Author

@lattera lattera commented Jul 19, 2020

Gentle ping. :)

@lattera
Copy link
Contributor Author

@lattera lattera commented Jul 26, 2020

Friendly ping. :)

@lattera
Copy link
Contributor Author

@lattera lattera commented Aug 2, 2020

Hopeful ping.

@mmatuska mmatuska self-requested a review Aug 9, 2020
Copy link
Member

@mmatuska mmatuska left a comment

Please fix or investigate failing tests (see my comments in the review).

if (list_size == -1 && errno == EPERM)
return (ARCHIVE_OK);
Copy link
Member

@mmatuska mmatuska Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bsdtar's test_option_acls changes behavior, please investigate and fix test and/or behavior

Copy link
Contributor Author

@lattera lattera Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing the system filesystem extended namespace is a privileged operation. The reason for this conditional is to ignore permission denied when an unprivileged user attempts to access the system namespace.

Copy link
Member

@mmatuska mmatuska Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just writing this to check why does one of the bsdtar tests behave differently (fail) after this change on FreeBSD.

@mmatuska
Copy link
Member

@mmatuska mmatuska commented Sep 10, 2020

@lattera are you still interested in merging this? It is really just this ACL test to fix :)

@lattera
Copy link
Contributor Author

@lattera lattera commented Sep 11, 2020

Sorry for the lack of response. Been a bit busy with ${LIFE}. I'll take a look soon.

@lattera
Copy link
Contributor Author

@lattera lattera commented Sep 13, 2020

I figured it out: https://git-01.md.hardenedbsd.org/HardenedBSD/HardenedBSD/commit/b45af9e6f2fe248fe1ecc2f87bf1f626c4dc8c44

I'll pull that commit into my libarchive fork and rebase this pull request.

@lattera
Copy link
Contributor Author

@lattera lattera commented Sep 18, 2020

Still failing FreeBSD tests. I'll have to dig into them later.

@lattera
Copy link
Contributor Author

@lattera lattera commented Sep 18, 2020

I'm unable to reproduce the test failure on my HardenedBSD system.

@mmatuska
Copy link
Member

@mmatuska mmatuska commented Oct 14, 2020

I have started digging into this - to reproduce the problem you need to run the test on acl-enabled UFS drive
E.g. TMPDIR=/mnt/tmp ./bsdtar_test -vvv test_option_acls

@mmatuska
Copy link
Member

@mmatuska mmatuska commented Oct 14, 2020

Ok, I guess I found the problem. You have to skip storing the following system attributes:
posix1e.acl_default
posix1e.acl_access
nfs4.acl

We deal with these in the ACL code and it messes up operation on UFS.

@lattera
Copy link
Contributor Author

@lattera lattera commented Oct 14, 2020

This pull request doesn't have anything to do with ACLs.

@mmatuska
Copy link
Member

@mmatuska mmatuska commented Oct 14, 2020

This pull request doesn't have anything to do with ACLs.

Actually, it does. Before your patch, system extended attributes were not stored when creating a tar file. After your patch, they are stored. On the UFS file system, ACLs are stored in extended attributes posix1e.acl_default, posix1e.acl_access, nfs4.acl. When you create a tar that includes a file on an UFS system that has ACLs, the resulting extended attributes get stored with your patch. So a) we don't want to store the information twice, b) on extract the extended attributes (try) to overwrite the acls or vice versa and that is a no-go.

@mmatuska
Copy link
Member

@mmatuska mmatuska commented Oct 14, 2020

What about adding this and we are fine? :)

diff --git a/libarchive/archive_read_disk_entry_from_file.c b/libarchive/archive_read_disk_entry_from_file.c
index 7395ee72..279e8885 100644
--- a/libarchive/archive_read_disk_entry_from_file.c
+++ b/libarchive/archive_read_disk_entry_from_file.c
@@ -765,11 +765,16 @@ setup_xattrs_namespace(struct archive_read_disk *a,
 		size_t len = 255 & (int)*p;
 		char *name;
 
-		switch (namespace) {
-		case EXTATTR_NAMESPACE_SYSTEM:
+		/* FreeBSD: skip NFSv4 and POSIX.1e ACL extended attributes */
+		if (namespace == EXTATTR_NAMESPACE_SYSTEM) {
+			if (strcmp(p + 1, "nfs4.acl") ||
+			    strcmp(p + 1, "posix1e.acl_access") ||
+			    strcmp(p + 1, "posix1e.acl_default")) {
+				p += 1 + len;
+				continue;
+			}
 			strcpy(buff, "system.");
-			break;
-		default:
+		} else {
 			strcpy(buff, "user.");
 		}
 		name = buff + strlen(buff);
diff --git a/libarchive/archive_write_disk_posix.c b/libarchive/archive_write_disk_posix.c
index e522492f..04d85cdb 100644
--- a/libarchive/archive_write_disk_posix.c
+++ b/libarchive/archive_write_disk_posix.c
@@ -4432,6 +4432,14 @@ set_xattrs(struct archive_write_disk *a)
 			} else if (strncmp(name, "system.", 7) == 0) {
 				name += 7;
 				namespace = EXTATTR_NAMESPACE_SYSTEM;
+				/*
+				 * FreeBSD: quietly skip NFSv4 and POSIX.1e
+				 * ACL extended attributes
+				 */
+				if (strcmp(name, "nfs4.acl") ||
+				    strcmp(name, "posix1e.acl_access") ||
+				    strcmp(name, "posix1e.acl_default"))
+					continue;
 			} else {
 				/* Other namespaces are unsupported */
 				archive_strcat(&errlist, name);

@lattera
Copy link
Contributor Author

@lattera lattera commented Oct 14, 2020

Sure, I guess.

@mmatuska
Copy link
Member

@mmatuska mmatuska commented Oct 14, 2020

Sure, I guess.

If you would be so kind to add the code to your PR and rebase against master, it should pass all tests and I can merge it in automatically. Otherwise I need to do a manual merge.

lattera added 3 commits Oct 14, 2020
In order to teach HardenedBSD's packaging infrastructure how to support
HardenedBSD's method of exploit mitigation toggling, teach libarchive
how to handle the system filesystem extended attribute namespace.

Signed-off-by:	Shawn Webb <shawn.webb@hardenedbsd.org>
The function I added only applies to FreeBSD. As such, if the function
declaration isn't ifdef'd out for other architectures, continuous
integration (CI) builds fail. Mitigate the failure by guarding the
function declaration with the proper preprocessor macro conditional.

Signed-off-by:	Shawn Webb <shawn.webb@hardenedbsd.org>
In order to teach the packaging infrastructure how to support
HardenedBSD's method of exploit mitigation toggling, teach libarchive
how to handle the system filesystem extended attribute namespace.

Signed-off-by:	Shawn Webb <shawn.webb@hardenedbsd.org>
UFS stores NFSv4 ACLs in the system extended attribute namespace. Per
libarchive maintainer, ignore those attributes.

Signed-off-by:	Shawn Webb <shawn.webb@hardenedbsd.org>
@mmatuska mmatuska merged commit 265946a into libarchive:master Oct 14, 2020
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants