contrib/apparmor: align whitespace parsing with libapparmor#13278
contrib/apparmor: align whitespace parsing with libapparmor#13278thaJeztah wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes AppArmor profile generation by ensuring the daemon’s current profile name (read from /proc/self/attr/current) is sanitized to avoid trailing newlines/whitespace that can corrupt the rendered policy template.
Changes:
- Trim leading/trailing whitespace (including newlines) from the cleaned AppArmor profile name after removing the
" (enforce)"suffix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c8bb2f8 to
a419671
Compare
a419671 to
f456e92
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f456e92 to
1c89c8c
Compare
thaJeztah
added
cherry-pick/1.7.x
1c89c8c to
a1fc501
Compare
thaJeztah
changed the title
| // Profile names may contain spaces, so split on " (" rather than the | ||
| // first space. Trim whitespace first because the value includes a | ||
| // trailing newline. | ||
| profile, _, _ = strings.Cut(strings.TrimSpace(profile), " (") |
There was a problem hiding this comment.
Hmm I think it would be safer to go from the end - find the LAST ) and then find a matching ( before it.
Technically the profile name could also include parents.
That's also what the libapparmor does:
https://gitlab.com/apparmor/apparmor/-/blob/master/libraries/libapparmor/src/kernel.c?ref_type=heads#L578-615
With testify, expected values go to the left and use sub-tests. Signed-off-by: Sebastiaan van Stijn <[email protected]>
4ab47b8 to
395340b
Compare
395340b to
d3695e8
Compare
d3695e8 to
02dc375
Compare
| if strings.HasSuffix(con, ")") { | ||
| // Profile names may contain spaces, so split on the last " (" before | ||
| // the trailing ")" rather than the first space. | ||
| if i := strings.LastIndex(con[:len(con)-1], " ("); i >= 0 { | ||
| return con[:i], con[i+2 : len(con)-1] | ||
| } |
There was a problem hiding this comment.
We don't really use the mode, and are only interested in the profile name. For our purpose, such profile names would not us obscure names (profiles including parens). Keeping "valid" values in sync with the kernel won't be a realistic option, so keeping it best effort.
02dc375 to
7cbc66e
Compare
thaJeztah
changed the title
The profile we read from /proc/self/attr/current will contain a newline,
resulting in a stray newline to be added to the profile;
Diff:
--- Expected
+++ Actual
@@ -16,3 +16,4 @@
# Manager may send signals to container processes.
- signal (receive) peer=unconfined,
+ signal (receive) peer=unconfined
+,
# Container processes may send signals amongst themselves.
Trim whitespace to account for newlines and fix handling of whitespace as
AppArmor profile names are allowed to contain spaces when quoted; from the
[apparmor.d(5)] man-page:
PROFILE NAME ( UNQUOTED PROFILE NAME | QUOTED PROFILE NAME )
QUOTED PROFILE NAME = '"' UNQUOTED PROFILE NAME '"'
UNQUOTED PROFILE NAME = (must start with alphanumeric character (after
variable expansion), or '/' AARE have special meanings; see below. May
include VARIABLE. Rules with embedded spaces or tabs must be quoted.)
While we don't use those names in our code, let's make the code correct.
Also update the template to use quotes.
[apparmor.d(5)]: https://manpages.ubuntu.com/manpages/xenial/man5/apparmor.d.5.html
Signed-off-by: Sebastiaan van Stijn <[email protected]>
7cbc66e to
7f2f351
Compare
| } | ||
|
|
||
| if strings.HasSuffix(con, ")") { | ||
| // Profile names may contain spaces, so split on the last " (" before |
There was a problem hiding this comment.
Can profile names contain ( or )?
@samuelkarp Sorry, long answer, as I did some extra digging to be sure; It's not explicitly documented, but there's no real restrictions on the name other than some special characters at the start; AppArmor profile names are allowed to contain spaces when quoted; see apparmor.d(5)
That's also where the fun start; So technically (I doubt anyone would do this) a name like p='my profile (complain)'; f="$(mktemp)"; printf 'profile "%s" flags=(complain) {\n file,\n signal,\n}\n' "$p" > "$f" && sudo apparmor_parser -r "$f" && sudo aa-exec -p "$p" -- sh -c 'printf "self: "; cat /proc/self/attr/current; printf "parent: "; cat /proc/$PPID/attr/current'; sudo apparmor_parser -R "$f"; rm -f "$f"
self: my profile (complain) (complain)
parent: unconfinedWhich would be ambiguous if BUT it looks like the mode is ONLY omitted for Some examples of possible returned *label strings are
"unconfined", "/sbin/dhclient", and "Firefox". The string can
consist of any non-NUL characters but it will be NUL-terminated.
The *label string must be freed using free().
The possible *mode strings are "enforce" and "complain".
Additionally, *mode may be NULL when *label is "unconfined".
The *mode string must not be freed. The *label and *mode strings
come from a single buffer allocation and are separated by a NUL character.So;
|
contrib/apparmor: fix whitespace handling in profile names
The profile we read from /proc/self/attr/current will contain a newline,
resulting in a stray newline to be added to the profile;
Trim whitespace to account for newlines and fix handling of whitespace as
AppArmor profile names are allowed to contain spaces when quoted; from the
apparmor.d(5) man-page:
While we don't use those names in our code, let's make the code correct.