Skip to content
This repository has been archived by the owner on Oct 4, 2023. It is now read-only.

Add apparmor support module #188

Merged
merged 11 commits into from Nov 30, 2016
Merged

Add apparmor support module #188

merged 11 commits into from Nov 30, 2016

Conversation

zyga
Copy link
Contributor

@zyga zyga commented Nov 23, 2016

This patch adds a new set of files, mimicking the xxx-support pattern
used for other changes, that so some extent abstracts the use of
apparmor in snap-confine.

Even when snap-confine is not compiled with apparmor support the same
APIs are available and gracefully degrade to no-ops.

As a small extension, snap confine can now know if it is confined (e.g.
a development version running in a random directory is not confined) and
will no longer crash when changing hats via aa_change_hat().

The patch doesn't yet change any of the tree to use the new functions.
This will be done in the next commit.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

This patch adds a new set of files, mimicking the xxx-support pattern
used for other changes, that to some extent abstracts the use of
apparmor in snap-confine.

Even when snap-confine is not compiled with apparmor support the same
APIs are available and gracefully degrade to no-ops.

As a small extension, snap confine can now know if it is confined (e.g.
a development version running in a random directory is not confined) and
will no longer crash when changing hats via aa_change_hat().

The patch doesn't yet change any of the tree to use the new functions.
This will be done in the next commit.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@zyga zyga mentioned this pull request Nov 23, 2016
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Unconfined processes get "unconfined", not an empty label.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Copy link
Contributor

@tyhicks tyhicks left a comment

Choose a reason for hiding this comment

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

This looks good. I only have two small suggestions that I left inline.

SC_AA_ENFORCE = 1,
// The enforcement mode is "complain"
SC_AA_COMPLAIN,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a new'ish mode (as of the 16.04 kernel), called "mixed". It means that two or more AppArmor profiles are stacked together but not all of their enforcement modes are the same. The mode string returned from aa_getcon() is "mixed".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll update the code to handle this as well.

char *mode = NULL; // mode cannot be free'd
if (aa_getcon(&label, &mode) < 0) {
die("cannot query current apparmor profile");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This misses the situation where AppArmor support is compiled into snap-confine but AppArmor has been explicitly disabled by passing "apparmor=0" on the kernel command line. aa_getcon() will return -1 with errno set to EINVAL in this condition but that errno unfortunately overlaps with some other conditions.

The definitive way to see if AppArmor is enabled is aa_is_enabled(). You should call it first and, if it returns 1, proceed to calling aa_getcon() to check if snap-confine is confined. See the aa_is_enabled() man page for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 Thanks for this feedback. I'll do this.

apparmor->mode = SC_AA_ENFORCE;
} else {
apparmor->mode = SC_AA_INVALID;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to check for "mixed" mode here. I've left a more descriptive review comment in the accompanying header file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This patch changes the apparmor support initialization routine to first
call aa_is_enabled(). The rest of the code is tweaked for readability.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@zyga zyga merged commit 267ca91 into master Nov 30, 2016
@zyga zyga deleted the aa-support branch November 30, 2016 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants