Re: [apparmor] [PATCH] apparmor: make aa_set_current_onexec return void
On 2023-01-15 00:49:52, Quanfa Fu wrote:
> Change the return type to void since it always return 0, and no need
> to do the checking in aa_set_current_onexec.
>
> Signed-off-by: Quanfa Fu
This looks like a safe change to me. There's nothing to error check
within aa_set_current_onexec() so returning void is fine.
Reviewed-by: "Tyler Hicks (Microsoft)"
Tyler
> ---
> security/apparmor/domain.c | 2 +-
> security/apparmor/include/task.h | 2 +-
> security/apparmor/task.c | 5 +
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 6dd3cc5309bf..bbc9c8a87b8e 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -1446,7 +1446,7 @@ int aa_change_profile(const char *fqname, int flags)
> }
>
> /* full transition will be built in exec path */
> - error = aa_set_current_onexec(target, stack);
> + aa_set_current_onexec(target, stack);
> }
>
> audit:
> diff --git a/security/apparmor/include/task.h
> b/security/apparmor/include/task.h
> index 13437d62c70f..01717fe432c3 100644
> --- a/security/apparmor/include/task.h
> +++ b/security/apparmor/include/task.h
> @@ -30,7 +30,7 @@ struct aa_task_ctx {
> };
>
> int aa_replace_current_label(struct aa_label *label);
> -int aa_set_current_onexec(struct aa_label *label, bool stack);
> +void aa_set_current_onexec(struct aa_label *label, bool stack);
> int aa_set_current_hat(struct aa_label *label, u64 token);
> int aa_restore_previous_label(u64 cookie);
> struct aa_label *aa_get_task_label(struct task_struct *task);
> diff --git a/security/apparmor/task.c b/security/apparmor/task.c
> index 84d16a29bfcb..5671a716fcd2 100644
> --- a/security/apparmor/task.c
> +++ b/security/apparmor/task.c
> @@ -93,9 +93,8 @@ int aa_replace_current_label(struct aa_label *label)
> * aa_set_current_onexec - set the tasks change_profile to happen onexec
> * @label: system label to set at exec (MAYBE NULL to clear value)
> * @stack: whether stacking should be done
> - * Returns: 0 or error on failure
> */
> -int aa_set_current_onexec(struct aa_label *label, bool stack)
> +void aa_set_current_onexec(struct aa_label *label, bool stack)
> {
> struct aa_task_ctx *ctx = task_ctx(current);
>
> @@ -103,8 +102,6 @@ int aa_set_current_onexec(struct aa_label *label, bool
> stack)
> aa_put_label(ctx->onexec);
> ctx->onexec = label;
> ctx->token = stack;
> -
> - return 0;
> }
>
> /**
> --
> 2.31.1
>
>
Re: [apparmor] Missing /sys/kernel/security/apparmor
On 2019-10-29 22:28:42, Justin Dick wrote: > Hello all - > > I'm trying to enable snapd on an embedded device, and looking into getting > apparmor support sorted out. I'm working with kernel 3.10 and AFAIK have > everything set up properly in the config. After boot, > /sys/module/apparmor/parameters/enabled is 'Y', but /sys/kernel/security/ is > completely empty. I've tried booting with no explicit flags set in the > kernel boot parameters (relying on the kernel config defaults), and with > setting "security=apparmor apparmor=1". Nothing seems to help. > > Any ideas from anyone? I'm pasting the relevant entries in /proc/config.gz > below. You must mount securityfs as part of the boot process. You can do this manually to verify that it works: $ sudo mount -t securityfs securityfs /sys/kernel/security If that works, you'll need to determine how to best make that happen in early boot of your embedded device. Tyler > > Thanks, all! > Justin > > -sh-3.2# cat /proc/config.gz | gzip -d | grep SECURITY > CONFIG_EXT4_FS_SECURITY=y > # CONFIG_SECURITY_DMESG_RESTRICT is not set > CONFIG_SECURITY=y > CONFIG_SECURITYFS=y > CONFIG_SECURITY_NETWORK=y > # CONFIG_SECURITY_NETWORK_XFRM is not set > CONFIG_SECURITY_PATH=y > # CONFIG_SECURITY_SELINUX is not set > # CONFIG_SECURITY_SMACK is not set > # CONFIG_SECURITY_TOMOYO is not set > CONFIG_SECURITY_APPARMOR=y > CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE=1 > # CONFIG_SECURITY_YAMA is not set > CONFIG_DEFAULT_SECURITY_APPARMOR=y > # CONFIG_DEFAULT_SECURITY_DAC is not set > CONFIG_DEFAULT_SECURITY="apparmor" > -sh-3.2# ls /sys/kernel/security/ > -sh-3.2# cat /sys/module/apparmor/parameters/enabled > Y > > -- > AppArmor mailing list > AppArmor@lists.ubuntu.com > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] You may want to directly subscribe to apparmor-profiles bug mail
Hello, Jann Horn reported that private security bug mail for the apparmor-profiles project on Launchpad was incorrectly made public on the AppArmor mailing list: https://lists.ubuntu.com/archives/apparmor/2018-November/011847.html To fix this problem, I've unsubscribed the AppArmor mailing list from the apparmor-profiles bug mail. If you want to continue receiving bug mail for the apparmor-profiles project, please follow this link to subscribe yourself directly: https://bugs.launchpad.net/apparmor-profiles/+subscriptions Tyler signature.asc Description: PGP signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] private apparmor security bug on public list?
On 2018-11-06 13:55:45, Tyler Hicks wrote: > On 2018-11-06 20:48:40, Jann Horn wrote: > > Hi! > > > > I'm subscribed to apparmor@lists.ubuntu.com, and I noticed that I got > > bug mail for https://bugs.launchpad.net/bugs/1800789 via this list > > when the bug was still marked as a security bug. > > The problem looks to be in the bug subscription configuration for the > apparmor-profiles project: > > https://bugs.launchpad.net/apparmor-profiles/+subscriptions > > The ~apparmor-dev team is subscribed which is described here: > > https://launchpad.net/~apparmor-dev > > You can see that the email address listed is the address for this list. I've unsubscribed ~apparmor-dev from the apparmor-profiles bug notifications. I created a test bug to verify that the change worked as intended: https://bugs.launchpad.net/apparmor-profiles/+bug/1801997 Thanks for reporting this to us! Tyler signature.asc Description: PGP signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] private apparmor security bug on public list?
On 2018-11-06 20:48:40, Jann Horn wrote: > Hi! > > I'm subscribed to apparmor@lists.ubuntu.com, and I noticed that I got > bug mail for https://bugs.launchpad.net/bugs/1800789 via this list > when the bug was still marked as a security bug. The problem looks to be in the bug subscription configuration for the apparmor-profiles project: https://bugs.launchpad.net/apparmor-profiles/+subscriptions The ~apparmor-dev team is subscribed which is described here: https://launchpad.net/~apparmor-dev You can see that the email address listed is the address for this list. It is worth noting that the apparmor project itself does not have this problem: https://bugs.launchpad.net/apparmor/+subscriptions Tyler signature.asc Description: PGP signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] AppArmor Logo Vote
On 05/30/2018 01:57 PM, John Johansen wrote: > A new logo has been proposed by Noah Davis for the apparmor project to use. > All versions of the logo under considerations are included below. > > > This is an open vote, anyone in the community can participate. > > > 1. Vote for the logos basic form a) Vertical split > 2. For default logo color. a) cyan Tyler -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] AppArmor Logo vote
On 05/30/2018 01:50 PM, John Johansen wrote: > > A new logo has been proposed by Noah Davis for the apparmor project to use. > All versions of the logo under considerations are included below. > > > This is an open vote, anyone in the community can participate. > > > 1. Vote for the logos basic form a) Vertical split > 2. For default logo color. a) cyan Tyler -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH][NEXT] apparmor: Fix memory leak of rule on error exit path
Currently on the error exit path the allocated rule is not free'd
causing a memory leak. Fix this by calling aa_audit_rule_free().
Detected by CoverityScan, CID#1468966 ("Resource leaks")
Fixes: cb740f574c7b ("apparmor: modify audit rule support to support profile
stacks")
Signed-off-by: Tyler Hicks
---
security/apparmor/audit.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 575f3e9c8c80..eeaddfe0c0fb 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -200,10 +200,12 @@ int aa_audit_rule_init(u32 field, u32 op, char *rulestr,
void **vrule)
/* Currently rules are treated as coming from the root ns */
rule->label = aa_label_parse(&root_ns->unconfined->label, rulestr,
GFP_KERNEL, true, false);
- if (IS_ERR(rule->label))
+ if (IS_ERR(rule->label)) {
+ aa_audit_rule_free(rule);
return PTR_ERR(rule->label);
- *vrule = rule;
+ }
+ *vrule = rule;
return 0;
}
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] AppArmor and /etc/
On 03/23/2018 05:48 PM, Tyler Hicks wrote: > On 03/23/2018 12:10 PM, John Johansen wrote: >> On 02/06/2018 09:29 AM, Christian Boltz wrote: >>> Hello, >>> >>> Am Montag, 5. Februar 2018, 22:13:19 CET schrieb Marco d'Itri: >>>> On Feb 05, Jamie Strandboge wrote: >>>>> It continues to be a tricky problem. I think mostly we really need >>>>> to >>>>> make sure the binary policy is on the same partition as the text >>>>> policy. If we start thinking of it as binary policy, perhaps we can >>>>> instead put it in /lib. Eg, /lib/apparmor/policy. FHS adherents will >>>>> argue that this isn't the right place, but /etc is no better and the >>>>> FHS doesn't handle early boot well at all (this is presumably why >>>>> system uses /lib/systemd/system). >>>> >>>> If the binary policy may change when /etc is changed then the only >>>> options are /etc/ and /var/. >>>> Please please please do not break this: /lib (which nowadays is >>>> a symlink to /usr/lib) is immutable and can be shared between systems. >>> >>> Agreed, but let me mix in another idea/discussion we [1] had at FOSDEM: >>> >>> What about using an override directory - /usr/something for cache files >>> _shipped in the packages_ (for unmodified profiles), and /var/something >>> to handle the cache for modified profiles. >>> >>> I know this means some additional code in the parser, but would make >>> packaging a pre-built cache much easier when it comes to avoiding >>> *.rpmnew files etc. >>> >>> The way this could work would be: >>> >>> a) for reading the cache / loading a profile >>> - check if there's a valid cache file in /var/something and use it >>> - otherwise check if there's a valid cache file in /usr/something and >>> use it >>> - otherwise write the cache file to /var/something >>> >>> b) for writing the cache >>> - write to /var/something by default >>> - write to /usr/something only when using >>> apparmor_parser --cache-loc /usr/something >>> >>> c) for --purge-cache >>> - only delete files in /var/something (except if --cache-loc is used) >> >> and this already exists (its not ready to land quite yet) in >> https://gitlab.com/jjohansen/apparmor/tree/multicache >> >> it supports overlay caches, where you can provide a list of cache >> locations that are to be searched in order >> >> --cache-loc=/A,/B,/C > > How do I use the cache location of /var/lib/larry,moe,curly with an > overlay using the relative path shemp/? Should we just accept multiple, > ordered --cache-loc's instead? After applying some thought, we're better off accepting quoted comma separated paths: --cache-loc="/var/lib/larry,moe,curly",shemp Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] AppArmor and /etc/
On 03/23/2018 12:10 PM, John Johansen wrote: > On 02/06/2018 09:29 AM, Christian Boltz wrote: >> Hello, >> >> Am Montag, 5. Februar 2018, 22:13:19 CET schrieb Marco d'Itri: >>> On Feb 05, Jamie Strandboge wrote: It continues to be a tricky problem. I think mostly we really need to make sure the binary policy is on the same partition as the text policy. If we start thinking of it as binary policy, perhaps we can instead put it in /lib. Eg, /lib/apparmor/policy. FHS adherents will argue that this isn't the right place, but /etc is no better and the FHS doesn't handle early boot well at all (this is presumably why system uses /lib/systemd/system). >>> >>> If the binary policy may change when /etc is changed then the only >>> options are /etc/ and /var/. >>> Please please please do not break this: /lib (which nowadays is >>> a symlink to /usr/lib) is immutable and can be shared between systems. >> >> Agreed, but let me mix in another idea/discussion we [1] had at FOSDEM: >> >> What about using an override directory - /usr/something for cache files >> _shipped in the packages_ (for unmodified profiles), and /var/something >> to handle the cache for modified profiles. >> >> I know this means some additional code in the parser, but would make >> packaging a pre-built cache much easier when it comes to avoiding >> *.rpmnew files etc. >> >> The way this could work would be: >> >> a) for reading the cache / loading a profile >> - check if there's a valid cache file in /var/something and use it >> - otherwise check if there's a valid cache file in /usr/something and >> use it >> - otherwise write the cache file to /var/something >> >> b) for writing the cache >> - write to /var/something by default >> - write to /usr/something only when using >> apparmor_parser --cache-loc /usr/something >> >> c) for --purge-cache >> - only delete files in /var/something (except if --cache-loc is used) > > and this already exists (its not ready to land quite yet) in > https://gitlab.com/jjohansen/apparmor/tree/multicache > > it supports overlay caches, where you can provide a list of cache > locations that are to be searched in order > > --cache-loc=/A,/B,/C How do I use the cache location of /var/lib/larry,moe,curly with an overlay using the relative path shemp/? Should we just accept multiple, ordered --cache-loc's instead? This is the only question my checked out mind could come up with on a Friday afternoon. Tyler > > with the first cache location (/A) being also the writeable location > (assuming --write-cache is enabled). > > In addition to allowing a set of overlay cache directories it also > provides for multiple caches. One set per kernel feature set. So each > kernel now has its own binary cache that can be pre built and > rebooting into different kernels won't clear the cache. > > This helps solve some of the problems but not all of them. All of the > binary locations have to be available at early boot if we are going to > have systemd load the cache early. And we have different communities > with different requirements. > > - We have read only images, with read only text and binary policy > - We have people wanting to empty out /etc/ (no policy or cache) > - we have people who want to put the early policy in the initrd/initramfs > - We have people who are doing multiple policy and cache locations > ... > > Taking the above overlay approach and applying it to text policy we > could allow for local modification that override shipped distro policy > (in fact something like this is going to be needed for read only > images, but you loose the ability to detect collisions of policy > updates with local changes that dpkg and rpm give us .dokg-new/old/bak > and .rpmnew/save) with the modifications and cache being able to be > placed in complimentary locations. > > In the end we are just going to have to come up with some upstream > defaults that are easy for down streams to change, because we are not > going to be able to please everyone. > > The current idea bouncing around is to have a policy.conf file, which > init and similar functions can use to determine policy and > corresponding cache locations. > > Bearing in mind that syntax etc haven't been determined, it would be > something like > > > [system policy] #notice the overlay policy and cache locations > location=/var/libapparmor/,/etc/apparmor.d/ > cache-loc=/var/cache/apparmor,/etc/apparmor.d/cache > options=--write-cache > > [click policy] > location=/var/lib/apparmor/ > cache-loc=/var/cache/apparmor > options=--write-cache -O no-expr-simplify > > [snap policy] > location=/var/lib/snapd/profiles > cachel-loc=/var/cache/apparmor > options=--write-cache > > [lxd policy] > load-only > managed-by: lxd > cache-loc=/var/lib/lxd/apparmor/cache > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppA
[apparmor] [Bug 1739909] Re: apparmor profile prevents syslog-ng startup (fix included)
A fix for this bug was released in AppArmor 2.12. The upstream commit is
e55583ff27308e3338b5c046de42536bbdd48120
** Changed in: apparmor-profiles
Status: New => Fix Released
--
You received this bug notification because you are a member of AppArmor
Developers, which is subscribed to AppArmor Profiles.
https://bugs.launchpad.net/bugs/1739909
Title:
apparmor profile prevents syslog-ng startup (fix included)
Status in AppArmor Profiles:
Fix Released
Bug description:
Tested on gentoo, syslog-ng-3.13.2, apparmor-profiles-2.11.1-r2;
The apparmor-profile for syslog-ng prevents syslog-ng from accessing
/dev/kmsg, which in turn leads to a failure when starting the daemon.
This occurs when using a source similar to this one:
source kernsrc {
file("/proc/kmsg");
};
Even though the file should be accessed through /proc/kmsg, syslog-ng
checks some conditions on /dev/kmsg before proceeding (checked with
strace). As this file is not allowed to be read by the apparmor
profile, syslog-ng fails to start.
To fix this issue, simply add this permissions line to the apparmor
profile:
/dev/kmsg r,
To manage notifications about this bug go to:
https://bugs.launchpad.net/apparmor-profiles/+bug/1739909/+subscriptions
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Unique audit record type ranges for individual LSMs
On 12/06/2017 12:47 PM, Casey Schaufler wrote: > On 12/6/2017 9:51 AM, Tyler Hicks wrote: >> Hello - The AppArmor project would like for AppArmor audit records to be >> supported by the audit-userspace tools, such as ausearch, but it >> requires some coordination between the linux-security-module and >> linux-audit lists. This was raised as a feature request years ago in >> Ubuntu and more recently in Debian: >> >> https://launchpad.net/bugs/1117804 >> https://bugs.debian.org/872726 >> >> The quick summary of the problem at hand is that the audit-userspace >> project requires that each LSM use a unique record type range for audit >> records while the kernel's common_lsm_audit() function uses the same >> record type (1400) for all records. SELinux, AppArmor, and SMACK are all >> using common_lsm_audit() today and, therefore, the 1400-1499 range. > > My, but this is a rat's nest, isn't it? The constants, such as > AUDIT_MAC_STATUS, > look as if they are intended to be generic. But the comment says the range is > for SELinux. Some of the events, including AUDIT_MAC_MAP_ADD *are* generic, in > that they are from the netlbl subsystem. But some, AUDIT_AVC being paramount, > are indeed SELinux specific. It is a rat's nest, which is why the Ubuntu bug above has lingered for so long. Good point regarding AUDIT_MAC_MAP_ADD being generic. > >> While it will be potentially painful to switch, the AppArmor project is >> considering to use a unique range in order for audit-userspace to >> support AppArmor audit records. IMHO, SMACK would be free to continue >> using 1400-1499 as long as they don't need audit-userspace support and >> SELinux would continue using 1400-1499. > > Aside from the comment that says 1400-1499 are for SELinux, and the three > events (1400-1402) that are SELinux specific, the events really are general. > Why not add the AppArmor specifics to the 1400 range? Give them a generic > sounding name and you'll achieve consistency. Change the comment to say > "Security Module use" instead of "SELinux use". I would be alright with this solution. We could even claim 1400-1599 or even 1400-1699 for "Security Module use" if we thought 100 record types may not be enough in the future. I could see how there may be a desire for each LSM to have their own blocks of 100 where they can assign record types freely without coordination with LSM maintainers but I don't feel like new record types come around often enough for this to matter much. > >> Steve Grubb previously told me that he intends 1500-1599 to be used by >> AppArmor: >> >> https://www.redhat.com/archives/linux-audit/2014-May/msg00119.html >> >> >> John Johansen tells me that AppArmor previously used the 1500-1599 range >> before AppArmor was upstreamed. >> >> There's a conflicting comment in the kernel stating that 1500-1599 is to >> by used by kernel LSPP events. As far as I can tell, there were never >> any kernel LSPP events that used the range. Steve is the one that added >> that comment so I think it is a safe range for AppArmor to use: >> >> https://git.kernel.org/linus/90d526c074ae5db484388da56c399acf892b6c17 >> >> Considering audit-userspace's stance, does the LSM community agree that >> common_lsm_audit() should be modified to accept an audit record type >> parameter to pass on to audit_log_start()? >> >> If so, does everyone agree that 1500-1599 would be acceptable for >> AppArmor to use? > > Why not change the comment and continue to use the 1400 range, adding > events as necessary? I don't have any problems with that but I'd like Steve Grubb to weigh in on it. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] Unique audit record type ranges for individual LSMs
Hello - The AppArmor project would like for AppArmor audit records to be supported by the audit-userspace tools, such as ausearch, but it requires some coordination between the linux-security-module and linux-audit lists. This was raised as a feature request years ago in Ubuntu and more recently in Debian: https://launchpad.net/bugs/1117804 https://bugs.debian.org/872726 The quick summary of the problem at hand is that the audit-userspace project requires that each LSM use a unique record type range for audit records while the kernel's common_lsm_audit() function uses the same record type (1400) for all records. SELinux, AppArmor, and SMACK are all using common_lsm_audit() today and, therefore, the 1400-1499 range. While it will be potentially painful to switch, the AppArmor project is considering to use a unique range in order for audit-userspace to support AppArmor audit records. IMHO, SMACK would be free to continue using 1400-1499 as long as they don't need audit-userspace support and SELinux would continue using 1400-1499. Steve Grubb previously told me that he intends 1500-1599 to be used by AppArmor: https://www.redhat.com/archives/linux-audit/2014-May/msg00119.html John Johansen tells me that AppArmor previously used the 1500-1599 range before AppArmor was upstreamed. There's a conflicting comment in the kernel stating that 1500-1599 is to by used by kernel LSPP events. As far as I can tell, there were never any kernel LSPP events that used the range. Steve is the one that added that comment so I think it is a safe range for AppArmor to use: https://git.kernel.org/linus/90d526c074ae5db484388da56c399acf892b6c17 Considering audit-userspace's stance, does the LSM community agree that common_lsm_audit() should be modified to accept an audit record type parameter to pass on to audit_log_start()? If so, does everyone agree that 1500-1599 would be acceptable for AppArmor to use? Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] test failures in test-aa-easyprof.py
Hi - Please put me in the To or CC headers next time you directly
address me in an email. I don't always have time to read every piece of
mail that hits this list and this is a subject that I'd typically skip
in those times.
On 12/03/2017 08:16 AM, Christian Boltz wrote:
> Hello,
>
> I get several failures from test-aa-easyprof.py, for example
>
> ==
> ERROR: test_output_directory_single (__main__.T)
> Test output_directory (single)
> --
> Traceback (most recent call last):
> File "test-aa-easyprof.py", line 2439, in test_output_directory_single
> easyp.output_policy(params, dir=out_dir)
> File "/home/cb/apparmor/git/apparmor/utils/apparmor/easyprof.py", line 696,
> in output_policy
> policy = self.gen_policy(**params)
> File "/home/cb/apparmor/git/apparmor/utils/apparmor/easyprof.py", line 690,
> in gen_policy
> raise AppArmorException("Invalid policy")
> apparmor.easyprof.AppArmorException: 'Invalid policy'
>
>
> There's a total of 50 errors, all with 'Invalid policy'.
>
> git bisect tracked this down to
>
>
> 7ab65fa5f13c774088d64c3881df798c63d87a44 is the first bad commit
> commit 7ab65fa5f13c774088d64c3881df798c63d87a44
> Author: Tyler Hicks
> Date: Thu Mar 2 21:24:33 2017 +
>
> utils: Set parser executable path according to USE_SYSTEM make variable
>
> if USE_SYSTEM is not set, the utils make check target will instruct
> test-aa-easyprof.py to provide the path of the in-tree parser executable
> to aa-easyprof.
>
> If USE_SYSTEM is set, the default parser path (/sbin/apparmor_parser or
> the result of `which apparmor_parser`) is used.
>
> The test-aa-easyprof.py script receives the parser path by checking the
> __AA_PARSER environment variable. This environment variable is strictly
> used by the test script and not any user-facing code so two leading
> underscores were used.
>
> Signed-off-by: Tyler Hicks
> Acked-by: Christian Boltz
> Acked-by: Seth Arnold
>
> :04 04 870632a03f5f6b013312ed6e68ac0a13c54573a7
> b6b046c11c47cb5ed2dbe125ed45c19e5c3e9114 M utils
>
>
> The "fix" is make -C parser but I'd prefer to have a Makefile
> dependency instead of confusing and misleading test failures ;-)
>
> Tylor, since you broke it, can you please submit a fix for trunk and 2.11? ;-)
https://gitlab.com/apparmor/apparmor/merge_requests/25
Tyler
> (based on the commit date, I'd guess that older releases aren't affected)
>
> Also, a better error message than 'Invalid policy' would be helpful ;-)
>
>
> Regards,
>
> Christian Boltz
>
>
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] AppArmor dependency on python
On 11/20/2017 05:29 AM, Viacheslav Salnikov wrote: > Hi Tyler and John, > > /The majority of the profile manipulation tools are now written in python. > / > Could you please provide more detailed information about these tools? > Like a list, at least. The following tools will be available to you without Python: - apparmor_parser which allows you to compile and load AppArmor profiles - aa-exec which allows you to confine processes with arbitrary AppArmor profiles, namespaces, etc. - aa-enabled which allows you to verify that AppArmor is enabled The following tools will not be available to you since they depend on Python (or Perl): - aa-audit - aa-autodep - aa-cleanprof - aa-complain - aa-decode - aa-disable - aa-enforce - aa-genprof - aa-logprof - aa-mergeprof - aa-status - aa-unconfined - aa-update-browser They're optional and, with the exception of aa-status, not installed by default in Ubuntu. Please see their man pages for details. Tyler > > /$ (cd libraries/libapparmor && ./autogen.sh && ./configure \ > && make && make check) && \ > (cd binutils && make && make check) && \ > (cd parser && make) > / > Thank you, I will try. > > // > // > > 2017-11-17 21:06 GMT+02:00 Tyler Hicks <mailto:tyhi...@canonical.com>>: > > On 11/17/2017 12:57 PM, John Johansen wrote: > > On 11/17/2017 01:33 AM, Viacheslav Salnikov wrote: > >> Hi guys, > >> > >> I have a question about apparmor and its dependency from python. > >> I'm using it with Yocto, apparmor version is 2.11.0. > >> > >> Except*aa-easyprof*, does apparmor or its libraries and utilities use > python for something? I am talking not only about execution but also about > compilation, installing etc. > >> > > the very base of apparmor, parser, libraries, some basic tools > aa-enabled, aa-exec do not use python, this allows for minimal installs with > very few dependencies. > > You should be able to build the library, parser, and binutils without > Python. Your build commands would look something like: > > $ (cd libraries/libapparmor && ./autogen.sh && ./configure \ > && make && make check) && \ > (cd binutils && make && make check) && \ > (cd parser && make) > > You won't be able to run `make check` in parser/ as some of the tests > depend on Python (and some Perl). > > Tyler > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] AppArmor dependency on python
On 11/17/2017 12:57 PM, John Johansen wrote: > On 11/17/2017 01:33 AM, Viacheslav Salnikov wrote: >> Hi guys, >> >> I have a question about apparmor and its dependency from python. >> I'm using it with Yocto, apparmor version is 2.11.0. >> >> Except*aa-easyprof*, does apparmor or its libraries and utilities use python >> for something? I am talking not only about execution but also about >> compilation, installing etc. >> > the very base of apparmor, parser, libraries, some basic tools aa-enabled, > aa-exec do not use python, this allows for minimal installs with very few > dependencies. You should be able to build the library, parser, and binutils without Python. Your build commands would look something like: $ (cd libraries/libapparmor && ./autogen.sh && ./configure \ && make && make check) && \ (cd binutils && make && make check) && \ (cd parser && make) You won't be able to run `make check` in parser/ as some of the tests depend on Python (and some Perl). Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [Bug 1732040] Re: [Pull-Request] Chromium browser on Enforce
No worries at all! You'd have to be following along closely on the mailing list or IRC channel to know about the migration. -- You received this bug notification because you are a member of AppArmor Developers, which is subscribed to AppArmor Profiles. https://bugs.launchpad.net/bugs/1732040 Title: [Pull-Request] Chromium browser on Enforce Status in AppArmor Profiles: Invalid Bug description: Apologies in advance, this is probably the wrong way to send a "Pull- Request" on launchpad. I've been using Chromium Browser on enforce for a couple of weeks. The default profile requires a couple of changes to make it usable. I thought to contribute them back: https://git.launchpad.net/~bbriniotis/apparmor-profiles/commit/ There are two main changes: 1. The chrome-sandbox gets memory map exec permissions to make Chromium run on enforce mode. 2. Gave ixr permissions to gio to make magnet links work. All the changes are made directly to the 18.04 folder. To manage notifications about this bug go to: https://bugs.launchpad.net/apparmor-profiles/+bug/1732040/+subscriptions -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [Bug 1732040] Re: [Pull-Request] Chromium browser on Enforce
Hello and thanks for contacting us. We just migrated the AppArmor code hosting from Launchpad to GitLab a week or two ago. Would it be possible for you to create a merge request in GitLab against the apparmor- profiles project? https://gitlab.com/apparmor/apparmor-profiles Here's some info from GitLab on how to create merge requests: https://docs.gitlab.com/ee/gitlab-basics/add-merge-request.html Let us know if this isn't possible for you and we can figure something else out. Thanks! ** Changed in: apparmor-profiles Status: New => Invalid -- You received this bug notification because you are a member of AppArmor Developers, which is subscribed to AppArmor Profiles. https://bugs.launchpad.net/bugs/1732040 Title: [Pull-Request] Chromium browser on Enforce Status in AppArmor Profiles: Invalid Bug description: Apologies in advance, this is probably the wrong way to send a "Pull- Request" on launchpad. I've been using Chromium Browser on enforce for a couple of weeks. The default profile requires a couple of changes to make it usable. I thought to contribute them back: https://git.launchpad.net/~bbriniotis/apparmor-profiles/commit/ There are two main changes: 1. The chrome-sandbox gets memory map exec permissions to make Chromium run on enforce mode. 2. Gave ixr permissions to gio to make magnet links work. All the changes are made directly to the 18.04 folder. To manage notifications about this bug go to: https://bugs.launchpad.net/apparmor-profiles/+bug/1732040/+subscriptions -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Moving Debian/Ubuntu packaging to Git
On 11/05/2017 05:55 AM, intrigeri wrote: > Hi! > > So far the Debian packaging lives in bzr and I regularly merge from > the apparmor-ubuntu-citrain branch. I want to move it to Git ASAP. +1 > > Does Ubuntu have a plan wrt. packaging src:apparmor in Git? Not at this time. > If not, I will set something up. > > My preferred workflow is: > > - use git-buildpackage + pristine-tar I have only a small amount of gbp + pristine-tar experience with two packages so I've got some questions below. > > - always pass --upstream-vcs-tag to `gbp import-orig', so that the >upstream Git history is merged when importing the new release >tarball When you say that the upstream git history is merged, do you mean that it is squashed into a single commit with a message like "Import v2.12 from upstream"? > > - use `gbp pq' to manage the quilt patch series (but this is left to >personal preferences as the patch-queue branch is never pushed, and >debian/patches/ remains the common format used for sharing/storing >the patch series in Git) I need to read some more about `gbp pq`. This is the first time I've heard of it. > > This is the workflow we're using on the Debian Perl team to maintain > 3k+ packages. I've also been using it on a number of other Debian > packaging teams. It's not perfect but I've found it works better than > anything else I've tried. But I will consider using another workflow > if you folks prefer something else. Can you point us to the packaging git tree of a package that you are particularly happy with? > Additionally, when maintaining packages for more than 1 distro at > a time, I've had good experience with the > http://dep.debian.net/deps/dep14/ branch & tag naming conventions. > It would allow us to share a single packaging Git repo between Debian, > Ubuntu, and other derivatives, if we want to. > > Thoughts? I think we'd benefit from documentation on how many basic tasks should be carried out (importing a new patch, importing a new upstream release, etc.). Like git, gbp feels like a low level utility that everyone uses differently and the general documentation doesn't explain the nuances. > Wrt. the actual repository conversion: > > - I expect some trade-offs will be needed wrt. preserving history: >AFAICT you folks have been merging *by hand* my work into the >apparmor-ubuntu-citrain branch, so bzr is not aware of it, i.e. >my commits don't appear anywhere in your branch. But I've been >merging your branch into mine with bzr, so your commits do appear >in my branch and the merges are encoded as such. In other words, my >the history of my branch is a superset of yours. >So I think I'll convert my own Vcs-Bzr to Git. That sounds reasonable. Tyler > - Suggestions and hints welcome as I've never done bzr→Git >conversions. Steve, could you please share your scripts or notes? > > Cheers, > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/02/2017 04:08 PM, John Johansen wrote: > On 11/02/2017 01:03 PM, Tyler Hicks wrote: >> On 11/02/2017 03:00 PM, John Johansen wrote: >>> ] >>>> We walked through a merge yesterday with this merge request: >>>> >>>> https://gitlab.com/apparmor/apparmor/merge_requests/1 >>>> >>>> The audit trail of who merged the code is implicitly present in the >>>> merge commit. By default, there's no information about who reviewed the >>>> changes but the merge commit contains a mention of where to find the >>>> merge request and that page will contain much more info about who >>>> reviewed which parts of the merge request. >>>> >>> That makes the dangerous assumption we keep our infrastructure on gitlab, >>> and don't endup migrating again (this is the 4 or 5 migration in the >>> projects history). I would strongly prefer having that information >>> integral to the commit message. >>> >>>> I'm fine with the default merge commit message. I think Steve had an >>>> issue with the subject line of the default merge commit message. I'll >>>> let him voice his opposition to it and maybe he'll have a better >>>> suggestion. >>>> I am really not happy with with what I have seen so far. >>> >>> Merge branch 'make-variable' into 'master' >>> >>> all: Use the MAKE variable >>> >>> See merge request apparmor/apparmor!1 >>> >>> >>> uhmm, no that really fails the migration test >> >> Please provide a suggested commit message format that we can all follow. >> > I don't have one, I am really not too bothered with the format. I don't > even care if people are consistent with it. > > What is missing is a couple critical pieces of information. Who was > involved in the merge discussion doing both the reviewing and acking. > Partly as breadcrumbs in the future partly because I want a more > permanent form of acknowledgement of the contribution, which is > both critical and all too easily overlooked. > > I don't even care if it shows up on every patch in the merge or just > the merge message. I just want the info available in the logs, > instead of in the meta info stored in the cloud that may one day > disappear. Does anyone else have strong feelings about this and care to suggest a format/process? We're seemingly blocked on accepting merge requests otherwise. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/02/2017 03:00 PM, John Johansen wrote: > ] >> We walked through a merge yesterday with this merge request: >> >> https://gitlab.com/apparmor/apparmor/merge_requests/1 >> >> The audit trail of who merged the code is implicitly present in the >> merge commit. By default, there's no information about who reviewed the >> changes but the merge commit contains a mention of where to find the >> merge request and that page will contain much more info about who >> reviewed which parts of the merge request. >> > That makes the dangerous assumption we keep our infrastructure on gitlab, > and don't endup migrating again (this is the 4 or 5 migration in the > projects history). I would strongly prefer having that information > integral to the commit message. > >> I'm fine with the default merge commit message. I think Steve had an >> issue with the subject line of the default merge commit message. I'll >> let him voice his opposition to it and maybe he'll have a better suggestion. >> I am really not happy with with what I have seen so far. > > Merge branch 'make-variable' into 'master' > > all: Use the MAKE variable > > See merge request apparmor/apparmor!1 > > > uhmm, no that really fails the migration test Please provide a suggested commit message format that we can all follow. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/02/2017 02:07 PM, Christian Boltz wrote: > Hello, > > Am Mittwoch, 1. November 2017, 21:46:17 CET schrieb Tyler Hicks: >> On 11/01/2017 02:41 PM, Christian Boltz wrote: > >>> Another question is if we want to continue sending patches to the >>> mailinglist, or if we'll switch over to using branches (prefixed >>> with the username, for example "cboltz-utils-foo") and then send >>> merge requests. >> >> What's the reason for the username prefix? > > I should probably explain or just show the full workflow we use for > managing the salt code ;-) > > git checkout -b cboltz-whatever # create a new branch > vi $whatever > git add $whatever > git commit > git push # fails because the branch doesn't have an origin set yet, > # but gives copy&paste-ready instructions with the correct > # push command > git push --options-shown-above # this prints a link for opening a > # merge request > # open the given link to create the merge request, and make sure to > # enable the option to delete the source branch after merging to > # avoid having lots of superfluous branches around. > > Note that we do all this in the *main repo* (for AppArmor, that would be > gitlab.com/apparmor/apparmor), not in a user repo like > gitlab.com/cboltz/apparmor. > > That should also explain the reason for the username prefix in the branch > name - it avoids naming conflicts. (If we decide to do all merge requests > from gitlab.com/$user/apparmor, the name prefix is superfluous.) > > Advantages of having those temporary branches in the main repo are: > - you don't need to refresh your $user repo regularly (by pulling from > the gitlab.com/apparmor/apparmor repo), just run git pull > - it's easy to checkout a branch someone created for review > > The only disadvantage I see is that having everything as branches in the > main repo might be a bit noisy, but that's not a real problem. Thanks. I see why you're using the prefix now. My preference would be to create merge requests from gitlab.com/$user/apparmor. One example of why is that I don't do many reviews of the Python utilities so I'd rather not spend the time pulling down all of the pending merge requests for the Python utilities when I simply want to fetch the latest version of the master branch or maybe a stable branch. In the case that I do a review of a merge request for the Python utilities, I'd add a new 'cboltz' remote and fetch a copy of your tree at that time. > >> I really enjoyed the web-based merge request workflow and think it can >> be an improvement over the mailing list patchset based flow. However, >> I'd strongly recommend that we require contributors to: >> >> 1) Create a merge request >> 2) Receive feedback from maintainers >> 3) If changes are required, fold changes necessary to address feedback >> into the existing patches, rebase, and force push to their merge >> request branch. >> >> #3 is necessary to avoid a bunch of fixup patches that shouldn't be >> standalone. It also makes for an bisect-able tree since there's no >> broken commits being merged (with separate fixup commits). > > Agreed, force push makes sense in this case. We walked through this yesterday with the following merge request: https://gitlab.com/apparmor/apparmor/merge_requests/2 The "Compare with previous version" feature is a killer feature. AFAIK, GitHub is sorely missing that. > >>> I also wonder how to handle the Acked-by messages in case we use >>> merge requests - while it's possible with git rebase + using the >>> "reword" keyword, it means we'll have to force-push to those >>> branches before merging them. >> >> What the maintainer did for the GitHub contribution that I mentioned >> above was to merge my pull request into a local branch, interactive >> rebase to add his Signed-off-by, and then push the resulting branch to >> to the master branch on GitHub. > > As Steve already mentioned, that sounds like lots of work. > > IMHO having the Acked-by only in the merge commit would be enough, at > least if it's the same for all patches/commits in the to-be-merged > branch. > > Things are more interesting[tm] if I ack the first 3 patches of a series, > and you ack the other 4 - in this (probably rare) case, adjusting the > commit messages might indeed make sense. We walked through a merge yesterday with this merge request: https://gitlab.com/apparmor/apparmor/merge_requests/1 The audit trail of who merged
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/01/2017 06:36 PM, Tyler Hicks wrote: > On 11/01/2017 06:34 PM, Seth Arnold wrote: >> On Wed, Nov 01, 2017 at 03:46:17PM -0500, Tyler Hicks wrote: >>> What the maintainer did for the GitHub contribution that I mentioned >>> above was to merge my pull request into a local branch, interactive >>> rebase to add his Signed-off-by, and then push the resulting branch to >>> to the master branch on GitHub. >> >> This sounds like a lot of overhead and extra work. What does the extra >> annotation buy that is lacking from a more simplistic merge? > > An in-tree audit trail of who acked and/or committed the patches. A possible alternative could be to include this information in the merge commit. If you look at a merge request, there's a 'Modify commit message' button that lets you put arbitrary text into the merge commit. I haven't seen a project do that but that doesn't mean much. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/01/2017 06:34 PM, Seth Arnold wrote: > On Wed, Nov 01, 2017 at 03:46:17PM -0500, Tyler Hicks wrote: >> What the maintainer did for the GitHub contribution that I mentioned >> above was to merge my pull request into a local branch, interactive >> rebase to add his Signed-off-by, and then push the resulting branch to >> to the master branch on GitHub. > > This sounds like a lot of overhead and extra work. What does the extra > annotation buy that is lacking from a more simplistic merge? An in-tree audit trail of who acked and/or committed the patches. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/01/2017 05:18 PM, Steve Beattie wrote: > On Wed, Nov 01, 2017 at 03:46:17PM -0500, Tyler Hicks wrote: >>> Am Mittwoch, 1. November 2017, 08:27:12 CET schrieb Steve Beattie: >>>> There more work to do to flesh out the above and standardize on some >>>> practices around git, but this should let us make progress. >>> >>> One thing we use for the openSUSE infrastructure salt code is the >>> "Protected Branches" feature: >>> https://docs.gitlab.com/ce/user/project/protected_branches.html >>> >>> Protected branches prevent force pushing and deleting a branch, which >>> IMHO makes sense for master and the apparmor-* maintenance branches. >>> (Ideally we'll never notice that we have that sort of protection, but it >>> helps to prevent accidents.) >> >> This sounds like a very good thing to enable. > > Agreed, I'll set this up for the apparmor and apparmor-profiles repos. > >>> That's something time will tell, and it probably also depends on the >>> size of the patch. (I'll assume everybody has notifications for new merge >>> requests enabled in the gitlab config, right? ;-) >> >> I recently contributed a fairly complex patch set to a GitHub project >> and will assume that it is a similar experience in GitLab in order to >> give my opinion here. >> >> I really enjoyed the web-based merge request workflow and think it can >> be an improvement over the mailing list patchset based flow. However, >> I'd strongly recommend that we require contributors to: >> >> 1) Create a merge request >> 2) Receive feedback from maintainers >> 3) If changes are required, fold changes necessary to address feedback >> into the existing patches, rebase, and force push to their merge request >> branch. >> >> #3 is necessary to avoid a bunch of fixup patches that shouldn't be >> standalone. It also makes for an bisect-able tree since there's no >> broken commits being merged (with separate fixup commits). >> >>> I also wonder how to handle the Acked-by messages in case we use merge >>> requests - while it's possible with git rebase + using the "reword" >>> keyword, it means we'll have to force-push to those branches before >>> merging them. >> >> What the maintainer did for the GitHub contribution that I mentioned >> above was to merge my pull request into a local branch, interactive >> rebase to add his Signed-off-by, and then push the resulting branch to >> to the master branch on GitHub. > > Do you have a pointer to the merge request/commit so that we can see > what that ended up looking like in git? PR: https://github.com/seccomp/libseccomp/pull/92 My branch for the PR: https://github.com/tyhicks/libseccomp/commits/improved-logging Upstream commit log after merging: https://github.com/seccomp/libseccomp/commits/0cc7203760c4776c67602a531bd633aba63a7851 Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [administrivia] git conversion complete; gitlab projects set up
On 11/01/2017 02:41 PM, Christian Boltz wrote: > Hello, > > thanks for doing the migration! > > Am Mittwoch, 1. November 2017, 08:27:12 CET schrieb Steve Beattie: >> There more work to do to flesh out the above and standardize on some >> practices around git, but this should let us make progress. > > One thing we use for the openSUSE infrastructure salt code is the > "Protected Branches" feature: > https://docs.gitlab.com/ce/user/project/protected_branches.html > > Protected branches prevent force pushing and deleting a branch, which > IMHO makes sense for master and the apparmor-* maintenance branches. > (Ideally we'll never notice that we have that sort of protection, but it > helps to prevent accidents.) This sounds like a very good thing to enable. > Protected branches can also be configured to completely deny pushing to > a branch, but that would be too much for us IMHO, and would especially > make backporting more paperwork (create a branch on top of apparmor-2.x, > commit your changes, then merge it). > > According to the documentation, you should be able to choose a group for > "Allowed to merge" and "Allowed to push" for each branch. > > > Another question is if we want to continue sending patches to the > mailinglist, or if we'll switch over to using branches (prefixed with the > username, for example "cboltz-utils-foo") and then send merge requests. What's the reason for the username prefix? > That's something time will tell, and it probably also depends on the > size of the patch. (I'll assume everybody has notifications for new merge > requests enabled in the gitlab config, right? ;-) I recently contributed a fairly complex patch set to a GitHub project and will assume that it is a similar experience in GitLab in order to give my opinion here. I really enjoyed the web-based merge request workflow and think it can be an improvement over the mailing list patchset based flow. However, I'd strongly recommend that we require contributors to: 1) Create a merge request 2) Receive feedback from maintainers 3) If changes are required, fold changes necessary to address feedback into the existing patches, rebase, and force push to their merge request branch. #3 is necessary to avoid a bunch of fixup patches that shouldn't be standalone. It also makes for an bisect-able tree since there's no broken commits being merged (with separate fixup commits). > I also wonder how to handle the Acked-by messages in case we use merge > requests - while it's possible with git rebase + using the "reword" > keyword, it means we'll have to force-push to those branches before > merging them. What the maintainer did for the GitHub contribution that I mentioned above was to merge my pull request into a local branch, interactive rebase to add his Signed-off-by, and then push the resulting branch to to the master branch on GitHub. Tyler > Oh, and there's still the question if we can get commit notification > mails - does someone have an idea? > (For now, I subscribed to the RSS feed - but it contains only the commit > message, not the diff.) > > > Regards, > > Christian Boltz > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] test git repo
On 10/03/2017 12:46 PM, intrigeri wrote: > Hi, > > Steve Beattie: >> So to be explicit, I'm not aware of anyone seriously suggesting we >> stay with Launchpad. What I'd personally rather hear are the pros and >> cons of maintaining a project on github vs gitlab, because I don't >> have experience doing so with either service. Most of my personal >> experience with either has been related to tracking down individual >> commits to cherry-pick for security updates, along with filing PRs. > > I'm not using GitHub to maintain any project so I can't tell. > > I'm using GitLab as one of the main maintainers of a few small > projects (some on gitlab.com, some other 0xacab.org). So far I like > it. The "merge if tests pass" button is amazing, once I've managed to > convince myself that it's OK that a machine commits to the repo (but > then I always compare the merge done by GitLab to the one I've done > locally ;) > > I'm not 100% convinced by the "flat namespace for issues" model, i.e. > do everything with tags. But apparently it works for big projects… > like developing the GitLab software itself. So I guess one simply has > to adjust their workflow to the tool/concepts rather than try to make > a pre-existing workflow, formatted for other issue tracking tools, > work as-is in GitLab. Last time I checked, GitHub had exactly the same > "problem" here. > >> (I personally have a mild bias against github due to it not being >> open source and seemingly fostering a culture of harassment and abuse.) > > Same. Unless there are very strong good reasons to go with GitHub, the > ability to move projects to another GitLab instance whenever we want > is the decisive factor for me. When using GitHub I feel that I'm > donating some of my work, data and power for free to an external > entity, which does not fit very well into my ethics. I understand and respect this point of view. However, I also recognize the value of drive-by contributions (which may turn into more regular contributions) that, as I understand it, are much more likely to happen on GitHub than GitLab. We shouldn't ignore that as more involvement in the project would be a wonderful outcome from changing the project hosting. Tyler > > FWIW, Debian's Git hosting will switch to GitLab soon; GNOME is > switching to GitLab as well. > > Cheers, > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] test git repo
On 09/26/2017 04:26 PM, Steve Beattie wrote: > Hello, > > I've made available a test apparmor git repository at > > https://code.launchpad.net/~sbeattie/apparmor/+git/apparmor > > You can git clone it via > > git clone https://git.launchpad.net/~sbeattie/apparmor/+git/apparmor > > Please feel free to take check out it, as I'd like to cut over > permanently to git in the next day or two. Thanks for doing this work! The tree looks really good to me and I'm unable to spot any potential problems. Tyler > I have not yet done the work to make the tarball generation work with > git and the .bzrignore needs to be converted to .gitignore, but > otherwise the tree should be complete. > > Please let me know if you see any issues with the repo. Thanks! > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] regression test: conditionaly run pivot_root domain, transitions
On 09/07/2017 06:44 PM, John Johansen wrote:
> Document the use of the features_X and requires() functions
>
> Signed-off-by: John Johansen
Thanks! I have a few typo fixes mentioned below but feel free to fix
them, add my ack, and commit.
Acked-by: Tyler Hicks
>
>
> === modified file 'tests/regression/apparmor/prologue.inc'
> --- tests/regression/apparmor/prologue.inc2017-09-07 09:28:06 +
> +++ tests/regression/apparmor/prologue.inc2017-09-07 23:42:21 +
> @@ -23,6 +23,12 @@
>
> #use $() to retreive the failure message or "true" if success
>
> +# kernel_features_istrue() - test whether boolean files are true
> +# $@: path(s) to test if true
> +# Returns: 0 and "true" if all specified paths exist and are true
> +# 1 and error message if features directory is not available
> +# 2 and error message if feature file does not exist
> +# 3 and error message if feature path is not a file
> kernel_features_istrue()
> {
> if [ ! -e "/sys/kernel/security/apparmor/features/" ] ; then
> @@ -46,6 +52,11 @@
> return 0;
> }
>
> +# kernel_features - test whether path(s) are present
> +# $@: feature path(s) to test
> +# Returns: 0 and outputs "true" if all paths exist
> +# 1 and error message if features dir is not available
> +# 2 and error message if path does not exist
> kernel_features()
> {
> if [ ! -e "/sys/kernel/security/apparmor/features/" ] ; then
> @@ -64,6 +75,8 @@
> return 0;
> }
>
> +# requires_kernek_features() - exit if kernel feature does not exist
s/requires_kernek_features/requires_kernel_features/
> +# $@: feature path(s) to test
> requires_kernel_features()
> {
> local res=$(kernel_features $@)
> @@ -73,6 +86,7 @@
> fi
> }
>
> +# requires_namespace_interface() - exit if namespace iterface is not
> available
s/iterface/interface/
> requires_namespace_interface()
> {
> if [ ! -e "/sys/kernel/security/apparmor/policy/namespaces" ]
> @@ -82,6 +96,7 @@
> fi
> }
>
> +# requires_query_interface() - exit if the query interface is not available
> requires_query_interface()
> {
> if [ ! -e "/sys/kernel/security/apparmor/.access" ]
> @@ -91,6 +106,10 @@
> fi
> }
>
> +# parser_supports() - test if the parser supports the following rules
> +# $@: rules to test, use quotes if the rule contains ws
s/ws/whitespace/
(I had to think about that one :)
> +# Returns: 0 and output "true" if all rules supported
> +# 1 and error message if compiler does not support rule
> parser_supports()
> {
> for R in "$@" ; do
> @@ -105,6 +124,8 @@
> return 0;
> }
>
> +#requires_parser_support() - exit if the parser does not suppor the rules
s/suppor/support/
Tyler
> +# $@: rules to test
> requires_parser_support()
> {
> local res=$(parser_supports $@)
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] regression test: conditionaly run pivot_root domain, transitions
On 09/07/2017 05:50 PM, John Johansen wrote: > On 09/07/2017 01:27 PM, Tyler Hicks wrote: >> On 09/06/2017 03:09 PM, John Johansen wrote: >>> Update the tests to test whether the kernel and parser support domain >>> transitions on pivot_root. >>> >>> Signed-off-by: John Johansen >>> --- >>> tests/regression/apparmor/pivot_root.sh | 68 >>> ++--- >>> tests/regression/apparmor/prologue.inc | 24 >>> 2 files changed, 62 insertions(+), 30 deletions(-) >>> >>> diff --git a/tests/regression/apparmor/pivot_root.sh >>> b/tests/regression/apparmor/pivot_root.sh >>> index b68f6cf..0e13a0a 100755 >>> --- a/tests/regression/apparmor/pivot_root.sh >>> +++ b/tests/regression/apparmor/pivot_root.sh >>> @@ -155,34 +155,42 @@ do_test "bad put_old, new_root" fail "$put_old" >>> "$new_root" "$test" >>> genprofile $cur $cap "pivot_root:oldroot=$put_old $bad" >>> do_test "put_old, bad new_root" fail "$put_old" "$new_root" "$test" >>> >>> -# Give sufficient perms and perform a profile transition >>> -genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur >>> -do_test "transition" pass "$put_old" "$new_root" "$new_prof" >>> +if [ "$(kernel_features_istrue namespaces/pivot_root)" != "true" ] ; then >>> +echo " kernel does not support pivot_root domain transitions skipping >>> tests ..." >>> +elif [ "$(parser_supports 'pivot_root -> foo,')" != "true" ] ; then >>> +#pivot_root domain transitions not supported >>> +echo " parser does not support pivot root domain transitions skipping >>> tests ..." >>> +else >>> +# Give sufficient perms and perform a profile transition >>> +genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur >>> +do_test "transition" pass "$put_old" "$new_root" "$new_prof" >>> + >>> +# Ensure failure when the the new profile can't read >>> /proc//attr/current >>> +genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof >>> +do_test "transition, no perms" fail "$put_old" "$new_root" "$new_prof" >>> + >>> +# Ensure failure when the new profile doesn't exist >>> +genprofile $cap "pivot_root:-> $bad" -- image=$new_prof $cur >>> +do_test "bad transition" fail "$put_old" "$new_root" "$new_prof" >>> + >>> +# Ensure the test binary is accurately doing post pivot_root profile >>> verification >>> +genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur >>> +do_test "bad transition comparison" fail "$put_old" "$new_root" "$test" >>> + >>> +# Give sufficient perms with new_root and a transition >>> +genprofile $cap "pivot_root:$new_root -> $new_prof" -- image=$new_prof >>> $cur >>> +do_test "new_root, transition" pass "$put_old" "$new_root" "$new_prof" >>> + >>> +# Ensure failure when the new profile doesn't exist and new_root is >>> specified >>> +genprofile $cap "pivot_root:$new_root -> $bad" -- image=$new_prof $cur >>> +do_test "new_root, bad transition" fail "$put_old" "$new_root" >>> "$new_prof" >>> + >>> +# Give sufficient perms with new_root, put_old, and a transition >>> +genprofile $cap "pivot_root:oldroot=$put_old $new_root -> $new_prof" >>> -- image=$new_prof $cur >>> +do_test "put_old, new_root, transition" pass "$put_old" "$new_root" >>> "$new_prof" >>> + >>> +# Ensure failure when the new profile doesn't exist and new_root and >>> put_old are specified >>> +genprofile $cap "pivot_root:oldroot=$put_old $new_root -> $bad" -- >>> image=$new_prof $cur >>> +do_test "put_old, new_root, bad transition" fail "$put_old" >>> "$new_root" "$new_prof" >>> >>> -# Ensure failure when the the new profile
Re: [apparmor] [PATCH] regression test: conditionaly run pivot_root domain, transitions
On 09/06/2017 03:09 PM, John Johansen wrote: > Update the tests to test whether the kernel and parser support domain > transitions on pivot_root. > > Signed-off-by: John Johansen > --- > tests/regression/apparmor/pivot_root.sh | 68 > ++--- > tests/regression/apparmor/prologue.inc | 24 > 2 files changed, 62 insertions(+), 30 deletions(-) > > diff --git a/tests/regression/apparmor/pivot_root.sh > b/tests/regression/apparmor/pivot_root.sh > index b68f6cf..0e13a0a 100755 > --- a/tests/regression/apparmor/pivot_root.sh > +++ b/tests/regression/apparmor/pivot_root.sh > @@ -155,34 +155,42 @@ do_test "bad put_old, new_root" fail "$put_old" > "$new_root" "$test" > genprofile $cur $cap "pivot_root:oldroot=$put_old $bad" > do_test "put_old, bad new_root" fail "$put_old" "$new_root" "$test" > > -# Give sufficient perms and perform a profile transition > -genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur > -do_test "transition" pass "$put_old" "$new_root" "$new_prof" > +if [ "$(kernel_features_istrue namespaces/pivot_root)" != "true" ] ; then > +echo " kernel does not support pivot_root domain transitions skipping > tests ..." > +elif [ "$(parser_supports 'pivot_root -> foo,')" != "true" ] ; then > +#pivot_root domain transitions not supported > +echo " parser does not support pivot root domain transitions skipping > tests ..." > +else > +# Give sufficient perms and perform a profile transition > +genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur > +do_test "transition" pass "$put_old" "$new_root" "$new_prof" > + > +# Ensure failure when the the new profile can't read > /proc//attr/current > +genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof > +do_test "transition, no perms" fail "$put_old" "$new_root" "$new_prof" > + > +# Ensure failure when the new profile doesn't exist > +genprofile $cap "pivot_root:-> $bad" -- image=$new_prof $cur > +do_test "bad transition" fail "$put_old" "$new_root" "$new_prof" > + > +# Ensure the test binary is accurately doing post pivot_root profile > verification > +genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur > +do_test "bad transition comparison" fail "$put_old" "$new_root" "$test" > + > +# Give sufficient perms with new_root and a transition > +genprofile $cap "pivot_root:$new_root -> $new_prof" -- image=$new_prof > $cur > +do_test "new_root, transition" pass "$put_old" "$new_root" "$new_prof" > + > +# Ensure failure when the new profile doesn't exist and new_root is > specified > +genprofile $cap "pivot_root:$new_root -> $bad" -- image=$new_prof $cur > +do_test "new_root, bad transition" fail "$put_old" "$new_root" > "$new_prof" > + > +# Give sufficient perms with new_root, put_old, and a transition > +genprofile $cap "pivot_root:oldroot=$put_old $new_root -> $new_prof" -- > image=$new_prof $cur > +do_test "put_old, new_root, transition" pass "$put_old" "$new_root" > "$new_prof" > + > +# Ensure failure when the new profile doesn't exist and new_root and > put_old are specified > +genprofile $cap "pivot_root:oldroot=$put_old $new_root -> $bad" -- > image=$new_prof $cur > +do_test "put_old, new_root, bad transition" fail "$put_old" "$new_root" > "$new_prof" > > -# Ensure failure when the the new profile can't read /proc//attr/current > -genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof > -do_test "transition, no perms" fail "$put_old" "$new_root" "$new_prof" > - > -# Ensure failure when the new profile doesn't exist > -genprofile $cap "pivot_root:-> $bad" -- image=$new_prof $cur > -do_test "bad transition" fail "$put_old" "$new_root" "$new_prof" > - > -# Ensure the test binary is accurately doing post pivot_root profile > verification > -genprofile $cap "pivot_root:-> $new_prof" -- image=$new_prof $cur > -do_test "bad transition comparison" fail "$put_old" "$new_root" "$test" > - > -# Give sufficient perms with new_root and a transition > -genprofile $cap "pivot_root:$new_root -> $new_prof" -- image=$new_prof $cur > -do_test "new_root, transition" pass "$put_old" "$new_root" "$new_prof" > - > -# Ensure failure when the new profile doesn't exist and new_root is specified > -genprofile $cap "pivot_root:$new_root -> $bad" -- image=$new_prof $cur > -do_test "new_root, bad transition" fail "$put_old" "$new_root" "$new_prof" > - > -# Give sufficient perms with new_root, put_old, and a transition > -genprofile $cap "pivot_root:oldroot=$put_old $new_root -> $new_prof" -- > image=$new_prof $cur > -do_test "put_old, new_root, transition" pass "$put_old" "$new_root" > "$new_prof" > - > -# Ensure failure when the new profile doesn't exist and new_root and put_old > are specified > -genprofile $cap "pivot_root:oldroot=$put_old $new_root -> $bad" -- > image=$new_prof $cur > -do_test "put_old, new_root, bad transition" fail "$put_old" "$new_root"
Re: [apparmor] RFC: draft proposal for enabling AppArmor by default in Debian
On 08/04/2017 06:56 AM, intrigeri wrote: > Michael Biebl: >> One suggestion: I just tried to run "debcheckout apparmor" which failed >> because I didn't have bzr installed. I think you'd make apparmor more >> approachable for other maintainers if the repo was using git. > > Sure (and it would make my life much easier :) > > Both upstream and the Ubuntu packaging are still using bzr, so for now > stick to Vcs-Bzr for the Debian packaging eases cross-distro > collaboration. But they/we are in the process of migrating to Git, > e.g. the apparmor-profiles repo now uses Git. So this will be fixed > eventually. Speaking as a member of AppArmor upstream, we haven't had a strong reason to switch to git but I think it is safe to say that it is something we'd prioritize higher if we were about to have new people become active in the project. Personally, I'd love to see the switch happen soon. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 0/2] minor man page cleanups
I noticed a few things that could be cleaned up in the aa-enabled and aa-status man pages while reviewing Jamie's aa-status syntax fix. I'm only nominating these for master as these don't fix build failures or anything along those lines. Tyler -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 2/2] utils: update aa-status.pod to unify exit status and bugs sections
Create an EXIT STATUS header and place the BUGS section after the EXIT STATUS section to match the style in aa-enabled.pod. Signed-off-by: Tyler Hicks --- utils/aa-status.pod | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/utils/aa-status.pod b/utils/aa-status.pod index 22f9868..677f89b 100644 --- a/utils/aa-status.pod +++ b/utils/aa-status.pod @@ -91,13 +91,9 @@ displays a short usage statement. =back -=head1 BUGS - -B must be run as root to read the state of the loaded -policy from the apparmor module. It uses the /proc filesystem to determine -which processes are confined and so is susceptible to race conditions. +=head1 EXIT STATUS -Upon exiting, B will set its return value to the +Upon exiting, B will set its exit status to the following values: =over 4 @@ -125,6 +121,12 @@ the apparmor control files. =back +=head1 BUGS + +B must be run as root to read the state of the loaded +policy from the apparmor module. It uses the /proc filesystem to determine +which processes are confined and so is susceptible to race conditions. + If you find any additional bugs, please report them at L<https://bugs.launchpad.net/apparmor/+filebug>. -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 1/2] binutils: update aa-enabled.pod to unify exit status styles
Make the possible exit status values bold to match the style used in aa-status.pod as of r3680. Signed-off-by: Tyler Hicks --- binutils/aa-enabled.pod | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/binutils/aa-enabled.pod b/binutils/aa-enabled.pod index bc9603e..5eef9de 100644 --- a/binutils/aa-enabled.pod +++ b/binutils/aa-enabled.pod @@ -56,27 +56,27 @@ Upon exiting, B will set its exit status to the following values: =over 4 -=item 0: +=item B<0> if AppArmor is enabled. -=item 1: +=item B<1> if AppArmor is not enabled/loaded. -=item 2: +=item B<2> intentionally not used as an B exit status. -=item 3: +=item B<3> if the AppArmor control files aren't available under /sys/kernel/security/. -=item 4: +=item B<4> if B doesn't have enough privileges to read the apparmor control files. -=item 64: +=item B<64> if any unexpected error or condition is encountered. -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] Location of the AppArmor test suite?
On 07/25/2017 06:00 PM, Casey Schaufler wrote: > What is the best place to get the AppArmor kernel test suite? > I haven't found an obvious source. Hey Casey - They're in the AppArmor userspace project. Here's a link to the README: http://bazaar.launchpad.net/~apparmor-dev/apparmor/master/view/head:/tests/regression/apparmor/README See the "Running tests" section for instructions. Be sure to run them in a testing VM and not on a system that you care about. Let us know if you have any other questions or need to make sense of any failures. Tyler > > Thank you. > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2] parser: Return non-zero when the given path is invalid
Christian reported that `apparmor_parser -r /file/not/found` returns 0
indicating that the profile was loaded as expected even though
/file/not/found does not exist in the filesystem. This patch ensures
that a non-zero error code is returned when a specified file or
directory is not found, can't be opened for reading, etc.
Signed-off-by: Tyler Hicks
Tested-by: Christian Boltz
Acked-by: John Johansen
---
parser/lib.c | 3 +++
parser/parser_main.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/parser/lib.c b/parser/lib.c
index 11c2210..053765e 100644
--- a/parser/lib.c
+++ b/parser/lib.c
@@ -16,6 +16,7 @@
* Ltd.
*/
+#include
#include
#include
@@ -32,10 +33,12 @@ int dirat_for_each(int dirfd, const char *name, void *data,
int (* cb)(int, const char *, struct stat *, void *))
{
int retval = _aa_dirat_for_each(dirfd, name, data, cb);
+ int save = errno;
if (retval)
PDEBUG("dirat_for_each failed: %m\n");
+ errno = save;
return retval;
}
diff --git a/parser/parser_main.c b/parser/parser_main.c
index 80c243d..8f1af4f 100644
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -1159,6 +1159,7 @@ int main(int argc, char *argv[])
continue;
if (profilename && stat(profilename, &stat_file) == -1) {
+ last_error = errno;
PERROR("File %s not found, skipping...\n", profilename);
continue;
}
@@ -1175,6 +1176,7 @@ int main(int argc, char *argv[])
cb = binary_input ? binary_dir_cb : profile_dir_cb;
if ((retval = dirat_for_each(AT_FDCWD, profilename,
&cb_data, cb))) {
+ last_error = errno;
PDEBUG("Failed loading profiles from %s\n",
profilename);
}
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] parser: Return non-zero when a specified profile fails to parse
On 05/11/2017 04:39 PM, Tyler Hicks wrote:
> Christian reported that `apparmor_parser -r /file/not/found` returns 0
> indicating that the profile was loaded as expected even though
> /file/not/found does not exist in the filesystem. This patch ensures
> that a non-zero error code is returned when a specified file or
> directory is not found, readable, etc.
>
> Signed-off-by: Tyler Hicks
> Tested-by: Christian Boltz
> Acked-by: John Johansen
> ---
> parser/lib.c | 3 +++
> parser/parser_main.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/parser/lib.c b/parser/lib.c
> index 11c2210..053765e 100644
> --- a/parser/lib.c
> +++ b/parser/lib.c
> @@ -16,6 +16,7 @@
> * Ltd.
> */
>
> +#include
> #include
>
> #include
> @@ -32,10 +33,12 @@ int dirat_for_each(int dirfd, const char *name, void
> *data,
> int (* cb)(int, const char *, struct stat *, void *))
> {
> int retval = _aa_dirat_for_each(dirfd, name, data, cb);
> + int save = errno;
>
> if (retval)
> PDEBUG("dirat_for_each failed: %m\n");
>
> + errno = save;
> return retval;
> }
>
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index 80c243d..5c5129f 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -1160,6 +1160,7 @@ int main(int argc, char *argv[])
>
> if (profilename && stat(profilename, &stat_file) == -1) {
> PERROR("File %s not found, skipping...\n", profilename);
> + last_error = ENOENT;
We can do better than this. last_error should be assigned the value of
errno just before PERROR() is called. I'll send a v2 after the tests
finish running.
Tyler
> continue;
> }
>
> @@ -1175,6 +1176,7 @@ int main(int argc, char *argv[])
> cb = binary_input ? binary_dir_cb : profile_dir_cb;
> if ((retval = dirat_for_each(AT_FDCWD, profilename,
>&cb_data, cb))) {
> + last_error = errno;
> PDEBUG("Failed loading profiles from %s\n",
> profilename);
> }
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] parser: Return non-zero when a specified profile fails to parse
On 05/11/2017 04:39 PM, Tyler Hicks wrote:
> Christian reported that `apparmor_parser -r /file/not/found` returns 0
> indicating that the profile was loaded as expected even though
> /file/not/found does not exist in the filesystem. This patch ensures
> that a non-zero error code is returned when a specified file or
> directory is not found, readable, etc.
>
> Signed-off-by: Tyler Hicks
> Tested-by: Christian Boltz
> Acked-by: John Johansen
> ---
Nominated for trunk, 2.11, and 2.10.
Tyler
> parser/lib.c | 3 +++
> parser/parser_main.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/parser/lib.c b/parser/lib.c
> index 11c2210..053765e 100644
> --- a/parser/lib.c
> +++ b/parser/lib.c
> @@ -16,6 +16,7 @@
> * Ltd.
> */
>
> +#include
> #include
>
> #include
> @@ -32,10 +33,12 @@ int dirat_for_each(int dirfd, const char *name, void
> *data,
> int (* cb)(int, const char *, struct stat *, void *))
> {
> int retval = _aa_dirat_for_each(dirfd, name, data, cb);
> + int save = errno;
>
> if (retval)
> PDEBUG("dirat_for_each failed: %m\n");
>
> + errno = save;
> return retval;
> }
>
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index 80c243d..5c5129f 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -1160,6 +1160,7 @@ int main(int argc, char *argv[])
>
> if (profilename && stat(profilename, &stat_file) == -1) {
> PERROR("File %s not found, skipping...\n", profilename);
> + last_error = ENOENT;
> continue;
> }
>
> @@ -1175,6 +1176,7 @@ int main(int argc, char *argv[])
> cb = binary_input ? binary_dir_cb : profile_dir_cb;
> if ((retval = dirat_for_each(AT_FDCWD, profilename,
>&cb_data, cb))) {
> + last_error = errno;
> PDEBUG("Failed loading profiles from %s\n",
> profilename);
> }
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] parser: Return non-zero when a specified profile fails to parse
Christian reported that `apparmor_parser -r /file/not/found` returns 0
indicating that the profile was loaded as expected even though
/file/not/found does not exist in the filesystem. This patch ensures
that a non-zero error code is returned when a specified file or
directory is not found, readable, etc.
Signed-off-by: Tyler Hicks
Tested-by: Christian Boltz
Acked-by: John Johansen
---
parser/lib.c | 3 +++
parser/parser_main.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/parser/lib.c b/parser/lib.c
index 11c2210..053765e 100644
--- a/parser/lib.c
+++ b/parser/lib.c
@@ -16,6 +16,7 @@
* Ltd.
*/
+#include
#include
#include
@@ -32,10 +33,12 @@ int dirat_for_each(int dirfd, const char *name, void *data,
int (* cb)(int, const char *, struct stat *, void *))
{
int retval = _aa_dirat_for_each(dirfd, name, data, cb);
+ int save = errno;
if (retval)
PDEBUG("dirat_for_each failed: %m\n");
+ errno = save;
return retval;
}
diff --git a/parser/parser_main.c b/parser/parser_main.c
index 80c243d..5c5129f 100644
--- a/parser/parser_main.c
+++ b/parser/parser_main.c
@@ -1160,6 +1160,7 @@ int main(int argc, char *argv[])
if (profilename && stat(profilename, &stat_file) == -1) {
PERROR("File %s not found, skipping...\n", profilename);
+ last_error = ENOENT;
continue;
}
@@ -1175,6 +1176,7 @@ int main(int argc, char *argv[])
cb = binary_input ? binary_dir_cb : profile_dir_cb;
if ((retval = dirat_for_each(AT_FDCWD, profilename,
&cb_data, cb))) {
+ last_error = errno;
PDEBUG("Failed loading profiles from %s\n",
profilename);
}
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] apparmor (2.10.95-4ubuntu5.3) yakkety-security freedesktop.org missing
On 05/10/2017 05:28 AM, Klaus Frick wrote: > Hello, > > i am using ubuntu16.04 (uname -r 4.8.0-51-generic). I have problems with > a DVB-T2 usb-driver on ubuntu16.10. So I went back to 16.04 and checked > syslog. I don`t think this is my problem, but it shuld be fixed. > > the file is in list, but not in the package. > > /etc/apparmor.d/abstractions/freedesktop.org > > it is referenced in telepathy > > May 10 11:04:21 kftdv-P5Q-VM kernel: [13066.034712] audit: type=1400 > audit(1494407061.513:79): apparmor="STATUS" operation="profile_replace" > profile="unconfined" name="/usr/lib/lightdm/lightdm-guest-session" > pid=8942 comm="apparmor_parser" This and the last line beow are harmless messages that are simply saying that the specified profile was loaded. > May 10 11:04:21 kftdv-P5Q-VM apparmor[8890]: AppArmor-Analysefehler f?r > /etc/apparmor.d/usr.lib.telepathy in /etc/apparmor.d/abstractions/gnome > in Zeile 15: >>abstractions/freedesktop.org<< konnte nicht ge?ffnet werden FYI for others, translate.google.com gives the following English translation: AppArmor analysis error for /etc/apparmor.d/usr.lib.telepathy in /etc/apparmor.d/abstractions/gnome in line 15: >> abstractions / freedesktop.org << could not be opened This is an error. However, it doesn't look to be the fault of the apparmor package in yakkety. The freedesktop.org abstraction was somehow deleted from your system but it was originally provided by the apparmor package: $ dpkg -l apparmor | cat Desired=Unknown/Install/Remove/Purge/Hold | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad) ||/ Name VersionArchitecture Description +++-==-==--== ii apparmor 2.10.95-4ubuntu5.3 amd64user-space parser utility for AppArmor $ dpkg --listfiles apparmor | grep freedesktop /etc/apparmor.d/abstractions/freedesktop.org To fix this problem on your system, you can run the following command: $ sudo apt-get -o Dpkg::Options::="--force-confask" install \ --reinstall apparmor You'll be prompted about how you want to handle any discrepancies between the abstractions shipped in the apparmor package and any local changes that you made to the abstractions files present on your system. This includes deleted abstractions. You'll want to select the "install the package maintainer's version" option for the freedesktop.org abstraction. Tyler > May 10 11:04:21 kftdv-P5Q-VM kernel: [13066.051541] audit: type=1400 > audit(1494407061.533:80): apparmor="STATUS" operation="profile_replace" > profile="unconfined" > name="/usr/lib/lightdm/lightdm-guest-session//chromium" pid=8942 > comm="apparmor_parser" > > regards Klaus Frick > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] restrictions on profile names
On 04/01/2017 10:51 PM, John Johansen wrote:
> There has been work upstream to bring generic LSM stacking to the
> Linux kernel. If this happens it will require changes to apparmor,
> specifically around the proc//attr interfaces that apparmor
> shares with other lsms. Currently only a single LSM can use these
> interfaces, however with full lsm stacking there will need to be some
> form of sharing. The current design has these interfaces being
> multiplexed by using lsm name tagging
>
> eg.
>
> > cat /proc/self/attr/current
> lsm1='v1'lsm2='v2'lsm3='v3'
>
> while multiplexing these interfaces is not the only possibility, any
> option will require changes to apparmor, and I would like to propose
> we begin making changes that will ensure apparmor is ready no matter
> what solution is chosen.
What are the chances that the stacking patches will be merged soon?
They've been discussed for years. I'd hate for us to make interface
changes for something that still isn't close to being merged.
> AppArmor's problem with these interfaces
> comes down to it allows arbitrary characters to be read and written,
> making it impossible for the generic infrastructure to parse out what
> is/should be apparmor's. The problem can be split into two distinct
> parts, what apparmor returns when the interface is read and what
> apparmor allows/requires be written to the interface.
>
> The read problem is basically what characters are allowed in the
> profiles name. Currently apparmor allows any character except \000 as
> part of a profile name that begins with / (any valid file path), other
> profile names are more restricted but the exact restrictions are
> inconsistent and have never been formalized. Arbitrary characters for
> a profile name are problematic as 3rd partly applications and generic
> infrastructure must be able to cope and display the label (eg. ps,
> pstree, ...). These applications do not handle the apparmor label well
> when there are control characters etc, embedded. Nor does it make it
> easy for a generic infrastructure to be able to tease apart the
> apparmor label from other lsm labels.
>
> So I propose we standardize on a set of printable characters for
> profile names. This will mean that profiles should be defined with a
> name separate from their attachment, ie.
>
> profile foo /usr/bin/bar { ... }
I agree that this is a good change. I've always been uneasy about how
the profile names are used throughout a number of userspace projects
even though they're not guaranteed to be printable.
> but even in cases where this is not the case we can have the parser
> convert the name to a valid set using a well defined
> transform. Somewhat like the standard we already employ for profile
> files in /etc/apparmor.d/. However even with such support we should
> encourage the movement to none attachment based profile names, as they
> are easier to write policy around and shorter (which is beneficial
> when stacking is used).
What's the justification for performing a transform? Why couldn't we
simply not allow non-printable names and let the profile author handle
the transform in a way that's known and predictable to them?
> The write issue is harder, in that \000 is allowed, even required, and
> we use the size of the write to determine how much input their really
> is. This is required to continue to allow interfaces like change_hatv
> to continue to work, so instead of restricting the write input I
> propose we move the writes to a custom apparmor interface. For most
> applications the details can be hidden behind libapparmor. Basically
> we would provide
>
> apparmor/current
> apparmor/exec
> apparmor/prev
>
> files in apparmorfs. Since writes are already restricted to be done
> only by the current task, we don't loose anything by not writing to
> the tasks attr. Once this interface is up, and the library is using
> it, we can deprecate writing to the proc interface in apparmor,
> defaulting to reject writes, but providing a sysctl to allow them. In
> this way we can find and migrate direct users of the current interface
> to the new one without breaking them.
IMO, this is the most disruptive change discussed in this email and I
think we need to think carefully about it. Is aa_change_hatv() the only
thing affected? If so, I think I'd prefer libapparmor's aa_change_hatv()
implementation to be updated to iterate over the list of hats and
effectively do an aa_change_hat() on each one of them instead of handing
the raw byte sequence, which contains NUL's, over to the kernel.
Tyler
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] Ignore test failures about duplicated conditionals in dbus rules
On 04/20/2017 02:23 PM, Tyler Hicks wrote: > On 04/15/2017 05:54 PM, Christian Boltz wrote: >> Am Samstag, 25. März 2017, 21:53:21 CEST schrieb Christian Boltz: >>> since r3634, the tools allow any order of dbus conditionals. >>> >>> Quoting the r3634 patch description: >>> >>> This patch eases the restriction on the ordering at the expense of >>> the utils no longer being able to detect and reject a single >>> attribute that is repeated multiple times. In that situation, only >>> the last occurrence of the attribute will be honored by the utils. >>> >>> It seems nobody tested with all test profiles generated ;-) so we have >>> to add some exceptions to the "does not raise an exception" list now. > > There was a typo in my build/test script that caused these tests to be > missed. I've just fixed my script. > > Is there a good reason for the utils `make check` target to pass even if > all the parser test profiles haven't been generated? If there's no good > reason, could you update the tests to fail in that situation? A make > variable could be added to indicate this this condition should be > ignored so that you can quickly iterate on utils changes without needing > all of the generated profiles. Hi - I thought I'd ping you again about this so that it isn't forgotten. Thanks! Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 2/2] libapparmor: Don't print shell commands that check for test failures
On 04/20/2017 02:28 PM, Tyler Hicks wrote: > Error messages shouldn't show up in build logs when the error has been > encountered. This patch silences these shell commands from being printed > before they're interpreted. Typo in the first sentence above. Changed locally to: "Error messages should only show up in build logs when the error has been encountered." Tyler > > Signed-off-by: Tyler Hicks > --- > libraries/libapparmor/testsuite/Makefile.am | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libraries/libapparmor/testsuite/Makefile.am > b/libraries/libapparmor/testsuite/Makefile.am > index 2cde5cc..55dcb68 100644 > --- a/libraries/libapparmor/testsuite/Makefile.am > +++ b/libraries/libapparmor/testsuite/Makefile.am > @@ -18,7 +18,7 @@ clean-local: > rm -f libaalogparse.log libaalogparse.sum > > check-local: check-DEJAGNU > - if ! test -f libaalogparse.log ; then echo '*** libaalogparse.log not > found - is dejagnu installed? ***'; exit 1; fi > - if grep ERROR libaalogparse.log ; then exit 1 ; fi > + @if ! test -f libaalogparse.log ; then echo '*** libaalogparse.log not > found - is dejagnu installed? ***'; exit 1; fi > + @if grep ERROR libaalogparse.log ; then exit 1 ; fi > > EXTRA_DIST = test_multi/*.in test_multi/*.out test_multi/*.err > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 1/2] libapparmor: Fix parallel make dependency issue in testsuite
A multi job `make check` command could fail due to check-local running before the check-DEJAGNU target, which is automatically generated by automake, would complete. This would result in a build failure due to libaalogparse.log not yet existing. Fix the issue by depending on the check-DEJAGNU target. Signed-off-by: Tyler Hicks --- I'm nominating this patch for 2.11 and trunk. libraries/libapparmor/testsuite/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/libapparmor/testsuite/Makefile.am b/libraries/libapparmor/testsuite/Makefile.am index 8f43cec..2cde5cc 100644 --- a/libraries/libapparmor/testsuite/Makefile.am +++ b/libraries/libapparmor/testsuite/Makefile.am @@ -17,7 +17,7 @@ clean-local: rm -rf tmp.err.* tmp.out.* site.exp site.bak test_multi/out rm -f libaalogparse.log libaalogparse.sum -check-local: +check-local: check-DEJAGNU if ! test -f libaalogparse.log ; then echo '*** libaalogparse.log not found - is dejagnu installed? ***'; exit 1; fi if grep ERROR libaalogparse.log ; then exit 1 ; fi -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 2/2] libapparmor: Don't print shell commands that check for test failures
Error messages shouldn't show up in build logs when the error has been encountered. This patch silences these shell commands from being printed before they're interpreted. Signed-off-by: Tyler Hicks --- libraries/libapparmor/testsuite/Makefile.am | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/libapparmor/testsuite/Makefile.am b/libraries/libapparmor/testsuite/Makefile.am index 2cde5cc..55dcb68 100644 --- a/libraries/libapparmor/testsuite/Makefile.am +++ b/libraries/libapparmor/testsuite/Makefile.am @@ -18,7 +18,7 @@ clean-local: rm -f libaalogparse.log libaalogparse.sum check-local: check-DEJAGNU - if ! test -f libaalogparse.log ; then echo '*** libaalogparse.log not found - is dejagnu installed? ***'; exit 1; fi - if grep ERROR libaalogparse.log ; then exit 1 ; fi + @if ! test -f libaalogparse.log ; then echo '*** libaalogparse.log not found - is dejagnu installed? ***'; exit 1; fi + @if grep ERROR libaalogparse.log ; then exit 1 ; fi EXTRA_DIST = test_multi/*.in test_multi/*.out test_multi/*.err -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] Ignore test failures about duplicated conditionals in dbus rules
On 04/15/2017 05:54 PM, Christian Boltz wrote: > Hello, > > Any comments or reviews on this patch? > > If nobody objects, I'll commit it (to trunk and 2.11) on Wednesday as > Acked-by . I see that the review period timed out already. That's fine by me as the change looks correct. Sorry that nobody could do a review in time. > Am Samstag, 25. März 2017, 21:53:21 CEST schrieb Christian Boltz: >> since r3634, the tools allow any order of dbus conditionals. >> >> Quoting the r3634 patch description: >> >> This patch eases the restriction on the ordering at the expense of >> the utils no longer being able to detect and reject a single >> attribute that is repeated multiple times. In that situation, only >> the last occurrence of the attribute will be honored by the utils. >> >> It seems nobody tested with all test profiles generated ;-) so we have >> to add some exceptions to the "does not raise an exception" list now. There was a typo in my build/test script that caused these tests to be missed. I've just fixed my script. Is there a good reason for the utils `make check` target to pass even if all the parser test profiles haven't been generated? If there's no good reason, could you update the tests to fail in that situation? A make variable could be added to indicate this this condition should be ignored so that you can quickly iterate on utils changes without needing all of the generated profiles. Tyler >> >> >> [ 01-parser-tests-dbus-duplicated-conditionals.diff ] >> >> === modified file 'utils/test/test-parser-simple-tests.py' >> --- utils/test/test-parser-simple-tests.py 2017-03-03 12:14:03 >> + +++ utils/test/test-parser-simple-tests.py 2017-03-25 >> 20:45:42 + @@ -49,6 +49,15 @@ >> 'change_profile/onx_conflict_unsafe1.sd', >> 'change_profile/onx_conflict_unsafe2.sd', >> >> +# duplicated conditionals aren't detected by the tools >> +'generated_dbus/duplicated-conditionals-45127.sd', >> +'generated_dbus/duplicated-conditionals-45131.sd', >> +'generated_dbus/duplicated-conditionals-45124.sd', >> +'generated_dbus/duplicated-conditionals-45130.sd', >> +'generated_dbus/duplicated-conditionals-45125.sd', >> +'generated_dbus/duplicated-conditionals-45128.sd', >> +'generated_dbus/duplicated-conditionals-45129.sd', >> + >> 'dbus/bad_modifier_2.sd', >> 'dbus/bad_regex_01.sd', >> 'dbus/bad_regex_02.sd', > > > > Regards, > > Christian Boltz > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] aa-notify: update to use 'normal' urgency to accommodate gnome-shell
On 04/11/2017 12:31 PM, Jamie Strandboge wrote: > > aa-notify currently calls notify-send with urgency of 'critical'. In > gnome-shell > critical urgency notifications result in a notification that must be explictly > clicked to dismiss (ie, they don't time out) and gnome-shell does not honor -- > expire-time with (at least) critical urgency. In other popular DEs critical > urgency notifications time out. This patch updates the urgency to 'normal' to > obtain intended behavior across DEs. > > Signed-off-by: Jamie Strandboge The libnotify documentation is of no help in determining what should be normal and what should be critical: https://developer.gnome.org/libnotify/0.7/NotifyNotification.html#NotifyUrgency I guess that means that we need to set the urgency according to how the popular DEs handle these notifications. Acked-by: Tyler Hicks Tyler > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch v3] tests: readdir - test both getdents() and getdents64() if available
On 04/05/2017 06:48 PM, Steve Beattie wrote:
> On Wed, Apr 05, 2017 at 04:09:15PM -0500, Tyler Hicks wrote:
>>> +#if defined(SYS_getdents) && defined(SYS_getdents64)
>>> + if (rc != rc64) {
>>> + printf("FAIL - getdents and getdents64 returned different
>>> values: %d vs %d\n", rc, rc64);
>>
>> I feel like this should be ERROR instead of FAIL. If we use FAIL here,
>> the test will "pass" in an expected fail test case when getdents()
>> returns something different than getdents64(). I don't see a way around
>> that other than printing a different (unexpected) output than FAIL.
>>
>> swap.sh looks to be the only other test using ERROR. prologue.inc
>> doesn't know about ERROR and handles it by calling testerror().
>>
>> We're getting into extreme corner case territory here. As before, I'm
>> prepared to declare "good enough". Let me know your thoughts.
>
> Hrm, yeah, you're right on that. Though, in the read access not
> permitted case, we should be getting rejected on the open() call,
> not the getdents()/getdents64() calls.
>
> Anyway, we're probably into over-engineered testcase area here, but what
> follows is v3 of the patch.
>
> Changes since v2:
>
> - Convert to passing the expected return value to the readdir test, and
>only fail if something unexpected is generated.
> - Change the readdir.sh test to pass the expected return values,
>and expect a PASS result for all tests.
> - Add a testcase that confirms that granting write access to a
>directory does not allow reading the directory's contents.
> - Add a debug print function to readdir.c and use it in a few cases
>when DEBUG is defined at compile time.
>
> Subject: tests: readdir - test both getdents() and getdents64() if available
>
> In commit 3649, Colin King fixed the readdir test build issue where
> aarch64 only supports getdetns64(), not getdents(). Realistically,
> however, we want to ensure mediation occurs on both syscalls where
> they exist. This patch changes the test to attempt performing both
> versions of getdents(). Because we want to catch the situation where
> the result of getdents differs from getdents64, we now pass in the
> expected result.
>
> Also add a test to verify that having write access does not grant
> the ability to read a directory's contents.
>
> Bug: https://bugs.launchpad.net/bugs/1674245
>
> Signed-off-by: Steve Beattie
Acked-by: Tyler Hicks
Thanks for iterating on this so much. getdents/getdents64 mediation has
never been tested so good.
Tyler
> ---
> tests/regression/apparmor/readdir.c | 129
> ---
> tests/regression/apparmor/readdir.sh | 21 -
> 2 files changed, 120 insertions(+), 30 deletions(-)
>
> Index: b/tests/regression/apparmor/readdir.c
> ===
> --- a/tests/regression/apparmor/readdir.c
> +++ b/tests/regression/apparmor/readdir.c
> @@ -1,14 +1,20 @@
> /*
> * Copyright (C) 2002-2006 Novell/SUSE
> + * Copyright (C) 2017 Canonical, Ltd.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> * published by the Free Software Foundation, version 2 of the
> * License.
> + *
> + * We attempt to test both getdents() and getdents64() here, but
> + * some architectures like aarch64 only support getdents64().
> */
> #define _GNU_SOURCE
>
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -18,45 +24,118 @@
> #include
> #include
>
> -int main(int argc, char *argv[])
> +/* error if neither SYS_getdents or SYS_getdents64 is defined */
> +#if !defined(SYS_getdents) && !defined(SYS_getdents64)
> + #error "Neither SYS_getdents or SYS_getdents64 is defined, something has
> gone wrong!"
> +#endif
> +
> +/* define DEBUG to enable debugging statements i.e. gcc -DDEBUG */
> +#ifdef DEBUG
> +void pdebug(const char *format, ...)
> {
> - int fd;
> -#if defined(SYS_getdents64)
> - struct dirent64 dir;
> + va_list arg;
> +
> + va_start(arg, format);
> + vprintf(format, arg);
> + va_end(arg);
> +}
> #else
> - struct dirent dir;
> +void pdebug(const char *format, ...) { return; }
> #endif
>
> - if (argc != 2){
> - fprintf(stderr, "usage: %s dir\n",
> - argv[0]);
> - return 1;
> +#ifdef SYS_getdents
> +int my_readdir(char *di
Re: [apparmor] [patch v2] tests: readdir - test both getdents() and getdents64() if available
On 04/05/2017 01:57 PM, Steve Beattie wrote:
> On Tue, Apr 04, 2017 at 03:41:41PM -0500, Tyler Hicks wrote:
>> I didn't mean to make this simple test improvement turn into something
>> complex. I'm willing to ack your original patch if you don't see a quick
>> and easy solution to this issue.
>
> Nah, let's do it right. V2 of the patch follows. Changes since v1:
>
> - compile error if neither SYS_getdents or SYS_getdents is defined
> - verify that getdents() and getdents64() return the same value if both
>are available.
> - propagate errno from getdents/getdents64 if failure occurs, instead
>of synthetic error value.
>
> (For reviewing, because so much changes, it might just be easier to
> review readdir.c after applying, rather than the diff itself.)
>
> Subject: tests: readdir - test both getdents() and getdents64() if available
>
> In commit 3649, Colin King fixed the readdir test build issue where
> aarch64 only supports getdetns64(), not getdents(). Realistically,
> however, we want to ensure mediation occurs on both syscalls where
> they exist. This patch changes the test to attempt performing both
> versions of getdents(). Also, if both versions exist, return a
> failure if the result of each differs from the other.
>
> Bug: https://bugs.launchpad.net/bugs/1674245
>
> Signed-off-by: Steve Beattie
> ---
> tests/regression/apparmor/readdir.c | 101
> +++-
> 1 file changed, 76 insertions(+), 25 deletions(-)
>
> Index: b/tests/regression/apparmor/readdir.c
> ===
> --- a/tests/regression/apparmor/readdir.c
> +++ b/tests/regression/apparmor/readdir.c
> @@ -1,10 +1,14 @@
> /*
> * Copyright (C) 2002-2006 Novell/SUSE
> + * Copyright (C) 2017 Canonical, Ltd.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> * published by the Free Software Foundation, version 2 of the
> * License.
> + *
> + * We attempt to test both getdents() and getdents64() here, but
> + * some architectures like aarch64 only support getdents64().
> */
> #define _GNU_SOURCE
>
> @@ -18,45 +22,92 @@
> #include
> #include
>
> -int main(int argc, char *argv[])
> +/* error if neither SYS_getdents or SYS_getdents64 is defined */
> +#if !defined(SYS_getdents) && !defined(SYS_getdents64)
> + #error "Neither SYS_getdents or SYS_getdents64 is defined, something has
> gone wrong!"
> +#endif
> +
> +#ifdef SYS_getdents
> +int my_readdir(char *dirname)
> {
> int fd;
> -#if defined(SYS_getdents64)
> - struct dirent64 dir;
> -#else
> struct dirent dir;
> -#endif
>
> - if (argc != 2){
> - fprintf(stderr, "usage: %s dir\n",
> - argv[0]);
> - return 1;
> + fd = open(dirname, O_RDONLY, 0);
> + if (fd == -1) {
> + printf("FAIL - open %s\n", strerror(errno));
> + return errno;
> }
>
> - fd = open(argv[1], O_RDONLY, 0);
> - if (fd == -1){
> - printf("FAIL - open %s\n", strerror(errno));
> - return 1;
> + /* getdents isn't exported by glibc, so must use syscall() */
> + if (syscall(SYS_getdents, fd, &dir, sizeof(dir)) == -1){
> + printf("FAIL - getdents %s\n", strerror(errno));
> + close(fd);
> + return errno;
> }
>
> - /*
> - if (fchdir(fd) == -1){
> - printf("FAIL - fchdir %s\n", strerror(errno));
> - return 1;
> + close(fd);
> + return 0;
> +}
> +#endif
> +
> +#ifdef SYS_getdents64
> +int my_readdir64(char *dirname)
> +{
> + int fd;
> + struct dirent64 dir;
> +
> + fd = open(dirname, O_RDONLY, 0);
> + if (fd == -1) {
> + printf("FAIL - open %s\n", strerror(errno));
> + return errno;
> }
> - */
>
> /* getdents isn't exported by glibc, so must use syscall() */
> -#if defined(SYS_getdents64)
> - if (syscall(SYS_getdents64, fd, &dir, sizeof(struct dirent64)) == -1){
> -#else
> - if (syscall(SYS_getdents, fd, &dir, sizeof(struct dirent)) == -1){
> + if (syscall(SYS_getdents64, fd, &dir, sizeof(dir)) == -1){
> + printf("FAIL - getdents64 %s\n", strerror(errno));
> + close(fd);
> + return errno;
> + }
> +
> + close(fd);
> + return 0;
> +}
>
Re: [apparmor] [patch] tests: readdir - test both getdents() and getdents64() if available
On 04/04/2017 03:24 PM, Steve Beattie wrote: > Hey Tyler, > > On Tue, Apr 04, 2017 at 02:03:53PM -0500, Tyler Hicks wrote: >> On 04/04/2017 01:14 PM, Steve Beattie wrote: >>> -int main(int argc, char *argv[]) >>> +#ifdef SYS_getdents >>> +int my_readdir(char *dirname) >> >> Nitpick... >> >> Please use my_getdents() and my_getdents64() since these are getdents >> wrappers and not readdir wrappers. > > The reason for calling the function my_readdir() and not my_getdents() > is the same reason the whole test is called the same; they're emulating > the work that the libc readdir() function does. while ensuring that > glibc doesn't do anything underneath us that might invalidate our > assumptions about the test. But I'm not too chuffed one or the other > about the name. I didn't even think about the name of the test. :) Feel free to keep it my_readdir(). > >>> + rc = my_readdir(argv[1]); >>> + if (rc != 0) >>> + goto err; >>> + >>> + rc = my_readdir64(argv[1]); >>> + if (rc != 0) >>> + goto err; >> >> Objection... >> >> In an expected fail test case, getdents64() will not be tested because >> getdents() will have already failed. >> >> The test script should pass the expected behavior to the test program >> and the test program will need to verify that both syscalls match the >> expected behavior. >> >> Alternatively, a shortcut would be to test both syscalls and if the >> results are different, raise an ERROR instead of a FAIL. > > Yes, I thought about this a bit before submitting. The tricky situation > is cases like aarch64 (aka arm64) where the getdents() syscall doesn't > exist at all and the stub function is used instead. The test would > need to take this into account, perhaps by having the stub function > return -ENOSYS or something, to make the situation detectable. > > I'll think about this more. Riiight. Maybe it is easier to stick an ifdef or two in the calling function. If both are defined, expect equality in the return code. If only one is defined, don't do an return code equality check. I didn't mean to make this simple test improvement turn into something complex. I'm willing to ack your original patch if you don't see a quick and easy solution to this issue. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] tests: readdir - test both getdents() and getdents64() if available
On 04/04/2017 01:14 PM, Steve Beattie wrote:
> Hey Colin,
>
> On Tue, Apr 04, 2017 at 03:16:29PM -, Colin Ian King wrote:
>> Colin Ian King has proposed merging
>> lp:~colin-king/apparmor/fix-arm64-test-builds into lp:apparmor.
>>
>> Requested reviews:
>> AppArmor Developers (apparmor-dev)
>>
>> For more details, see:
>> https://code.launchpad.net/~colin-king/apparmor/fix-arm64-test-builds/+merge/321876
>>
>> This fixes build issues for the readdir test for arm64 where getdents(2)
>> is not wired up as a system call but gendents64(2) is available.
>> This changes the preference to use the 64 bit system call by default
>> if it is available on 64 bit systems.
>
> Thanks for providing this fix.
>
> Realastically, though, where an architecture supports both getdents()
> and getdents64(), we ought to be ensuring that apparmor mediation occurs
> on both syscall paths. What follows is a patch that attempts to do that.
Smart thinking. I like the idea but have one nitpick and one objection
below...
>
> Subject: tests: readdir - test both getdents() and getdents64() if available
>
> In commit 3649, Colin King fixed the readdir test build issue where aarch64
> only
> supports getdetns64(), not getdents(). Realistically, however, we want
> to ensure mediation occurs on both syscalls where they exist. This patch
> changes the test to attempt performing both versions of getdents().
>
> Bug: https://bugs.launchpad.net/bugs/1674245
>
> Signed-off-by: Steve Beattie
> ---
> tests/regression/apparmor/readdir.c | 80
> ++--
> 1 file changed, 59 insertions(+), 21 deletions(-)
>
> Index: b/tests/regression/apparmor/readdir.c
> ===
> --- a/tests/regression/apparmor/readdir.c
> +++ b/tests/regression/apparmor/readdir.c
> @@ -1,10 +1,14 @@
> /*
> * Copyright (C) 2002-2006 Novell/SUSE
> + * Copyright (C) 2017 Canonical, Ltd.
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> * published by the Free Software Foundation, version 2 of the
> * License.
> + *
> + * We attempt to test both getdents() and getdents64() here, but
> + * some architectures like aarch64 only support getdents64().
> */
> #define _GNU_SOURCE
>
> @@ -18,45 +22,79 @@
> #include
> #include
>
> -int main(int argc, char *argv[])
> +#ifdef SYS_getdents
> +int my_readdir(char *dirname)
Nitpick...
Please use my_getdents() and my_getdents64() since these are getdents
wrappers and not readdir wrappers.
> {
> int fd;
> -#if defined(SYS_getdents64)
> - struct dirent64 dir;
> -#else
> struct dirent dir;
> -#endif
>
> - if (argc != 2){
> - fprintf(stderr, "usage: %s dir\n",
> - argv[0]);
> + fd = open(dirname, O_RDONLY, 0);
> + if (fd == -1) {
> + printf("FAIL - open %s\n", strerror(errno));
> return 1;
> }
>
> - fd = open(argv[1], O_RDONLY, 0);
> - if (fd == -1){
> - printf("FAIL - open %s\n", strerror(errno));
> + /* getdents isn't exported by glibc, so must use syscall() */
> + if (syscall(SYS_getdents, fd, &dir, sizeof(dir)) == -1){
> + printf("FAIL - getdents %s\n", strerror(errno));
> + close(fd);
> return 1;
> }
>
> - /*
> - if (fchdir(fd) == -1){
> - printf("FAIL - fchdir %s\n", strerror(errno));
> + close(fd);
> + return 0;
> +}
> +#else
> +int my_readdir(char *dirname) { return 0; }
> +#endif
> +
> +#ifdef SYS_getdents64
> +int my_readdir64(char *dirname)
> +{
> + int fd;
> + struct dirent64 dir;
> +
> + fd = open(dirname, O_RDONLY, 0);
> + if (fd == -1) {
> + printf("FAIL - open %s\n", strerror(errno));
> return 1;
> }
> - */
>
> /* getdents isn't exported by glibc, so must use syscall() */
> -#if defined(SYS_getdents64)
> - if (syscall(SYS_getdents64, fd, &dir, sizeof(struct dirent64)) == -1){
> + if (syscall(SYS_getdents64, fd, &dir, sizeof(dir)) == -1){
> + printf("FAIL - getdents64 %s\n", strerror(errno));
> + close(fd);
> + return 1;
> + }
> +
> + close(fd);
> + return 0;
> +}
> #else
> - if (syscall(SYS_getdents, fd, &dir, sizeof(struct dirent)) == -1){
> +int my_readdir64(char *dirname) { return 0; }
> #endif
> - printf("FAIL - getdents %s\n", strerror(errno));
> - return 1;
> +
> +int main(int argc, char *argv[])
> +{
> + int rc = 0;
> +
> + if (argc != 2) {
> + fprintf(stderr, "usage: %s dir\n",
> + argv[0]);
> + rc = 1;
> + goto err;
> }
>
> + rc = my_readdir(argv[1]);
> + if (rc != 0)
> + goto err;
> +
> + rc = my_readdir64(argv[1]);
> + if (rc != 0)
> +
Re: [apparmor] [Merge] lp:~osomon/apparmor/newer-nvidia-abstraction into lp:~apparmor-dev/apparmor/apparmor-ubuntu-citrain
Review: Resubmit Hi Olivier - Thanks for the merge proposal. Since this change affects the upstream AppArmor project, can you resubmit against lp:apparmor? It will likely help to get a few more eyes on the merge proposal, as well. FYI, I have an upcoming apparmor bug fix upload for zesty and can include this patch once the upstream MP gets an ack. Thanks! -- https://code.launchpad.net/~osomon/apparmor/newer-nvidia-abstraction/+merge/319116 Your team AppArmor Developers is subscribed to branch lp:~apparmor-dev/apparmor/apparmor-ubuntu-citrain. -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] Fix regressions caused by init_aa()
On 03/02/2017 05:52 PM, Christian Boltz wrote:
> Hello,
>
> With the init_aa() patch series commited, minitools_test.py showed
> several test failures - which effectively means the -d option of
> aa-complain, aa-cleanprof etc. was broken.
>
> These failures were caused by
> - calling init_aa() too late in tools.py - _after_ setting the
> profiledir, which then got overwritten by init_aa()
> - calling init_aa() twice (because apparmor.aa gets imported in two
> modules used by aa-cleanprof), which overwrote the manually set values
> on the second run
>
> This patch fixes the call order in tools.py and adds a check to
> init_aa() so that it can be run only once and ignores additional calls.
>
Acked-by: Tyler Hicks
Thanks!
>
> [ 02-fix-init_aa-regressions.diff ]
>
> === modified file ./utils/apparmor/aa.py
> --- utils/apparmor/aa.py2017-03-03 00:10:55.506361000 +0100
> +++ utils/apparmor/aa.py2017-03-03 00:37:47.283693765 +0100
> @@ -3779,6 +3781,9 @@
> global extra_profile_dir
> global parser
>
> +if CONFDIR:
> +return # config already initialized (and possibly changed
> afterwards), so don't overwrite the config variables
> +
> CONFDIR = confdir
> conf = apparmor.config.Config('ini', CONFDIR)
> cfg = conf.read_config('logprof.conf')
> === modified file ./utils/apparmor/tools.py
> --- utils/apparmor/tools.py 2017-03-03 00:10:55.506361000 +0100
> +++ utils/apparmor/tools.py 2017-03-03 00:23:54.587304297 +0100
> @@ -24,6 +24,8 @@
>
> class aa_tools:
> def __init__(self, tool_name, args):
> +apparmor.init_aa()
> +
> self.name = tool_name
> self.profiledir = args.dir
> self.profiling = args.program
> @@ -31,8 +33,6 @@
> self.silent = None
> self.do_reload = args.do_reload
>
> -apparmor.init_aa()
> -
> if tool_name in ['audit']:
> self.remove = args.remove
> elif tool_name == 'autodep':
>
>
>
> Regards,
>
> Christian Boltz
>
>
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH v2 3/8] utils: Require apparmor.aa users to call init_aa()
On 03/02/2017 01:32 PM, Christian Boltz wrote: > Hello, > > Am Mittwoch, 1. März 2017, 21:52:01 CET schrieb Tyler Hicks: >> Introduce an apparmor.aa.init_aa() method and move the initialization >> code of the apparmor.aa module into it. Note that this change will >> break any external users of apparmor.aa because global variables that >> were previously initialized when importing apparmor.aa will not be >> initialized unless a call to the new apparmor.aa.init_aa() method is >> made. > > >> diff --git a/utils/aa-mergeprof b/utils/aa-mergeprof >> index 4e1e633..1241515 100755 >> --- a/utils/aa-mergeprof >> +++ b/utils/aa-mergeprof >> @@ -43,6 +43,8 @@ args = parser.parse_args() >> >> args.other = None >> >> +apparmor.aa.init_aa() >> + >> profiles = args.files >> >> profiledir = args.dir >> @@ -136,6 +138,7 @@ class Merge(object): >> user, base = profiles >> >> #Read and parse base profile and save profile data, include >> data from it and reset them >> +apparmor.aa.init_aa() >> apparmor.aa.read_profile(base, True) >> self.base = cleanprofile.Prof(base) > > Just curious - what's the reason for calling init_aa() a second time? > I didn't test, but I'd guess that doing it in the global code should be > enough. Good catch. I meant to circle back to aa-mergeprof and make a final decision on whether it was best to do it in the global code or in the class constructor. I agree that doing it in the global code is sufficient. I'm going to drop the call from the constructor. > > >> --- a/utils/test/Makefile >> +++ b/utils/test/Makefile > >> check: __libapparmor >> -export PYTHONPATH=$(PYTHONPATH) ; export >> LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) ; export LC_ALL=C; $(foreach test, >> $(wildcard test-*.py), echo ; echo === $(test) === ; $(call pyalldo, >> $(test))) >> +export PYTHONPATH=$(PYTHONPATH) >> LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) LC_ALL=C __AA_CONFDIR=$(CONFDIR) ; >> $(foreach test, $(wildcard test-*.py), echo ; echo === $(test) === ; >> $(call pyalldo, $(test))) > > I remember discussions about line lenghts in python. Did we already have > such a discussion about Makefiles? ;-) Hmm? I don't recall what you're referring to. Are you wanting me to wrap the lines that I modified? I strongly prefer to follow existing conventions when making feature changes or bug fixes and leave coding style cleanups to separate patches. > (I know changing this in this patch would break the following patches, > so if you want shorter lines, feel free to send a follow-up patch.) FYI, this sort of thing isn't a problem with git rebase (I use git-remote-bzr for apparmor devel). > > Both questions shouldn't stop you from commiting, so > Acked-by: Christian Boltz Thanks! Tyler > > > Regards, > > Christian Boltz > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH v2 8/8] utils: Fix apparmor.easyprof import in test-aa-easyprof.py
On 03/01/2017 04:11 PM, Seth Arnold wrote: > On Wed, Mar 01, 2017 at 08:52:06PM +0000, Tyler Hicks wrote: >> The test-aa-easyprof.py script was attempting to do its own special >> setup to import the in-tree easyprof module. However, this proved to be >> very flaky and resulted in the test periodically failing due to an >> AttributeError the first time easyprof.parse_args() was called. >> >> This patch removes the flakiness by trusting that PYTHONPATH is set up >> appropriately before the test script is ran. PYTHONPATH is already >> initialized appropriately by utils/test/Makefile according to the >> USE_SYSTEM make variable. >> >> Signed-off-by: Tyler Hicks >> Cc: Christian Boltz > > Acked-by: Seth Arnold > > This feels so much less brittle. :) Agreed! I wish I understood why the old code intermittently failed but this change is such a no-brainer that I didn't think a deeper investigation was worth it. Thanks for the review! Tyler > > Thanks > >> --- >> utils/test/test-aa-easyprof.py | 26 ++ >> 1 file changed, 2 insertions(+), 24 deletions(-) >> >> diff --git a/utils/test/test-aa-easyprof.py b/utils/test/test-aa-easyprof.py >> index e7916de..ff69c42 100755 >> --- a/utils/test/test-aa-easyprof.py >> +++ b/utils/test/test-aa-easyprof.py >> @@ -18,6 +18,8 @@ import sys >> import tempfile >> import unittest >> >> +import apparmor.easyprof as easyprof >> + >> topdir = None >> debugging = False >> >> @@ -2673,40 +2675,16 @@ POLICYGROUPS_DIR="%s/templates" >> # Main >> # >> if __name__ == '__main__': >> -def cleanup(files): >> -for f in files: >> -if os.path.exists(f): >> -os.unlink(f) >> - >> absfn = os.path.abspath(sys.argv[0]) >> topdir = os.path.dirname(os.path.dirname(absfn)) >> >> if len(sys.argv) > 1 and (sys.argv[1] == '-d' or sys.argv[1] == >> '--debug'): >> debugging = True >> >> -created = [] >> - >> -# Create the necessary files to import aa-easyprof >> -init = os.path.join(os.path.dirname(absfn), '__init__.py') >> -if not os.path.exists(init): >> -open(init, 'a').close() >> -created.append(init) >> - >> -symlink = os.path.join(os.path.dirname(absfn), 'easyprof.py') >> -if not os.path.exists(symlink): >> -os.symlink(os.path.join(topdir, 'apparmor', 'easyprof.py'), symlink) >> -created.append(symlink) >> -created.append(symlink + 'c') >> - >> -# Now that we have everything we need, import aa-easyprof >> -import easyprof >> - >> # run the tests >> suite = unittest.TestSuite() >> suite.addTest(unittest.TestLoader().loadTestsFromTestCase(T)) >> rc = unittest.TextTestRunner(verbosity=2).run(suite) >> >> -cleanup(created) >> - >> if not rc.wasSuccessful(): >> sys.exit(1) >> >> -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 8/8] utils: Fix apparmor.easyprof import in test-aa-easyprof.py
The test-aa-easyprof.py script was attempting to do its own special setup to import the in-tree easyprof module. However, this proved to be very flaky and resulted in the test periodically failing due to an AttributeError the first time easyprof.parse_args() was called. This patch removes the flakiness by trusting that PYTHONPATH is set up appropriately before the test script is ran. PYTHONPATH is already initialized appropriately by utils/test/Makefile according to the USE_SYSTEM make variable. Signed-off-by: Tyler Hicks Cc: Christian Boltz --- utils/test/test-aa-easyprof.py | 26 ++ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/utils/test/test-aa-easyprof.py b/utils/test/test-aa-easyprof.py index e7916de..ff69c42 100755 --- a/utils/test/test-aa-easyprof.py +++ b/utils/test/test-aa-easyprof.py @@ -18,6 +18,8 @@ import sys import tempfile import unittest +import apparmor.easyprof as easyprof + topdir = None debugging = False @@ -2673,40 +2675,16 @@ POLICYGROUPS_DIR="%s/templates" # Main # if __name__ == '__main__': -def cleanup(files): -for f in files: -if os.path.exists(f): -os.unlink(f) - absfn = os.path.abspath(sys.argv[0]) topdir = os.path.dirname(os.path.dirname(absfn)) if len(sys.argv) > 1 and (sys.argv[1] == '-d' or sys.argv[1] == '--debug'): debugging = True -created = [] - -# Create the necessary files to import aa-easyprof -init = os.path.join(os.path.dirname(absfn), '__init__.py') -if not os.path.exists(init): -open(init, 'a').close() -created.append(init) - -symlink = os.path.join(os.path.dirname(absfn), 'easyprof.py') -if not os.path.exists(symlink): -os.symlink(os.path.join(topdir, 'apparmor', 'easyprof.py'), symlink) -created.append(symlink) -created.append(symlink + 'c') - -# Now that we have everything we need, import aa-easyprof -import easyprof - # run the tests suite = unittest.TestSuite() suite.addTest(unittest.TestLoader().loadTestsFromTestCase(T)) rc = unittest.TextTestRunner(verbosity=2).run(suite) -cleanup(created) - if not rc.wasSuccessful(): sys.exit(1) -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 5/8] utils: Set parser base path according to USE_SYSTEM make variable
If USE_SYSTEM is not set, the utils make check target will instruct
test-aa-easyprof.py to provide the path of the in-tree
profiles/apparmor.d directory to aa-easyprof as the parser base
directory.
If USE_SYSTEM is set, the default base directory (/etc/apparmor.d) is
used.
The test-aa-easyprof.py script receives the base path by checking the
__AA_BASEDIR environment variable. This environment variable is strictly
used by the test script and not any user-facing code so two leading
underscores were used.
Signed-off-by: Tyler Hicks
Acked-by: Christian Boltz
Acked-by: Seth Arnold
---
utils/test/Makefile| 6 --
utils/test/test-aa-easyprof.py | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/utils/test/Makefile b/utils/test/Makefile
index 025bba4..ce08cbb 100644
--- a/utils/test/Makefile
+++ b/utils/test/Makefile
@@ -24,12 +24,14 @@ ifdef USE_SYSTEM
LD_LIBRARY_PATH=
PYTHONPATH=
CONFDIR=
+BASEDIR=
else
# PYTHON_DIST_BUILD_PATH based on libapparmor/swig/python/test/Makefile.am
PYTHON_DIST_BUILD_PATH =
../../libraries/libapparmor/swig/python/build/$$($(PYTHON) -c "import
distutils.util; import platform; print(\"lib.%s-%s\"
%(distutils.util.get_platform(), platform.python_version()[:3]))")
LD_LIBRARY_PATH=../../libraries/libapparmor/src/.libs/
PYTHONPATH=..:$(PYTHON_DIST_BUILD_PATH)
CONFDIR=$(CURDIR)
+BASEDIR=../../profiles/apparmor.d
endif
.PHONY: __libapparmor
@@ -64,10 +66,10 @@ clean:
rm -rf __pycache__/ .coverage htmlcov
check: __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) ; $(foreach test, $(wildcard test-*.py), echo
; echo === $(test) === ; $(call pyalldo, $(test)))
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ; $(foreach test,
$(wildcard test-*.py), echo ; echo === $(test) === ; $(call pyalldo, $(test)))
.coverage: $(wildcard ../aa-* ../apparmor/*.py test-*.py) __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) ; $(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach
test, $(wildcard test-*.py), echo ; echo === $(test) === ; $(PYTHON) -m
coverage run --branch -p $(test); )
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ;
$(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test, $(wildcard test-*.py), echo ;
echo === $(test) === ; $(PYTHON) -m coverage run --branch -p $(test); )
$(PYTHON) -m coverage combine
coverage: .coverage
diff --git a/utils/test/test-aa-easyprof.py b/utils/test/test-aa-easyprof.py
index 3ebfec6..8f7e916 100755
--- a/utils/test/test-aa-easyprof.py
+++ b/utils/test/test-aa-easyprof.py
@@ -163,6 +163,16 @@ TEMPLATES_DIR="%s/templates"
self.binary = "/opt/bin/foo"
self.full_args = ['-c', self.conffile, self.binary]
+# Check __AA_BASEDIR, which may be set by the Makefile, to see if
+# we should use a non-default base directory path to find
+# abstraction files
+#
+# NOTE: Individual tests can append another --base path to the
+# args list and override a base path set here
+base = os.getenv('__AA_BASEDIR')
+if base:
+self.full_args.append('--base=%s' % base)
+
if debugging:
self.full_args.append('-d')
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 6/8] utils: Add option to aa-easyprof to specify the apparmor_parser path
When testing against a clean system without the apparmor_parser binary
installed, the test-aa-easyprof.py script ends up skipping profile
verification because it can't find the parser binary. This even causes a
test failure due to the test_genpolicy_invalid_template_policy test.
Adding a --parser option to aa-easyprof is the first step in addressing
this problem.
Signed-off-by: Tyler Hicks
Acked-by: Christian Boltz
Acked-by: Seth Arnold
---
utils/aa-easyprof.pod | 6 ++
utils/apparmor/easyprof.py | 25 +
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/utils/aa-easyprof.pod b/utils/aa-easyprof.pod
index 31687bf..56ef257 100644
--- a/utils/aa-easyprof.pod
+++ b/utils/aa-easyprof.pod
@@ -57,6 +57,12 @@ for supported policy groups. The available policy groups are
in
AppArmor rules or policies. They are similar to AppArmor abstractions, but
usually encompass more policy rules.
+=item --parser PATH
+
+Specify the PATH of the apparmor_parser binary to use when verifying
+policy. If this option is not specified, aa-easyprof will attempt to
+locate the path starting with /sbin/apparmor_parser.
+
=item -a ABSTRACTIONS, --abstractions=ABSTRACTIONS
Specify ABSTRACTIONS as a comma-separated list of AppArmor abstractions. It is
diff --git a/utils/apparmor/easyprof.py b/utils/apparmor/easyprof.py
index 01c7fd6..c6e6932 100644
--- a/utils/apparmor/easyprof.py
+++ b/utils/apparmor/easyprof.py
@@ -259,14 +259,11 @@ def open_file_read(path):
return orig
-def verify_policy(policy, base=None, include=None):
+def verify_policy(policy, exe, base=None, include=None):
'''Verify policy compiles'''
-exe = "/sbin/apparmor_parser"
-if not os.path.exists(exe):
-rc, exe = cmd(['which', 'apparmor_parser'])
-if rc != 0:
-warn("Could not find apparmor_parser. Skipping verify")
-return True
+if not exe:
+warn("Could not find apparmor_parser. Skipping verify")
+return True
fn = ""
# if policy starts with '/' and is one line, assume it is a path
@@ -309,6 +306,14 @@ class AppArmorEasyProfile:
if os.path.isfile(self.conffile):
self._get_defaults()
+self.parser_path = '/sbin/apparmor_parser'
+if opt.parser_path:
+self.parser_path = opt.parser_path
+elif not os.path.exists(self.parser_path):
+rc, self.parser_path = cmd(['which', 'apparmor_parser'])
+if rc != 0:
+self.parser_path = None
+
self.parser_base = "/etc/apparmor.d"
if opt.parser_base:
self.parser_base = opt.parser_base
@@ -680,7 +685,7 @@ class AppArmorEasyProfile:
if no_verify:
debug("Skipping policy verification")
-elif not verify_policy(policy, self.parser_base, self.parser_include):
+elif not verify_policy(policy, self.parser_path, self.parser_base,
self.parser_include):
msg("\n" + policy)
raise AppArmorException("Invalid policy")
@@ -823,6 +828,10 @@ def check_for_manifest_arg_append(option, opt_str, value,
parser):
def add_parser_policy_args(parser):
'''Add parser arguments'''
+parser.add_option("--parser",
+ dest="parser_path",
+ help="The path to the profile parser used for
verification",
+ metavar="PATH")
parser.add_option("-a", "--abstractions",
action="callback",
callback=check_for_manifest_arg,
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 3/8] utils: Require apparmor.aa users to call init_aa()
Introduce an apparmor.aa.init_aa() method and move the initialization
code of the apparmor.aa module into it. Note that this change will break
any external users of apparmor.aa because global variables that were
previously initialized when importing apparmor.aa will not be
initialized unless a call to the new apparmor.aa.init_aa() method is
made.
The main purpose of this change is to allow the utils tests to be able
to set a non-default location for configuration files. Instead of
hard-coding the location of logprof.conf and other utils related
configuration files to /etc/apparmor/, this patch allows it to be
configured by calling apparmor.aa.init_aa(confdir=PATH).
This allows for the make check target to use the in-tree config file,
profiles, and parser by default. A helper method, setup_aa(), is added
to common_test.py that checks for an environment variable containing a
non-default configuration directory path prior to calling
apparmor.aa.init_aa(). All test scripts that use apparmor.aa are updated
to call setup_aa().
Signed-off-by: Tyler Hicks
Suggested-by: Christian Boltz
---
utils/aa-genprof | 1 +
utils/aa-logprof | 1 +
utils/aa-mergeprof| 3 +++
utils/aa-unconfined | 1 +
utils/apparmor/aa.py | 43 +++
utils/apparmor/cleanprofile.py| 1 +
utils/apparmor/tools.py | 2 ++
utils/test/Makefile | 6 +++--
utils/test/common_test.py | 11
utils/test/minitools_test.py | 3 ++-
utils/test/test-aa.py | 3 ++-
utils/test/test-libapparmor-test_multi.py | 3 ++-
utils/test/test-mount_parse.py| 3 ++-
utils/test/test-parser-simple-tests.py| 3 ++-
utils/test/test-pivot_root_parse.py | 3 ++-
utils/test/test-regex_matches.py | 3 ++-
utils/test/test-unix_parse.py | 3 ++-
17 files changed, 66 insertions(+), 27 deletions(-)
diff --git a/utils/aa-genprof b/utils/aa-genprof
index 3fe72bb..e2e6544 100755
--- a/utils/aa-genprof
+++ b/utils/aa-genprof
@@ -66,6 +66,7 @@ args = parser.parse_args()
profiling = args.program
profiledir = args.dir
+apparmor.init_aa()
apparmor.set_logfile(args.file)
aa_mountpoint = apparmor.check_for_apparmor()
diff --git a/utils/aa-logprof b/utils/aa-logprof
index 05ebbd9..c05cbef 100755
--- a/utils/aa-logprof
+++ b/utils/aa-logprof
@@ -34,6 +34,7 @@ args = parser.parse_args()
profiledir = args.dir
logmark = args.mark or ''
+apparmor.init_aa()
apparmor.set_logfile(args.file)
aa_mountpoint = apparmor.check_for_apparmor()
diff --git a/utils/aa-mergeprof b/utils/aa-mergeprof
index 4e1e633..1241515 100755
--- a/utils/aa-mergeprof
+++ b/utils/aa-mergeprof
@@ -43,6 +43,8 @@ args = parser.parse_args()
args.other = None
+apparmor.aa.init_aa()
+
profiles = args.files
profiledir = args.dir
@@ -136,6 +138,7 @@ class Merge(object):
user, base = profiles
#Read and parse base profile and save profile data, include data from
it and reset them
+apparmor.aa.init_aa()
apparmor.aa.read_profile(base, True)
self.base = cleanprofile.Prof(base)
diff --git a/utils/aa-unconfined b/utils/aa-unconfined
index 69e0d65..0407395 100755
--- a/utils/aa-unconfined
+++ b/utils/aa-unconfined
@@ -40,6 +40,7 @@ args = parser.parse_args()
paranoid = args.paranoid
+aa.init_aa()
aa_mountpoint = aa.check_for_apparmor()
if not aa_mountpoint:
raise aa.AppArmorException(_("It seems AppArmor was not started. Please
enable AppArmor and try again."))
diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
index eecf8c7..1464a21 100644
--- a/utils/apparmor/aa.py
+++ b/utils/apparmor/aa.py
@@ -73,14 +73,14 @@ _ = init_translation()
# Setup logging incase of debugging is enabled
debug_logger = DebugLogger('aa')
-CONFDIR = '/etc/apparmor'
-
# The database for severity
sev_db = None
# The file to read log messages from
### Was our
logfile = None
+CONFDIR = None
+conf = None
cfg = None
repo_cfg = None
@@ -3741,24 +3741,33 @@ def logger_path():
##Initialisations##
-conf = apparmor.config.Config('ini', CONFDIR)
-cfg = conf.read_config('logprof.conf')
+def init_aa(confdir="/etc/apparmor"):
+global CONFDIR
+global conf
+global cfg
+global profile_dir
+global extra_profile_dir
+global parser
+
+CONFDIR = confdir
+conf = apparmor.config.Config('ini', CONFDIR)
+cfg = conf.read_config('logprof.conf')
-# prevent various failures if logprof.conf doesn't exist
-if not cfg.sections():
-cfg.add_section('settings')
-cfg.add_section('required_hats')
+# prevent various failures if logprof.conf doesn't exist
+if not cfg.sections():
+cfg.add_section(
[apparmor] [PATCH v2 2/8] utils: Update the logprof.conf in the test dir to point to in-tree paths
The utils tests should make use of the logprof.conf that resides in
utils/test/ when testing against the in-tree parser and profiles. When
testing against the system, it the utils tests should continue to use
the system logprof.conf.
This patch updates the parser and profiles paths to point to the in-tree
paths. Another patch is needed to get aa.py to honor a non-hardcoded
search path for logprof.conf and other configuration files.
Signed-off-by: Tyler Hicks
Acked-by: Christian Boltz
Acked-by: Seth Arnold
---
utils/test/logprof.conf | 6 +++---
utils/test/test-config.py | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/utils/test/logprof.conf b/utils/test/logprof.conf
index 47ad563..31ed5c9 100644
--- a/utils/test/logprof.conf
+++ b/utils/test/logprof.conf
@@ -10,11 +10,11 @@
# --
[settings]
- profiledir = /etc/apparmor.d /etc/subdomain.d
- inactive_profiledir = /usr/share/doc/apparmor-profiles/extras
+ profiledir = ../../profiles/apparmor.d
+ inactive_profiledir = ../../profiles/apparmor/profiles/extra
logfiles = /var/log/audit/audit.log /var/log/syslog /var/log/messages
- parser = /sbin/apparmor_parser /sbin/subdomain_parser
+ parser = ../../parser/apparmor_parser
ldd = /usr/bin/ldd
logger = /bin/logger /usr/bin/logger
diff --git a/utils/test/test-config.py b/utils/test/test-config.py
index 70bdfca..3468c3b 100755
--- a/utils/test/test-config.py
+++ b/utils/test/test-config.py
@@ -24,7 +24,7 @@ class Test(unittest.TestCase):
conf = ini_config.read_config('logprof.conf')
logprof_sections = ['settings', 'repository', 'qualifiers',
'required_hats', 'defaulthat', 'globs']
logprof_sections_options = ['profiledir', 'inactive_profiledir',
'logfiles', 'parser', 'ldd', 'logger', 'default_owner_prompt',
'custom_includes']
-logprof_settings_parser = '/sbin/apparmor_parser
/sbin/subdomain_parser'
+logprof_settings_parser = '../../parser/apparmor_parser'
self.assertEqual(conf.sections(), logprof_sections)
self.assertEqual(conf.options('settings'), logprof_sections_options)
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 1/8] utils: Improve error messages when profiles/parser is not found
When aa.py is imported, it looks for a set of profiles and it also looks
for the parser. Both of these paths are configured by logprof.conf but
it isn't always obvious which logprof.conf file was used and, therefore,
it isn't always obvious where aa.py is looking. This patch includes the
paths in the error messages.
Signed-off-by: Tyler Hicks
Acked-by: Christian Boltz
Acked-by: Seth Arnold
---
utils/apparmor/aa.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
index ab7f6c9..eecf8c7 100644
--- a/utils/apparmor/aa.py
+++ b/utils/apparmor/aa.py
@@ -3754,11 +3754,11 @@ if cfg['settings'].get('default_owner_prompt', False):
profile_dir = conf.find_first_dir(cfg['settings'].get('profiledir')) or
'/etc/apparmor.d'
if not os.path.isdir(profile_dir):
-raise AppArmorException('Can\'t find AppArmor profiles')
+raise AppArmorException('Can\'t find AppArmor profiles in %s' %
(profile_dir))
extra_profile_dir =
conf.find_first_dir(cfg['settings'].get('inactive_profiledir')) or
'/usr/share/apparmor/extra-profiles/'
parser = conf.find_first_file(cfg['settings'].get('parser')) or
'/sbin/apparmor_parser'
if not os.path.isfile(parser) or not os.access(parser, os.EX_OK):
-raise AppArmorException('Can\'t find apparmor_parser')
+raise AppArmorException('Can\'t find apparmor_parser at %s' % (parser))
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 7/8] utils: Set parser executable path according to USE_SYSTEM make variable
if USE_SYSTEM is not set, the utils make check target will instruct
test-aa-easyprof.py to provide the path of the in-tree parser executable
to aa-easyprof.
If USE_SYSTEM is set, the default parser path (/sbin/apparmor_parser or
the result of `which apparmor_parser`) is used.
The test-aa-easyprof.py script receives the parser path by checking the
__AA_PARSER environment variable. This environment variable is strictly
used by the test script and not any user-facing code so two leading
underscores were used.
Signed-off-by: Tyler Hicks
Acked-by: Christian Boltz
Acked-by: Seth Arnold
---
utils/test/Makefile| 6 --
utils/test/test-aa-easyprof.py | 7 +++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/utils/test/Makefile b/utils/test/Makefile
index ce08cbb..db2312e 100644
--- a/utils/test/Makefile
+++ b/utils/test/Makefile
@@ -25,6 +25,7 @@ ifdef USE_SYSTEM
PYTHONPATH=
CONFDIR=
BASEDIR=
+PARSER=
else
# PYTHON_DIST_BUILD_PATH based on libapparmor/swig/python/test/Makefile.am
PYTHON_DIST_BUILD_PATH =
../../libraries/libapparmor/swig/python/build/$$($(PYTHON) -c "import
distutils.util; import platform; print(\"lib.%s-%s\"
%(distutils.util.get_platform(), platform.python_version()[:3]))")
@@ -32,6 +33,7 @@ else
PYTHONPATH=..:$(PYTHON_DIST_BUILD_PATH)
CONFDIR=$(CURDIR)
BASEDIR=../../profiles/apparmor.d
+PARSER=../../parser/apparmor_parser
endif
.PHONY: __libapparmor
@@ -66,10 +68,10 @@ clean:
rm -rf __pycache__/ .coverage htmlcov
check: __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ; $(foreach test,
$(wildcard test-*.py), echo ; echo === $(test) === ; $(call pyalldo, $(test)))
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) __AA_PARSER=$(PARSER)
; $(foreach test, $(wildcard test-*.py), echo ; echo === $(test) === ; $(call
pyalldo, $(test)))
.coverage: $(wildcard ../aa-* ../apparmor/*.py test-*.py) __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ;
$(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test, $(wildcard test-*.py), echo ;
echo === $(test) === ; $(PYTHON) -m coverage run --branch -p $(test); )
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C __AA_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) __AA_PARSER=$(PARSER)
; $(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test, $(wildcard test-*.py), echo
; echo === $(test) === ; $(PYTHON) -m coverage run --branch -p $(test); )
$(PYTHON) -m coverage combine
coverage: .coverage
diff --git a/utils/test/test-aa-easyprof.py b/utils/test/test-aa-easyprof.py
index 8f7e916..e7916de 100755
--- a/utils/test/test-aa-easyprof.py
+++ b/utils/test/test-aa-easyprof.py
@@ -173,6 +173,13 @@ TEMPLATES_DIR="%s/templates"
if base:
self.full_args.append('--base=%s' % base)
+# Check __AA_PARSER, which may be set by the Makefile, to see if
+# we should use a non-default apparmor_parser path to verify
+# policy
+parser = os.getenv('__AA_PARSER')
+if parser:
+self.full_args.append('--parser=%s' % parser)
+
if debugging:
self.full_args.append('-d')
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v2 4/8] utils: Accept parser base and include options in aa-easyprof
https://launchpad.net/bugs/1521031
aa-easyprof accepts a list of abstractions to include and, by default,
execs apparmor_parser to verify the generated profile including any
abstractions. However, aa-easyprof didn't provide the same flexibility
as apparmor_parser when it came to where in the filesystem the
abstraction files could exist.
The parser supports --base (defaulting to /etc/apparmor.d) and --Include
(defaulting to unset) options to specify the search paths for
abstraction files. This patch adds the same options to aa-easyprof to
aide in two different situations:
1) Some Ubuntu packages use aa-easyprof to generate AppArmor profiles
at build time. Something that has been previously needed is a way
for those packages to ship their own abstractions file(s) that are
#included in the easyprof-generated profile. That's not been
possible since the abstraction file(s) have not yet been installed
during the package build.
2) The test-aa-easyprof.py script contains some tests that specify
abstractions that should be #included. Without the ability to
specify a different --base or --Include directory, the abstractions
were required to be present in /etc/apparmor.d/abstractions/ or the
tests would fail. This prevents the Python utils from being able to
strictly test against in-tree code/profiles/etc.
I don't like the names of the command line options --base and --Include.
They're not particularly descriptive and the capital 'I' is not user
friendly. However, I decided to preserve the name of the options from
apparmor_parser.
Signed-off-by: Tyler Hicks
Acked-by: Christian Boltz
Acked-by: Seth Arnold
---
utils/aa-easyprof.pod | 10 +
utils/apparmor/easyprof.py | 43 +
utils/test/test-aa-easyprof.py | 88 ++
3 files changed, 133 insertions(+), 8 deletions(-)
diff --git a/utils/aa-easyprof.pod b/utils/aa-easyprof.pod
index e82041b..31687bf 100644
--- a/utils/aa-easyprof.pod
+++ b/utils/aa-easyprof.pod
@@ -64,6 +64,16 @@ usually recommended you use policy groups instead, but this
is provided as a
convenience. AppArmor abstractions are located in /etc/apparmor.d/abstractions.
See apparmor.d(5) for details.
+=item -b PATH, --base=PATH
+
+Set the base PATH for resolving abstractions specified by --abstractions.
+See the same option in apparmor_parser(8) for details.
+
+=item -I PATH, --Include=PATH
+
+Add PATH to the search paths used for resolving abstractions specified by
+--abstractions. See the same option in apparmor_parser(8) for details.
+
=item -r PATH, --read-path=PATH
Specify a PATH to allow owner reads. May be specified multiple times. If the
diff --git a/utils/apparmor/easyprof.py b/utils/apparmor/easyprof.py
index d7ca3e1..01c7fd6 100644
--- a/utils/apparmor/easyprof.py
+++ b/utils/apparmor/easyprof.py
@@ -259,7 +259,7 @@ def open_file_read(path):
return orig
-def verify_policy(policy):
+def verify_policy(policy, base=None, include=None):
'''Verify policy compiles'''
exe = "/sbin/apparmor_parser"
if not os.path.exists(exe):
@@ -279,7 +279,14 @@ def verify_policy(policy):
os.write(f, policy)
os.close(f)
-rc, out = cmd([exe, '-QTK', fn])
+command = [exe, '-QTK']
+if base:
+command.extend(['-b', base])
+if include:
+command.extend(['-I', include])
+command.append(fn)
+
+rc, out = cmd(command)
os.unlink(fn)
if rc == 0:
return True
@@ -302,6 +309,14 @@ class AppArmorEasyProfile:
if os.path.isfile(self.conffile):
self._get_defaults()
+self.parser_base = "/etc/apparmor.d"
+if opt.parser_base:
+self.parser_base = opt.parser_base
+
+self.parser_include = None
+if opt.parser_include:
+self.parser_include = opt.parser_include
+
if opt.templates_dir and os.path.isdir(opt.templates_dir):
self.dirs['templates'] = os.path.abspath(opt.templates_dir)
elif not opt.templates_dir and \
@@ -350,8 +365,6 @@ class AppArmorEasyProfile:
if not 'policygroups' in self.dirs:
raise AppArmorException("Could not find policygroups directory")
-self.aa_topdir = "/etc/apparmor.d"
-
self.binary = binary
if binary:
if not valid_binary_path(binary):
@@ -506,9 +519,15 @@ class AppArmorEasyProfile:
def gen_abstraction_rule(self, abstraction):
'''Generate an abstraction rule'''
-p = os.path.join(self.aa_topdir, "abstractions", abstraction)
-if not os.path.exists(p):
-raise AppArmorException("%s does not exist" % p)
+base = os.path.join(self.parser_base, "abstractions"
[apparmor] [PATCH v2 0/8] Adjust the utils tests to test what's in the source tree
The utils tests, ran via $(make -C utils/ check), have long suffered from requiring files which originate in the AppArmor source tree to be installed in a system-wide manner. Some examples of files that are assumed to be installed in system-wide locations are profiles, abstractions, configuration files for the utils, and the parser itself. This patch set adjusts the utils tests to, by default, utilize the files in the source tree rather than looking outside of the source tree. This drove some changes into the utils themselves in the case of aa-easyprof. With this patch set applied, I can successfully perform a run of the utils tests in a minimal, pristine Ubuntu Zesty chroot containing no installed AppArmor packages. For developers that want to continue testing against the system packages, the USE_SYSTEM=1 make variable can be passed to the make command. * Changes since v1: - Rewrote the third patch from using an env var to set a non-default config directory to requiring that a new init_aa() method be called when using the apparmor.aa module, as requested by cboltz. - Separately committed the fourth patch, titled "Fix failing tests in test-aa.py", and dropped it from this patch set. - Wrapped the changes to the aa-easyprof man page at 80 chars - Added a new patch to the series, patch 8, which fixes flaky test results in test-aa-easyprof.py Tyler Tyler Hicks (8): utils: Improve error messages when profiles/parser is not found utils: Update the logprof.conf in the test dir to point to in-tree paths utils: Require apparmor.aa users to call init_aa() utils: Accept parser base and include options in aa-easyprof utils: Set parser base path according to USE_SYSTEM make variable utils: Add option to aa-easyprof to specify the apparmor_parser path utils: Set parser executable path according to USE_SYSTEM make variable utils: Fix apparmor.easyprof import in test-aa-easyprof.py utils/aa-easyprof.pod | 16 utils/aa-genprof | 1 + utils/aa-logprof | 1 + utils/aa-mergeprof| 3 + utils/aa-unconfined | 1 + utils/apparmor/aa.py | 43 ++ utils/apparmor/cleanprofile.py| 1 + utils/apparmor/easyprof.py| 64 +++ utils/apparmor/tools.py | 2 + utils/test/Makefile | 10 ++- utils/test/common_test.py | 11 +++ utils/test/logprof.conf | 6 +- utils/test/minitools_test.py | 3 +- utils/test/test-aa-easyprof.py| 131 -- utils/test/test-aa.py | 3 +- utils/test/test-config.py | 2 +- utils/test/test-libapparmor-test_multi.py | 3 +- utils/test/test-mount_parse.py| 3 +- utils/test/test-parser-simple-tests.py| 3 +- utils/test/test-pivot_root_parse.py | 3 +- utils/test/test-regex_matches.py | 3 +- utils/test/test-unix_parse.py | 3 +- 22 files changed, 247 insertions(+), 69 deletions(-) -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 3/8] utils: Add confdir env variable to aa.py for in-tree testing
On 02/15/2017 06:29 PM, Christian Boltz wrote: > Hello, > > Am Mittwoch, 15. Februar 2017, 12:21:05 CET schrieb Tyler Hicks: >> On 02/12/2017 12:55 PM, Christian Boltz wrote: >>> Am Mittwoch, 8. Februar 2017, 22:01:40 CET schrieb Tyler Hicks: >>>> Instead of hard-coding the location of logprof.conf and other utils >>>> related configuration files to /etc/apparmor/, this patch looks for >>>> the "APPARMOR_PY_CONFDIR" environment variable and, when set, uses >>>> its value for the configuration directory. >>>> >>>> This allows for the make check target to use the in-tree config >>>> file, >>>> profiles, and parser by default. To override this behavior, the >>>> >>>> USE_SYSTEM make variable needs to be set like so: >>>> $ make USE_SYSTEM=1 -C utils check >>>> >>>> The APPARMOR_PY_CONFDIR should be considered somewhat user-facing, >>>> although undocumented at this time. >>> >>> I'm not the biggest fan of env variables, so I'm a bit undecided on >>> this patch. >> >> Agreed. I don't like env vars for stuff like this. >> >>> Pro: >>> - the env variable is an easy solution >>> - It makes it easy to test aa-logprof, aa-complain etc. with a >>> different> >>> config dir. Otherwise we'd have to add commandline parameters >>> similar >>> to what 5/8 does in aa-easyprof. >> >> I think the most important pro is that it doesn't break existing users >> of aa.py. > > Silly question - are you aware of any external users of aa.py? I'm not aware of any. I have very little involvement with aa.py so I'm probably not the best person to ask. If we regularly break the API, then I don't mind going the init_aa() route. Tyler > > I changed aa.py quite a lot, and never got any complaints IIRC. > So either we have no external users, or I was very lucky and only > touched code we only use internally ;-) Given my luck in triggering > each and every bug, I'd consider this highly unlikely ;-) > > Oh, and IIRC we never gave any guarantee about stable interfaces. This > doesn't mean we should break it without a good reason, but avoiding env > variables would be a good reason IMHO ;-) > > I'm quite sure that apparmor/rule/*.py will have a stable interface > (unless something unexpected happens) - but I won't guarantee that for > aa.py because it contains too much code that is more or less on my > "needs a rewrite" list. > > I even thought about making parts of aa.py (the part that handles and > stores profiles) a class, which would avoid some workarounds in aa- > mergeprof. This won't happen in the near future, but maybe on the long > term. And there are some other things that need to be changed first ;-) > > Side note: If someone needs stable interfaces for something, please > speak up. Ideally submit a test-external-interface.py which tests all > the function calls etc. you need - this is the fastest way to make > breakage visible ;-) > >>> Cons: >>> - undocumented env vars are not nice[tm] >>> - env variables are less "visible" than commandline parameters, so >>> if >>> >>> someone "accidently" has them set, it will make debugging more >>> funny. >>> >>> An alternative solution would be to move the global code in aa.py >>> into> >>> def init_aa( ) >>> >>> This would mean every user of aa.py has to call that function (at >>> least if using a function that depends on the configfile), but it >>> would also prevent executing some code (like reading the config >>> file) when "just" importing apparmor.aa. >> >> I like this idea but would hate to see it deployed for this use case. >> AFAICT, this confdir change is really only useful for our internal >> test suite and > > This is exactly why I don't like the env variable - if someone has it > set accidentially, funny things[tm] might happen. Especially the fact > that we only need it for internal usage (unit tests) means that exposing > it to production code can only hurt. > > [Insert usual note about global variables (which are not too different > from env variables) and the "fun" you can have with them here] > >> breaking existing, external code for such a reason >> would be a bad thing for us to do. > > We should split off a 2.11 branch before changing the interface - but > besides that, I see no
Re: [apparmor] [PATCH 3/8] utils: Add confdir env variable to aa.py for in-tree testing
On 02/12/2017 12:55 PM, Christian Boltz wrote:
> Hello,
>
> Am Mittwoch, 8. Februar 2017, 22:01:40 CET schrieb Tyler Hicks:
>> Instead of hard-coding the location of logprof.conf and other utils
>> related configuration files to /etc/apparmor/, this patch looks for
>> the "APPARMOR_PY_CONFDIR" environment variable and, when set, uses
>> its value for the configuration directory.
>>
>> This allows for the make check target to use the in-tree config file,
>> profiles, and parser by default. To override this behavior, the
>> USE_SYSTEM make variable needs to be set like so:
>>
>> $ make USE_SYSTEM=1 -C utils check
>>
>> The APPARMOR_PY_CONFDIR should be considered somewhat user-facing,
>> although undocumented at this time.
>
> I'm not the biggest fan of env variables, so I'm a bit undecided on this
> patch.
Agreed. I don't like env vars for stuff like this.
>
> Pro:
> - the env variable is an easy solution
> - It makes it easy to test aa-logprof, aa-complain etc. with a different
> config dir. Otherwise we'd have to add commandline parameters similar
> to what 5/8 does in aa-easyprof.
I think the most important pro is that it doesn't break existing users
of aa.py.
>
> Cons:
> - undocumented env vars are not nice[tm]
> - env variables are less "visible" than commandline parameters, so if
> someone "accidently" has them set, it will make debugging more funny.
>
>
> An alternative solution would be to move the global code in aa.py into
> def init_aa( )
>
> This would mean every user of aa.py has to call that function (at least
> if using a function that depends on the configfile), but it would also
> prevent executing some code (like reading the config file) when "just"
> importing apparmor.aa.
I like this idea but would hate to see it deployed for this use case.
AFAICT, this confdir change is really only useful for our internal test
suite and breaking existing, external code for such a reason would be a
bad thing for us to do.
Do we have any other good reasons, which benefit external users of
aa.py, to introduce a requirement to call init_aa()?
Tyler
>
> BTW: Honoring the env variable in the tests would be acceptable - I'm
> not too keen to handle a --system or --local option in all test-*.py
>
>
> As I said, I'm still undecided, so please try to convince me that the
> env variable is the better choice ;-)
>
>> Signed-off-by: Tyler Hicks
>> Cc: Christian Boltz
>> ---
>> utils/apparmor/aa.py | 2 +-
>> utils/test/Makefile | 6 --
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
>> index eecf8c7..9450baa 100644
>> --- a/utils/apparmor/aa.py
>> +++ b/utils/apparmor/aa.py
>> @@ -73,7 +73,7 @@ _ = init_translation()
>> # Setup logging incase of debugging is enabled
>> debug_logger = DebugLogger('aa')
>>
>> -CONFDIR = '/etc/apparmor'
>> +CONFDIR = os.getenv('APPARMOR_PY_CONFDIR', '/etc/apparmor')
>
> As Seth already noted, the env variable could be empty.
>
> Let me add that it could include crap, for exampe
> APPARMOR_PY_CONFDIR=/you/will/never/find/me
>
> I didn't test what happens in such cases, but maybe some sanity check
> might be a good idea.
>
>
> Besides this (and my general concerns about using an env variable), the
> patch looks good.
>
>
>
> Regards,
>
> Christian Boltz
>
>
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] utils: Don't enforce ordering of dbus rule attributes
On 02/12/2017 01:30 PM, Christian Boltz wrote:
> Hello,
>
> Am Mittwoch, 8. Februar 2017, 23:56:27 CET schrieb Tyler Hicks:
>> https://launchpad.net/bugs/1628286
>>
>> The utils were enforcing that the dbus rule attributes were strictly
>> ordered in the following fashion:
>>
>> bus -> path -> interface -> member -> peer
>>
>> However, the parser has always accepted the attributes in any order.
>> If the system contained a profile which did not use the strict
>> ordering enforced by the utils, the utils would refuse to operate at
>> all.
>>
>> This patch eases the restriction on the ordering at the expense of the
>> utils no longer being able to detect and reject a single attribute
>> that is repeated multiple times. In that situation, only the last
>> occurrence of the attribute will be honored by the utils.
>
> Also note that writing (in "clean" mode) a dbus rule back to a profile
> will only include the last match. This has the advantage of accidently
> fixing the profile, and the disadvantage of altering the profile without
> any notice.
>
>> Signed-off-by: Tyler Hicks
>> Cc: Christian Boltz
>> ---
>> utils/apparmor/rule/dbus.py| 12 ++--
>> utils/test/test-dbus.py| 6 ++
>> utils/test/test-parser-simple-tests.py | 8 +++-
>> 3 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/utils/apparmor/rule/dbus.py b/utils/apparmor/rule/dbus.py
>> index 60f1ecf..58dc7b5 100644
>> --- a/utils/apparmor/rule/dbus.py
>> +++ b/utils/apparmor/rule/dbus.py
>> @@ -40,11 +40,11 @@ RE_FLAG =
>> '(?P<%s>(\S+|"[^"]+"|\(\s*\S+\s*\)|\(\s*"[^"]+"\)\s*))'# s
>> RE_DBUS_DETAILS = re.compile(
>
> It's probably a good idea to add something like
> # XXX this regex will allow repeated parameters, last one wins
> # XXX (the parser will reject such rules)
>
>
>> diff --git a/utils/test/test-dbus.py b/utils/test/test-dbus.py
>> index 5b676bc..f1bcb25 100644
>> --- a/utils/test/test-dbus.py
>> +++ b/utils/test/test-dbus.py
>> @@ -89,6 +89,10 @@ class DbusTestParse(DbusTest):
>
>> + ('dbus bus=system path=/foo/bar bus=session,'
>
> Please add
> # XXX bus= specified twice, last one wins
> to make clear this test is about suboptimal behaviour
>
>> + ('dbus bus=1 bus=2 bus=3 bus=4 bus=5 bus=6,'
>
> This deserves the same comment ;-) (well, s/twice/multipe times/)
>
>
> With these comments added,
> Acked-by: Christian Boltz
Thanks for the reviews! I've made the changes that you requested here.
>
>
> As I already stated in the bugreport, having a parse_dbus_rule() in
> libapparmor would be even better ;-)
Agreed. I hope we can get to that point someday.
Tyler
>
>
> Regards,
>
> Christian Boltz
>
>
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 8/8] utils: Set parser executable path according to USE_SYSTEM make variable
On 02/08/2017 06:23 PM, Seth Arnold wrote:
> On Wed, Feb 08, 2017 at 10:01:45PM +0000, Tyler Hicks wrote:
>> if USE_SYSTEM is not set, the utils make check target will instruct
>> test-aa-easyprof.py to provide the path of the in-tree parser executable
>> to aa-easyprof.
>>
>> If USE_SYSTEM is set, the default parser path (/sbin/apparmor_parser or
>> the result of `which apparmor_parser`) is used.
>>
>> The test-aa-easyprof.py script receives the parser path by checking the
>> __AA_PARSER environment variable. This environment variable is strictly
>> used by the test script and not any user-facing code so two leading
>> underscores were used.
>>
>> Signed-off-by: Tyler Hicks
>> Cc: Christian Boltz
>> Cc: Jamie Strandboge
>> ---
>
> This appears to suffer a similar problem:
>
>> +# Check __AA_PARSER, which may be set by the Makefile, to see if
>> +# we should use a non-default apparmor_parser path to verify
>> +# policy
>> +parser = os.getenv('__AA_PARSER')
>> +if parser:
>> +self.full_args.append('--parser=%s' % parser)
>> +
I feel like this one behaves as intended. If __AA_PARSER is an empty
string, the "if parser:" conditional is tests out to be false and
self.full_args remains unchanged.
Tyler
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 3/8] utils: Add confdir env variable to aa.py for in-tree testing
On 02/08/2017 06:00 PM, Seth Arnold wrote:
> On Wed, Feb 08, 2017 at 10:01:40PM +0000, Tyler Hicks wrote:
>> --- a/utils/apparmor/aa.py
>> +++ b/utils/apparmor/aa.py
>> @@ -73,7 +73,7 @@ _ = init_translation()
>> # Setup logging incase of debugging is enabled
>> debug_logger = DebugLogger('aa')
>>
>> -CONFDIR = '/etc/apparmor'
>> +CONFDIR = os.getenv('APPARMOR_PY_CONFDIR', '/etc/apparmor')
>>
>
>> check: __libapparmor
>> +export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
>> LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) ; $(foreach test, $(wildcard
>> test-*.py), echo ; echo === $(test) === ; $(call pyalldo, $(test)))
>>
>
> Note that it's possible for the variable to be set, but not to a value;
> os.getenv() won't return the fallback in this spot:
>
> $ export SARS= ; python3 -c 'import os; print(os.getenv("SARNOLD", "NOT
> SET"))'
> NOT SET
> $ export SARS= ; python3 -c 'import os; print(os.getenv("SARS", "NOT SET"))'
>
> $
>
>
Good catch! I'll change the line to:
CONFDIR = os.getenv('APPARMOR_PY_CONFDIR') or '/etc/apparmor'
Let me know if you'd like me to send a v2 of the patch.
Tyler
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 5/8] utils: Accept parser base and include options in aa-easyprof
On 02/08/2017 06:22 PM, Seth Arnold wrote: > On Wed, Feb 08, 2017 at 10:01:42PM +0000, Tyler Hicks wrote: >> https://launchpad.net/bugs/1521031 >> >> aa-easyprof accepts a list of abstractions to include and, by default, >> execs apparmor_parser to verify the generated profile including any >> abstractions. However, aa-easyprof didn't provide the same flexibility >> as apparmor_parser when it came to where in the filesystem the >> abstraction files could exist. >> >> The parser supports --base (defaulting to /etc/apparmor.d) and --Include >> (defaulting to unset) options to specify the search paths for >> abstraction files. This patch adds the same options to aa-easyprof to >> aide in two different situations: >> >> 1) Some Ubuntu packages use aa-easyprof to generate AppArmor profiles >> at build time. Something that has been previously needed is a way >> for those packages to ship their own abstractions file(s) that are >> #included in the easyprof-generated profile. That's not been >> possible since the abstraction file(s) have not yet been installed >> during the package build. >> >> 2) The test-aa-easyprof.py script contains some tests that specify >> abstractions that should be #included. Without the ability to >> specify a different --base or --Include directory, the abstractions >> were required to be present in /etc/apparmor.d/abstractions/ or the >> tests would fail. This prevents the Python utils from being able to >> strictly test against in-tree code/profiles/etc. >> >> I don't like the names of the command line options --base and --Include. >> They're not particularly descriptive and the capital 'I' is not user >> friendly. However, I decided to preserve the name of the options from >> apparmor_parser. >> >> Signed-off-by: Tyler Hicks >> Cc: Christian Boltz >> Cc: Jamie Strandboge >> --- > > I'd rather the manpage text wrap before 80 chars but otherwise looks good. Me too. Fixed locally. > Acked-by: Seth Arnold > Thanks Thanks for your reviews! Tyler > > >> >> A different approach to fixing bug 1521031 was previously sent to the list >> for >> discussion: >> >> https://lists.ubuntu.com/archives/apparmor/2015-November/008875.html >> >> However, that proposed solution didn't receive positive reviews and doesn't >> address the needs of the utils testsuite. IMO, adding functionality for >> discovering abstractions equivalent to what the parser supports is the proper >> solution here. >> >> Tyler >> >> utils/aa-easyprof.pod | 8 >> utils/apparmor/easyprof.py | 43 + >> utils/test/test-aa-easyprof.py | 88 >> ++ >> 3 files changed, 131 insertions(+), 8 deletions(-) >> >> diff --git a/utils/aa-easyprof.pod b/utils/aa-easyprof.pod >> index e82041b..1a08408 100644 >> --- a/utils/aa-easyprof.pod >> +++ b/utils/aa-easyprof.pod >> @@ -64,6 +64,14 @@ usually recommended you use policy groups instead, but >> this is provided as a >> convenience. AppArmor abstractions are located in >> /etc/apparmor.d/abstractions. >> See apparmor.d(5) for details. >> >> +=item -b PATH, --base=PATH >> + >> +Set the base PATH for resolving abstractions specified by --abstractions. >> See the same option in apparmor_parser(8) for details. >> + >> +=item -I PATH, --Include=PATH >> + >> +Add PATH to the search paths used for resolving abstractions specified by >> --abstractions. See the same option in apparmor_parser(8) for details. >> + >> =item -r PATH, --read-path=PATH >> >> Specify a PATH to allow owner reads. May be specified multiple times. If the >> diff --git a/utils/apparmor/easyprof.py b/utils/apparmor/easyprof.py >> index d7ca3e1..01c7fd6 100644 >> --- a/utils/apparmor/easyprof.py >> +++ b/utils/apparmor/easyprof.py >> @@ -259,7 +259,7 @@ def open_file_read(path): >> return orig >> >> >> -def verify_policy(policy): >> +def verify_policy(policy, base=None, include=None): >> '''Verify policy compiles''' >> exe = "/sbin/apparmor_parser" >> if not os.path.exists(exe): >> @@ -279,7 +279,14 @@ def verify_policy(policy): >> os.write(f, policy) >> os.close(f) >> >> -rc, out = cmd([exe, '-QTK', fn]) >> +command = [exe, '-QTK']
[apparmor] [PATCH] utils: Don't enforce ordering of dbus rule attributes
https://launchpad.net/bugs/1628286
The utils were enforcing that the dbus rule attributes were strictly
ordered in the following fashion:
bus -> path -> interface -> member -> peer
However, the parser has always accepted the attributes in any order. If
the system contained a profile which did not use the strict ordering
enforced by the utils, the utils would refuse to operate at all.
This patch eases the restriction on the ordering at the expense of the
utils no longer being able to detect and reject a single attribute that
is repeated multiple times. In that situation, only the last occurrence
of the attribute will be honored by the utils.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
---
utils/apparmor/rule/dbus.py| 12 ++--
utils/test/test-dbus.py| 6 ++
utils/test/test-parser-simple-tests.py | 8 +++-
3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/utils/apparmor/rule/dbus.py b/utils/apparmor/rule/dbus.py
index 60f1ecf..58dc7b5 100644
--- a/utils/apparmor/rule/dbus.py
+++ b/utils/apparmor/rule/dbus.py
@@ -40,11 +40,11 @@ RE_FLAG =
'(?P<%s>(\S+|"[^"]+"|\(\s*\S+\s*\)|\(\s*"[^"]+"\)\s*))'# s
RE_DBUS_DETAILS = re.compile(
'^' +
'(\s+(?P' + RE_ACCESS_KEYWORDS + '))?' + # optional access
keyword(s)
-'(\s+(bus\s*=\s*' + RE_FLAG % 'bus' + '))?' + # optional bus=
system | session | AARE, (...) optional
-'(\s+(path\s*=\s*' + RE_FLAG % 'path' + '))?' + # optional
path=AARE, (...) optional
-'(\s+(name\s*=\s*' + RE_FLAG % 'name' + '))?' + # optional
name=AARE, (...) optional
-'(\s+(interface\s*=\s*' + RE_FLAG % 'interface' + '))?' + # optional
interface=AARE, (...) optional
-'(\s+(member\s*=\s*'+ RE_FLAG % 'member'+ '))?' + # optional
member=AARE, (...) optional
+'((\s+(bus\s*=\s*' + RE_FLAG % 'bus' + '))?|' + # optional
bus= system | session | AARE, (...) optional
+'(\s+(path\s*=\s*' + RE_FLAG % 'path' + '))?|' + # optional
path=AARE, (...) optional
+'(\s+(name\s*=\s*' + RE_FLAG % 'name' + '))?|' + # optional
name=AARE, (...) optional
+'(\s+(interface\s*=\s*' + RE_FLAG % 'interface' + '))?|' + # optional
interface=AARE, (...) optional
+'(\s+(member\s*=\s*'+ RE_FLAG % 'member'+ '))?|' + # optional
member=AARE, (...) optional
'(\s+(peer\s*=\s*\((,|\s)*'+ # optional peer=( name=AARE and/or
label=AARE ), (...) required
'(' +
'(' + '(,|\s)*' + ')' + # empty peer=()
@@ -57,7 +57,7 @@ RE_DBUS_DETAILS = re.compile(
'|' # or
'(' + 'label\s*=\s*' + RE_PROFILE_NAME % 'peerlabel3' +
'(,|\s)+' + 'name\s*=\s*' + RE_PROFILE_NAME % 'peername3' + ')' + # peer
label + name (match name peername3/peerlabel3)
')'
-'(,|\s)*\)))?'
+'(,|\s)*\)))?){0,6}'
'\s*$')
diff --git a/utils/test/test-dbus.py b/utils/test/test-dbus.py
index 5b676bc..f1bcb25 100644
--- a/utils/test/test-dbus.py
+++ b/utils/test/test-dbus.py
@@ -89,6 +89,10 @@ class DbusTestParse(DbusTest):
('dbus peer=(, label = bar, name = foo ),', exp(False, False,
False, '',None , True , None, True, None,
True, None, True, None, True, None, True, 'foo',
False, 'bar,', False)), # XXX peerlabel includes the comma
('dbus peer=( label = bar , name = foo ),', exp(False, False,
False, '',None , True , None, True, None,
True, None, True, None, True, None, True, 'foo',
False, 'bar', False)),
('dbus peer=( label = "bar" name = "foo" ),' , exp(False, False,
False, '',None , True , None, True, None,
True, None, True, None, True, None, True, 'foo',
False, 'bar', False)),
+('dbus path=/foo/bar bus=session,' , exp(False, False,
False, '',None , True , 'session', False, '/foo/bar',
False, None, True, None, True, None, True, None,
True, None, Tr
[apparmor] [PATCH 8/8] utils: Set parser executable path according to USE_SYSTEM make variable
if USE_SYSTEM is not set, the utils make check target will instruct
test-aa-easyprof.py to provide the path of the in-tree parser executable
to aa-easyprof.
If USE_SYSTEM is set, the default parser path (/sbin/apparmor_parser or
the result of `which apparmor_parser`) is used.
The test-aa-easyprof.py script receives the parser path by checking the
__AA_PARSER environment variable. This environment variable is strictly
used by the test script and not any user-facing code so two leading
underscores were used.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
Cc: Jamie Strandboge
---
utils/test/Makefile| 6 --
utils/test/test-aa-easyprof.py | 7 +++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/utils/test/Makefile b/utils/test/Makefile
index 0b56a0a..5e91c7c 100644
--- a/utils/test/Makefile
+++ b/utils/test/Makefile
@@ -25,6 +25,7 @@ ifdef USE_SYSTEM
PYTHONPATH=
CONFDIR=
BASEDIR=
+PARSER=
else
# PYTHON_DIST_BUILD_PATH based on libapparmor/swig/python/test/Makefile.am
PYTHON_DIST_BUILD_PATH =
../../libraries/libapparmor/swig/python/build/$$($(PYTHON) -c "import
distutils.util; import platform; print(\"lib.%s-%s\"
%(distutils.util.get_platform(), platform.python_version()[:3]))")
@@ -32,6 +33,7 @@ else
PYTHONPATH=..:$(PYTHON_DIST_BUILD_PATH)
CONFDIR=$(CURDIR)
BASEDIR=../../profiles/apparmor.d
+PARSER=../../parser/apparmor_parser
endif
.PHONY: __libapparmor
@@ -66,10 +68,10 @@ clean:
rm -rf __pycache__/ .coverage htmlcov
check: __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ; $(foreach
test, $(wildcard test-*.py), echo ; echo === $(test) === ; $(call pyalldo,
$(test)))
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR)
__AA_PARSER=$(PARSER) ; $(foreach test, $(wildcard test-*.py), echo ; echo ===
$(test) === ; $(call pyalldo, $(test)))
.coverage: $(wildcard ../aa-* ../apparmor/*.py test-*.py) __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ;
$(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test, $(wildcard test-*.py), echo ;
echo === $(test) === ; $(PYTHON) -m coverage run --branch -p $(test); )
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR)
__AA_PARSER=$(PARSER) ; $(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test,
$(wildcard test-*.py), echo ; echo === $(test) === ; $(PYTHON) -m coverage run
--branch -p $(test); )
$(PYTHON) -m coverage combine
coverage: .coverage
diff --git a/utils/test/test-aa-easyprof.py b/utils/test/test-aa-easyprof.py
index 8f7e916..e7916de 100755
--- a/utils/test/test-aa-easyprof.py
+++ b/utils/test/test-aa-easyprof.py
@@ -173,6 +173,13 @@ TEMPLATES_DIR="%s/templates"
if base:
self.full_args.append('--base=%s' % base)
+# Check __AA_PARSER, which may be set by the Makefile, to see if
+# we should use a non-default apparmor_parser path to verify
+# policy
+parser = os.getenv('__AA_PARSER')
+if parser:
+self.full_args.append('--parser=%s' % parser)
+
if debugging:
self.full_args.append('-d')
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 0/8] Adjust the utils tests to test what's in the source tree
The utils tests, ran via $(make -C utils/ check), have long suffered from requiring files which originate in the AppArmor source tree to be installed in a system-wide manner. Some examples of files that are assumed to be installed in system-wide locations are profiles, abstractions, configuration files for the utils, and the parser itself. This patch set adjusts the utils tests to, by default, utilize the files in the source tree rather than looking outside of the source tree. This drove some changes into the utils themselves in the case of aa-easyprof. With this patch set applied, I can successfully perform a run of the utils tests in a minimal, pristine Ubuntu Zesty chroot containing no installed AppArmor packages. For developers that want to continue testing against the system packages, the USE_SYSTEM=1 make variable can be passed to the make command. Tyler Hicks (8): utils: Improve error messages when profiles/parser is not found utils: Update the logprof.conf in the test dir to point to in-tree paths utils: Add confdir env variable to aa.py for in-tree testing utils: Fix failing tests in test-aa.py utils: Accept parser base and include options in aa-easyprof utils: Set parser base path according to USE_SYSTEM make variable utils: Add option to aa-easyprof to specify the apparmor_parser path utils: Set parser executable path according to USE_SYSTEM make variable utils/aa-easyprof.pod | 14 ++ utils/apparmor/aa.py | 6 +-- utils/apparmor/easyprof.py | 64 +++-- utils/test/Makefile| 10 +++- utils/test/logprof.conf| 6 +-- utils/test/test-aa-easyprof.py | 105 + utils/test/test-aa.py | 8 ++-- utils/test/test-config.py | 2 +- 8 files changed, 188 insertions(+), 27 deletions(-) -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 1/8] utils: Improve error messages when profiles/parser is not found
When aa.py is imported, it looks for a set of profiles and it also looks
for the parser. Both of these paths are configured by logprof.conf but
it isn't always obvious which logprof.conf file was used and, therefore,
it isn't always obvious where aa.py is looking. This patch includes the
paths in the error messages.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
---
utils/apparmor/aa.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
index ab7f6c9..eecf8c7 100644
--- a/utils/apparmor/aa.py
+++ b/utils/apparmor/aa.py
@@ -3754,11 +3754,11 @@ if cfg['settings'].get('default_owner_prompt', False):
profile_dir = conf.find_first_dir(cfg['settings'].get('profiledir')) or
'/etc/apparmor.d'
if not os.path.isdir(profile_dir):
-raise AppArmorException('Can\'t find AppArmor profiles')
+raise AppArmorException('Can\'t find AppArmor profiles in %s' %
(profile_dir))
extra_profile_dir =
conf.find_first_dir(cfg['settings'].get('inactive_profiledir')) or
'/usr/share/apparmor/extra-profiles/'
parser = conf.find_first_file(cfg['settings'].get('parser')) or
'/sbin/apparmor_parser'
if not os.path.isfile(parser) or not os.access(parser, os.EX_OK):
-raise AppArmorException('Can\'t find apparmor_parser')
+raise AppArmorException('Can\'t find apparmor_parser at %s' % (parser))
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 3/8] utils: Add confdir env variable to aa.py for in-tree testing
Instead of hard-coding the location of logprof.conf and other utils
related configuration files to /etc/apparmor/, this patch looks for the
"APPARMOR_PY_CONFDIR" environment variable and, when set, uses its value
for the configuration directory.
This allows for the make check target to use the in-tree config file,
profiles, and parser by default. To override this behavior, the
USE_SYSTEM make variable needs to be set like so:
$ make USE_SYSTEM=1 -C utils check
The APPARMOR_PY_CONFDIR should be considered somewhat user-facing,
although undocumented at this time.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
---
utils/apparmor/aa.py | 2 +-
utils/test/Makefile | 6 --
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
index eecf8c7..9450baa 100644
--- a/utils/apparmor/aa.py
+++ b/utils/apparmor/aa.py
@@ -73,7 +73,7 @@ _ = init_translation()
# Setup logging incase of debugging is enabled
debug_logger = DebugLogger('aa')
-CONFDIR = '/etc/apparmor'
+CONFDIR = os.getenv('APPARMOR_PY_CONFDIR', '/etc/apparmor')
# The database for severity
sev_db = None
diff --git a/utils/test/Makefile b/utils/test/Makefile
index 014c094..f5273e3 100644
--- a/utils/test/Makefile
+++ b/utils/test/Makefile
@@ -23,11 +23,13 @@ include $(COMMONDIR)/Make.rules
ifdef USE_SYSTEM
LD_LIBRARY_PATH=
PYTHONPATH=
+CONFDIR=
else
# PYTHON_DIST_BUILD_PATH based on libapparmor/swig/python/test/Makefile.am
PYTHON_DIST_BUILD_PATH =
../../libraries/libapparmor/swig/python/build/$$($(PYTHON) -c "import
distutils.util; import platform; print(\"lib.%s-%s\"
%(distutils.util.get_platform(), platform.python_version()[:3]))")
LD_LIBRARY_PATH=../../libraries/libapparmor/src/.libs/
PYTHONPATH=..:$(PYTHON_DIST_BUILD_PATH)
+CONFDIR=$(CURDIR)
endif
.PHONY: __libapparmor
@@ -62,10 +64,10 @@ clean:
rm -rf __pycache__/ .coverage htmlcov
check: __libapparmor
- export PYTHONPATH=$(PYTHONPATH) ; export
LD_LIBRARY_PATH=$(LD_LIBRARY_PATH) ; export LC_ALL=C; $(foreach test,
$(wildcard test-*.py), echo ; echo === $(test) === ; $(call pyalldo, $(test)))
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) ; $(foreach test, $(wildcard
test-*.py), echo ; echo === $(test) === ; $(call pyalldo, $(test)))
.coverage: $(wildcard ../aa-* ../apparmor/*.py test-*.py) __libapparmor
- export PYTHONPATH=$(PYTHONPATH) ; export
LD_LIBRARY_PATH=$(LD_LIBRARY_PATH); export LC_ALL=C;
$(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test, $(wildcard test-*.py), echo ;
echo === $(test) === ; $(PYTHON) -m coverage run --branch -p $(test); )
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) ; $(COVERAGE_IGNORE_FAILURES_CMD) ;
$(foreach test, $(wildcard test-*.py), echo ; echo === $(test) === ; $(PYTHON)
-m coverage run --branch -p $(test); )
$(PYTHON) -m coverage combine
coverage: .coverage
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 4/8] utils: Fix failing tests in test-aa.py
The merged /usr patches to the policy broke some utils tests due to a
change in the expected output.
Fixes: r3600 update lots of profiles for usrMerge
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
---
utils/test/test-aa.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py
index af4edbd..65cbd1f 100644
--- a/utils/test/test-aa.py
+++ b/utils/test/test-aa.py
@@ -784,8 +784,8 @@ class AaTest_get_file_perms_2(AATest):
('/dev/null', {'allow': {'all': {'r',
'w', 'k'}, 'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': {'/dev/null'} }),
('/foo/bar',{'allow': {'all': {'r',
'w'}, 'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': {'/foo/bar'} }), # exec perms not
included
('/no/thing', {'allow': {'all': set(),
'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': set() }),
-('/usr/lib/ispell/',{'allow': {'all': {'r'},
'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': {'/usr/lib/ispell/', '/usr/lib{,32,64}/**'}}), # from
abstractions/enchant
-('/usr/lib/aspell/*.so',{'allow': {'all': {'m',
'r'}, 'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': {'/usr/lib/aspell/*', '/usr/lib/aspell/*.so', '/usr/lib{,32,64}/**'}
}), # from abstractions/aspell via abstractions/enchant
+('/usr/lib/ispell/',{'allow': {'all': {'r'},
'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': {'/usr/lib/ispell/', '/{usr/,}lib{,32,64}/**'}}), # from
abstractions/enchant
+('/usr/lib/aspell/*.so',{'allow': {'all': {'m',
'r'}, 'owner': set() }, 'deny': {'all':set(),'owner': set()},
'paths': {'/usr/lib/aspell/*', '/usr/lib/aspell/*.so',
'/{usr/,}lib{,32,64}/**'} }), # from abstractions/aspell via
abstractions/enchant
]
def _run_test(self, params, expected):
@@ -820,8 +820,8 @@ class AaTest_propose_file_rules(AATest):
(['/usr/share/common-licenses/foo/bar', 'w'],
['/usr/share/common*/foo/* rw,', '/usr/share/common-licenses/** rw,',
'/usr/share/common-licenses/foo/bar rw,'] ),
(['/dev/null', 'wk'], ['/dev/null rwk,']
),
(['/foo/bar', 'rw'], ['/foo/bar rw,']
),
-(['/usr/lib/ispell/', 'w'], ['/usr/lib{,32,64}/**
rw,', '/usr/lib/ispell/ rw,']
),
-(['/usr/lib/aspell/some.so','k'], ['/usr/lib/aspell/*
mrk,', '/usr/lib/aspell/*.so mrk,', '/usr/lib{,32,64}/** mrk,',
'/usr/lib/aspell/some.so mrk,'] ),
+(['/usr/lib/ispell/', 'w'],
['/{usr/,}lib{,32,64}/** rw,', '/usr/lib/ispell/ rw,']
),
+(['/usr/lib/aspell/some.so','k'], ['/usr/lib/aspell/*
mrk,', '/usr/lib/aspell/*.so mrk,', '/{usr/,}lib{,32,64}/** mrk,',
'/usr/lib/aspell/some.so mrk,'] ),
]
def _run_test(self, params, expected):
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 7/8] utils: Add option to aa-easyprof to specify the apparmor_parser path
When testing against a clean system without the apparmor_parser binary
installed, the test-aa-easyprof.py script ends up skipping profile
verification because it can't find the parser binary. This even causes a
test failure due to the test_genpolicy_invalid_template_policy test.
Adding a --parser option to aa-easyprof is the first step in addressing
this problem.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
Cc: Jamie Strandboge
---
utils/aa-easyprof.pod | 6 ++
utils/apparmor/easyprof.py | 25 +
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/utils/aa-easyprof.pod b/utils/aa-easyprof.pod
index 1a08408..88288b7 100644
--- a/utils/aa-easyprof.pod
+++ b/utils/aa-easyprof.pod
@@ -57,6 +57,12 @@ for supported policy groups. The available policy groups are
in
AppArmor rules or policies. They are similar to AppArmor abstractions, but
usually encompass more policy rules.
+=item --parser PATH
+
+Specify the PATH of the apparmor_parser binary to use when verifying
+policy. If this option is not specified, aa-easyprof will attempt to
+locate the path starting with /sbin/apparmor_parser.
+
=item -a ABSTRACTIONS, --abstractions=ABSTRACTIONS
Specify ABSTRACTIONS as a comma-separated list of AppArmor abstractions. It is
diff --git a/utils/apparmor/easyprof.py b/utils/apparmor/easyprof.py
index 01c7fd6..c6e6932 100644
--- a/utils/apparmor/easyprof.py
+++ b/utils/apparmor/easyprof.py
@@ -259,14 +259,11 @@ def open_file_read(path):
return orig
-def verify_policy(policy, base=None, include=None):
+def verify_policy(policy, exe, base=None, include=None):
'''Verify policy compiles'''
-exe = "/sbin/apparmor_parser"
-if not os.path.exists(exe):
-rc, exe = cmd(['which', 'apparmor_parser'])
-if rc != 0:
-warn("Could not find apparmor_parser. Skipping verify")
-return True
+if not exe:
+warn("Could not find apparmor_parser. Skipping verify")
+return True
fn = ""
# if policy starts with '/' and is one line, assume it is a path
@@ -309,6 +306,14 @@ class AppArmorEasyProfile:
if os.path.isfile(self.conffile):
self._get_defaults()
+self.parser_path = '/sbin/apparmor_parser'
+if opt.parser_path:
+self.parser_path = opt.parser_path
+elif not os.path.exists(self.parser_path):
+rc, self.parser_path = cmd(['which', 'apparmor_parser'])
+if rc != 0:
+self.parser_path = None
+
self.parser_base = "/etc/apparmor.d"
if opt.parser_base:
self.parser_base = opt.parser_base
@@ -680,7 +685,7 @@ class AppArmorEasyProfile:
if no_verify:
debug("Skipping policy verification")
-elif not verify_policy(policy, self.parser_base, self.parser_include):
+elif not verify_policy(policy, self.parser_path, self.parser_base,
self.parser_include):
msg("\n" + policy)
raise AppArmorException("Invalid policy")
@@ -823,6 +828,10 @@ def check_for_manifest_arg_append(option, opt_str, value,
parser):
def add_parser_policy_args(parser):
'''Add parser arguments'''
+parser.add_option("--parser",
+ dest="parser_path",
+ help="The path to the profile parser used for
verification",
+ metavar="PATH")
parser.add_option("-a", "--abstractions",
action="callback",
callback=check_for_manifest_arg,
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 6/8] utils: Set parser base path according to USE_SYSTEM make variable
If USE_SYSTEM is not set, the utils make check target will instruct
test-aa-easyprof.py to provide the path of the in-tree
profiles/apparmor.d directory to aa-easyprof as the parser base
directory.
If USE_SYSTEM is set, the default base directory (/etc/apparmor.d) is
used.
The test-aa-easyprof.py script receives the base path by checking the
__AA_BASEDIR environment variable. This environment variable is strictly
used by the test script and not any user-facing code so two leading
underscores were used.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
Cc: Jamie Strandboge
---
utils/test/Makefile| 6 --
utils/test/test-aa-easyprof.py | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/utils/test/Makefile b/utils/test/Makefile
index f5273e3..0b56a0a 100644
--- a/utils/test/Makefile
+++ b/utils/test/Makefile
@@ -24,12 +24,14 @@ ifdef USE_SYSTEM
LD_LIBRARY_PATH=
PYTHONPATH=
CONFDIR=
+BASEDIR=
else
# PYTHON_DIST_BUILD_PATH based on libapparmor/swig/python/test/Makefile.am
PYTHON_DIST_BUILD_PATH =
../../libraries/libapparmor/swig/python/build/$$($(PYTHON) -c "import
distutils.util; import platform; print(\"lib.%s-%s\"
%(distutils.util.get_platform(), platform.python_version()[:3]))")
LD_LIBRARY_PATH=../../libraries/libapparmor/src/.libs/
PYTHONPATH=..:$(PYTHON_DIST_BUILD_PATH)
CONFDIR=$(CURDIR)
+BASEDIR=../../profiles/apparmor.d
endif
.PHONY: __libapparmor
@@ -64,10 +66,10 @@ clean:
rm -rf __pycache__/ .coverage htmlcov
check: __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) ; $(foreach test, $(wildcard
test-*.py), echo ; echo === $(test) === ; $(call pyalldo, $(test)))
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ; $(foreach
test, $(wildcard test-*.py), echo ; echo === $(test) === ; $(call pyalldo,
$(test)))
.coverage: $(wildcard ../aa-* ../apparmor/*.py test-*.py) __libapparmor
- export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) ; $(COVERAGE_IGNORE_FAILURES_CMD) ;
$(foreach test, $(wildcard test-*.py), echo ; echo === $(test) === ; $(PYTHON)
-m coverage run --branch -p $(test); )
+ export PYTHONPATH=$(PYTHONPATH) LD_LIBRARY_PATH=$(LD_LIBRARY_PATH)
LC_ALL=C APPARMOR_PY_CONFDIR=$(CONFDIR) __AA_BASEDIR=$(BASEDIR) ;
$(COVERAGE_IGNORE_FAILURES_CMD) ; $(foreach test, $(wildcard test-*.py), echo ;
echo === $(test) === ; $(PYTHON) -m coverage run --branch -p $(test); )
$(PYTHON) -m coverage combine
coverage: .coverage
diff --git a/utils/test/test-aa-easyprof.py b/utils/test/test-aa-easyprof.py
index 3ebfec6..8f7e916 100755
--- a/utils/test/test-aa-easyprof.py
+++ b/utils/test/test-aa-easyprof.py
@@ -163,6 +163,16 @@ TEMPLATES_DIR="%s/templates"
self.binary = "/opt/bin/foo"
self.full_args = ['-c', self.conffile, self.binary]
+# Check __AA_BASEDIR, which may be set by the Makefile, to see if
+# we should use a non-default base directory path to find
+# abstraction files
+#
+# NOTE: Individual tests can append another --base path to the
+# args list and override a base path set here
+base = os.getenv('__AA_BASEDIR')
+if base:
+self.full_args.append('--base=%s' % base)
+
if debugging:
self.full_args.append('-d')
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 5/8] utils: Accept parser base and include options in aa-easyprof
https://launchpad.net/bugs/1521031
aa-easyprof accepts a list of abstractions to include and, by default,
execs apparmor_parser to verify the generated profile including any
abstractions. However, aa-easyprof didn't provide the same flexibility
as apparmor_parser when it came to where in the filesystem the
abstraction files could exist.
The parser supports --base (defaulting to /etc/apparmor.d) and --Include
(defaulting to unset) options to specify the search paths for
abstraction files. This patch adds the same options to aa-easyprof to
aide in two different situations:
1) Some Ubuntu packages use aa-easyprof to generate AppArmor profiles
at build time. Something that has been previously needed is a way
for those packages to ship their own abstractions file(s) that are
#included in the easyprof-generated profile. That's not been
possible since the abstraction file(s) have not yet been installed
during the package build.
2) The test-aa-easyprof.py script contains some tests that specify
abstractions that should be #included. Without the ability to
specify a different --base or --Include directory, the abstractions
were required to be present in /etc/apparmor.d/abstractions/ or the
tests would fail. This prevents the Python utils from being able to
strictly test against in-tree code/profiles/etc.
I don't like the names of the command line options --base and --Include.
They're not particularly descriptive and the capital 'I' is not user
friendly. However, I decided to preserve the name of the options from
apparmor_parser.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
Cc: Jamie Strandboge
---
A different approach to fixing bug 1521031 was previously sent to the list for
discussion:
https://lists.ubuntu.com/archives/apparmor/2015-November/008875.html
However, that proposed solution didn't receive positive reviews and doesn't
address the needs of the utils testsuite. IMO, adding functionality for
discovering abstractions equivalent to what the parser supports is the proper
solution here.
Tyler
utils/aa-easyprof.pod | 8
utils/apparmor/easyprof.py | 43 +
utils/test/test-aa-easyprof.py | 88 ++
3 files changed, 131 insertions(+), 8 deletions(-)
diff --git a/utils/aa-easyprof.pod b/utils/aa-easyprof.pod
index e82041b..1a08408 100644
--- a/utils/aa-easyprof.pod
+++ b/utils/aa-easyprof.pod
@@ -64,6 +64,14 @@ usually recommended you use policy groups instead, but this
is provided as a
convenience. AppArmor abstractions are located in /etc/apparmor.d/abstractions.
See apparmor.d(5) for details.
+=item -b PATH, --base=PATH
+
+Set the base PATH for resolving abstractions specified by --abstractions. See
the same option in apparmor_parser(8) for details.
+
+=item -I PATH, --Include=PATH
+
+Add PATH to the search paths used for resolving abstractions specified by
--abstractions. See the same option in apparmor_parser(8) for details.
+
=item -r PATH, --read-path=PATH
Specify a PATH to allow owner reads. May be specified multiple times. If the
diff --git a/utils/apparmor/easyprof.py b/utils/apparmor/easyprof.py
index d7ca3e1..01c7fd6 100644
--- a/utils/apparmor/easyprof.py
+++ b/utils/apparmor/easyprof.py
@@ -259,7 +259,7 @@ def open_file_read(path):
return orig
-def verify_policy(policy):
+def verify_policy(policy, base=None, include=None):
'''Verify policy compiles'''
exe = "/sbin/apparmor_parser"
if not os.path.exists(exe):
@@ -279,7 +279,14 @@ def verify_policy(policy):
os.write(f, policy)
os.close(f)
-rc, out = cmd([exe, '-QTK', fn])
+command = [exe, '-QTK']
+if base:
+command.extend(['-b', base])
+if include:
+command.extend(['-I', include])
+command.append(fn)
+
+rc, out = cmd(command)
os.unlink(fn)
if rc == 0:
return True
@@ -302,6 +309,14 @@ class AppArmorEasyProfile:
if os.path.isfile(self.conffile):
self._get_defaults()
+self.parser_base = "/etc/apparmor.d"
+if opt.parser_base:
+self.parser_base = opt.parser_base
+
+self.parser_include = None
+if opt.parser_include:
+self.parser_include = opt.parser_include
+
if opt.templates_dir and os.path.isdir(opt.templates_dir):
self.dirs['templates'] = os.path.abspath(opt.templates_dir)
elif not opt.templates_dir and \
@@ -350,8 +365,6 @@ class AppArmorEasyProfile:
if not 'policygroups' in self.dirs:
raise AppArmorException("Could not find policygroups directory")
-self.aa_topdir = "/etc/apparmor.d"
-
self.binary = binary
if binary:
if not valid_binary_path(binary):
@@ -506,9 +519
[apparmor] [PATCH 2/8] utils: Update the logprof.conf in the test dir to point to in-tree paths
The utils tests should make use of the logprof.conf that resides in
utils/test/ when testing against the in-tree parser and profiles. When
testing against the system, it the utils tests should continue to use
the system logprof.conf.
This patch updates the parser and profiles paths to point to the in-tree
paths. Another patch is needed to get aa.py to honor a non-hardcoded
search path for logprof.conf and other configuration files.
Signed-off-by: Tyler Hicks
Cc: Christian Boltz
---
utils/test/logprof.conf | 6 +++---
utils/test/test-config.py | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/utils/test/logprof.conf b/utils/test/logprof.conf
index 47ad563..31ed5c9 100644
--- a/utils/test/logprof.conf
+++ b/utils/test/logprof.conf
@@ -10,11 +10,11 @@
# --
[settings]
- profiledir = /etc/apparmor.d /etc/subdomain.d
- inactive_profiledir = /usr/share/doc/apparmor-profiles/extras
+ profiledir = ../../profiles/apparmor.d
+ inactive_profiledir = ../../profiles/apparmor/profiles/extra
logfiles = /var/log/audit/audit.log /var/log/syslog /var/log/messages
- parser = /sbin/apparmor_parser /sbin/subdomain_parser
+ parser = ../../parser/apparmor_parser
ldd = /usr/bin/ldd
logger = /bin/logger /usr/bin/logger
diff --git a/utils/test/test-config.py b/utils/test/test-config.py
index 70bdfca..3468c3b 100755
--- a/utils/test/test-config.py
+++ b/utils/test/test-config.py
@@ -24,7 +24,7 @@ class Test(unittest.TestCase):
conf = ini_config.read_config('logprof.conf')
logprof_sections = ['settings', 'repository', 'qualifiers',
'required_hats', 'defaulthat', 'globs']
logprof_sections_options = ['profiledir', 'inactive_profiledir',
'logfiles', 'parser', 'ldd', 'logger', 'default_owner_prompt',
'custom_includes']
-logprof_settings_parser = '/sbin/apparmor_parser
/sbin/subdomain_parser'
+logprof_settings_parser = '../../parser/apparmor_parser'
self.assertEqual(conf.sections(), logprof_sections)
self.assertEqual(conf.options('settings'), logprof_sections_options)
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] parser: Preserve techdoc files in the clean target
On 01/20/2017 09:46 AM, intrigeri wrote: > Tyler Hicks: >> On 01/20/2017 02:15 AM, intrigeri wrote: >>> note that as far the Debian packaging is concerned, I'll keep building >>> that file from source: that's the only way to guarantee that we >>> distribute the source "code" corresponding to the binary artifacts >>> included in our binary packages. This doesn't conflict with the >>> proposed patch though, I'll just need to delete the pre-built PDF >>> found in the upstream tarball before building, to ensure it is indeed >>> built from scratch :) > >> I don't think this is going to work as well as you'd like. :/ > >> The pdf is being generated in a 'setup' target, which the Debian >> packaging doesn't currently use and I'm not even sure if it should use >> it. Deleting the pdf and then building the package, in the way that >> Debian packaging is currently doing it, will not regenerate the pdf file. > > Do you mean this won't work anymore once the patch proposed on this > thread is applied? (FTR it seems to work just fine in 2.11.0-1.) No, I didn't think it would work with or without this patch. Great to hear that it did work for you! >> In Ubuntu, I was planning on using the pre-generated pdf. > > Then I guess we should simply agree to disagree about this aspect of > Free Software :) I don't even know if I disagree with you on this topic. I just don't have strong convictions about using a PDF that upstream generated and was instead excited about the simplified build process. In the time that it'd take us to enjoy a single round of beers, I bet you could convince me that it is the wrong way of thinking. :) Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] parser: Preserve techdoc files in the clean target
On 01/20/2017 02:15 AM, intrigeri wrote: > Hi, > > note that as far the Debian packaging is concerned, I'll keep building > that file from source: that's the only way to guarantee that we > distribute the source "code" corresponding to the binary artifacts > included in our binary packages. This doesn't conflict with the > proposed patch though, I'll just need to delete the pre-built PDF > found in the upstream tarball before building, to ensure it is indeed > built from scratch :) I don't think this is going to work as well as you'd like. :/ The pdf is being generated in a 'setup' target, which the Debian packaging doesn't currently use and I'm not even sure if it should use it. Deleting the pdf and then building the package, in the way that Debian packaging is currently doing it, will not regenerate the pdf file. In Ubuntu, I was planning on using the pre-generated pdf. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] parser: Preserve techdoc files in the clean target
On 01/20/2017 06:31 AM, Simon McVittie wrote: > On Fri, 20 Jan 2017 at 04:14:53 +0000, Tyler Hicks wrote: >> -rm -rf techdoc.aux techdoc.out techdoc.log techdoc.pdf techdoc.toc >> techdoc.txt techdoc/ > > If my (admittedly very rusty) memory of LaTeX is correct, shouldn't > you still be deleting the .log, .out, etc., and only keeping > the final product techdoc.pdf? You're not the only one that has rusty LaTeX knowledge. :) I think you're right that those files shouldn't be retained but I don't think `make clean` is the right way to remove them. They're now generated as part of the tarball release process and I think we should be deleting them after successful pdf generation but before creating the tar. Does that sound about right to you? Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] parser: Preserve techdoc files in the clean target
The techdoc files are now part of the upstream release tarball and should not be removed as part of the clean target. Removing the techdoc files, especially techdoc.pdf, breaks existing build scripts that call the clean target before building AppArmor. The techdoc.pdf file is not generated as part of the build any longer so the pdf will not be present at the end of the build if it was initially deleted by the clean target. Rather than creating a new set of maintainer-clean targets throughout the source tree, I think it is best to simply not remove the techdoc files via make. These are files that should only be generated when a release is being made and, if needed, the AppArmor maintainers can use the VCS for cleaning untracked files. The maintainer-clean targets would be very rarely used and would needlessly complicate the Makefiles. Signed-off-by: Tyler Hicks --- parser/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/parser/Makefile b/parser/Makefile index ca2a44e..08f89c1 100644 --- a/parser/Makefile +++ b/parser/Makefile @@ -393,7 +393,6 @@ clean: pod_clean rm -f $(NAME)*.tar.gz $(NAME)*.tgz rm -f af_names.h rm -f cap_names.h - rm -rf techdoc.aux techdoc.out techdoc.log techdoc.pdf techdoc.toc techdoc.txt techdoc/ $(MAKE) -s -C $(AAREDIR) clean $(MAKE) -s -C po clean $(MAKE) -s -C tst clean -- 2.7.4 -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH] profiles: Grant access to systemd-resolved in the nameservice abstraction
https://launchpad.net/bugs/1598759
Profiles that rely on the nameservice abstraction are experiencing
denials on systems configured to use systemd-resolved via the
libnss-resolve plugin.
libnss-resolve talks to systemd-resolved over D-Bus and this patch
attempts to only grant access to the safe members of the D-Bus API.
Special considerations need to be made when applying this patch to most
Linux distributions as many of them do not have the ability to perform
fine-grained AppArmor mediation of D-Bus traffic. In those cases, any
users of the nameservice abstraction (such as tcpdump or ntpd) will have
full access to the D-Bus system bus once this change is applied to the
nameservice abstraction.
Signed-off-by: Tyler Hicks
---
profiles/apparmor.d/abstractions/nameservice | 19 +++
1 file changed, 19 insertions(+)
diff --git a/profiles/apparmor.d/abstractions/nameservice
b/profiles/apparmor.d/abstractions/nameservice
index 5dff44d..a28aeeb 100644
--- a/profiles/apparmor.d/abstractions/nameservice
+++ b/profiles/apparmor.d/abstractions/nameservice
@@ -84,6 +84,25 @@
# kerberos
#include
+ # resolve
+ #
+ # Allow access to the safe members of the systemd-resolved D-Bus API:
+ #
+ # https://www.freedesktop.org/wiki/Software/systemd/resolved/
+ #
+ # This API may be used directly over the D-Bus system bus or it may be used
+ # indirectly via the nss-resolve plugin:
+ #
+ # https://www.freedesktop.org/software/systemd/man/nss-resolve.html
+ #
+ #include
+ dbus send
+ bus=system
+ path="/org/freedesktop/resolve1"
+ interface="org.freedesktop.resolve1.Manager"
+ member="Resolve{Address,Hostname,Record,Service}"
+ peer=(name="org.freedesktop.resolve1"),
+
# TCP/UDP network access
network inet stream,
network inet6 stream,
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] - fix exec_stack to work on pre 4.8 kernels
On 10/05/2016 02:46 AM, John Johansen wrote: > On 10/04/2016 07:32 PM, Tyler Hicks wrote: >> On 10/04/2016 06:31 PM, John Johansen wrote: >>> exec_stack picked up a fix to address a semantic change introduced in >>> 4.8 kernels. However this breaks the exec_stack test for kernel pre >>> 4.8. This patch uses an apparmor kernel flag to detect whether the >>> semantic change is present and adjusts the test accordingly. >> >> A couple questions below... >> >>> >>> --- >>> >>> === modified file 'tests/regression/apparmor/exec_stack.sh' >>> --- tests/regression/apparmor/exec_stack.sh 2016-09-29 04:11:29 + >>> +++ tests/regression/apparmor/exec_stack.sh 2016-10-04 21:15:48 + >>> @@ -43,6 +43,12 @@ >>> >>> touch $file $otherfile $sharedfile $thirdfile >>> >>> +if [ "$(kernel_features domain/fix_binfmt_elf_mmap)" == "true" ]; then >> >> Why is the kernel doing domain/fix_binfmt_elf_mmap instead of bumping >> the kABI? Maybe I'm misunderstanding the purpose of the kABI but I >> understood it to be bumped when there were was a change in mediation >> that causes policy change. >> > > For a few reasons. > 1. I brought up bumping the potential of bumping the abi and got no > feedback. Sorry about that. I'm still trying to fully understand how kABI is intended to be used (I think I'm getting there now) so I don't know that I could have provided much feedback. > 2. The abi bump here is odd in that it is kernel caused not apparmor. > That doesn't mean we don't bump the abi but it is less clear. Hence 1 > 3. I am worried that the patch that introduced the semantic change > will show up in stable kernels as it is an information leak fix > for tracing. For none ubuntu kernels this will mean a different > abi on the apparmor side, so bumping the abi alone is problematic. > Ubuntu is at v7, while upstream is at v6. We can bump the abi to v8 > but what do we do for upstream or other distro kernels? We can't > bump them to v7, nor v8 as they are missing other semantic changes. Interesting and frustrating problem. > > This is a case where a sub version would be nice. > > so with that issue in place so it can be properly detected and conditioned > in the regression tests. Since we can't currently tie it to a single > abi number. > > With that said I am still open to bumping to v8 for stacking based > kernels. But we need the flag to deal with v6 abi based kernels if > this ever happens. > > I am no also open to the idea of using one or two bits of the abi > for a sub version. So we can handle something like this better in the > future. One or two bits doesn't seem like enough. If that's all that we have to spare, I think I would have chosen 'domain/fix_binfmt_elf_mmap', too. > >>> +elfmmap="m" >>> +else >>> +elfmmap="" >>> +fi >>> + >>> # Verify file access and contexts by an unconfined process >>> runchecktest "EXEC_STACK (unconfined - file)" pass -f $file >>> runchecktest "EXEC_STACK (unconfined - otherfile)" pass -f $otherfile >>> @@ -66,7 +72,7 @@ >>> >>> # Verify file access and contexts by 2 stacked profiles >>> genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ >>> - image=$othertest addimage:$test $otherok $sharedok $getcon $test:rm >>> + image=$othertest addimage:$test $otherok $sharedok $getcon >>> $test:r$elfmmap >> >> The previous change (r3509) simply added 'm' to the existing '$test r,' >> rules but this patch description says, "this breaks the exec_stack test >> for kernel pre 4.8." Is it true that adding 'm' actually broke the tests >> in pre 4.8 or are you just trying to make the tests more accurate? >> > In this case yes. I originally was worried about it breaking over looking > that its just an addition. But even so these are the only tests we have > that caught the behavioral change, and I didn't want to loose that. I asked which of two scenarios are true and you answered "yes". :) I assume that you were answering yes to "are you just trying to make the tests more accurate?". My concern was that if adding 'm' to the target profile broke things, we have a larger problem on our hands in that if we update any profiles to add 'm' to the target profile, as required by 4.8 and newer kernels, then those profiles wouldn't work on older kernels any longer. I wanted
Re: [apparmor] [patch] - fix exec_stack to work on pre 4.8 kernels
On 10/04/2016 06:31 PM, John Johansen wrote: > exec_stack picked up a fix to address a semantic change introduced in > 4.8 kernels. However this breaks the exec_stack test for kernel pre > 4.8. This patch uses an apparmor kernel flag to detect whether the > semantic change is present and adjusts the test accordingly. A couple questions below... > > --- > > === modified file 'tests/regression/apparmor/exec_stack.sh' > --- tests/regression/apparmor/exec_stack.sh 2016-09-29 04:11:29 + > +++ tests/regression/apparmor/exec_stack.sh 2016-10-04 21:15:48 + > @@ -43,6 +43,12 @@ > > touch $file $otherfile $sharedfile $thirdfile > > +if [ "$(kernel_features domain/fix_binfmt_elf_mmap)" == "true" ]; then Why is the kernel doing domain/fix_binfmt_elf_mmap instead of bumping the kABI? Maybe I'm misunderstanding the purpose of the kABI but I understood it to be bumped when there were was a change in mediation that causes policy change. > +elfmmap="m" > +else > +elfmmap="" > +fi > + > # Verify file access and contexts by an unconfined process > runchecktest "EXEC_STACK (unconfined - file)" pass -f $file > runchecktest "EXEC_STACK (unconfined - otherfile)" pass -f $otherfile > @@ -66,7 +72,7 @@ > > # Verify file access and contexts by 2 stacked profiles > genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ > - image=$othertest addimage:$test $otherok $sharedok $getcon $test:rm > + image=$othertest addimage:$test $otherok $sharedok $getcon > $test:r$elfmmap The previous change (r3509) simply added 'm' to the existing '$test r,' rules but this patch description says, "this breaks the exec_stack test for kernel pre 4.8." Is it true that adding 'm' actually broke the tests in pre 4.8 or are you just trying to make the tests more accurate? Tyler > runchecktest_errno EACCES "EXEC_STACK (2 stacked - file)" fail -- $test -f > $file > runchecktest_errno EACCES "EXEC_STACK (2 stacked - otherfile)" fail -- $test > -f $otherfile > runchecktest_errno EACCES "EXEC_STACK (2 stacked - thirdfile)" fail -- $test > -f $thirdfile > @@ -79,7 +85,7 @@ > # Verify file access and contexts by 3 stacked profiles > genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ > image=$othertest addimage:$test $otherok $sharedok $getcon $test:"rix > -> &$thirdtest" -- \ > - image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:rm > + image=$thirdtest addimage:$test $thirdok $sharedok $getcon > $test:r$elfmmap > runchecktest_errno EACCES "EXEC_STACK (3 stacked - file)" fail -- $test -- > $test -f $file > runchecktest_errno EACCES "EXEC_STACK (3 stacked - otherfile)" fail -- $test > -- $test -f $otherfile > runchecktest_errno EACCES "EXEC_STACK (3 stacked - thirdfile)" fail -- $test > -- $test -f $thirdfile > @@ -89,7 +95,7 @@ > > genprofile -I $sharedok $stackotherok $stackthirdok $test:"rix -> > &$othertest" -- \ > image=$othertest addimage:$test $sharedok $stackthirdok $test:"rix -> > &$thirdtest" -- \ > - image=$thirdtest addimage:$test $sharedok $stackthirdok $test:rm > + image=$thirdtest addimage:$test $sharedok $stackthirdok $test:r$elfmmap > # Triggered an AppArmor WARN in the initial stacking patch set > runchecktest "EXEC_STACK (3 stacked - old AA WARN)" pass -p $othertest -- > $test -p $thirdtest -f $sharedfile > > @@ -120,7 +126,7 @@ > > # Verify file access and contexts in mixed mode > genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \ > - image=$othertest flag:complain addimage:$test $otherok $sharedok > $getcon $test:rm > + image=$othertest flag:complain addimage:$test $otherok $sharedok > $getcon $test:r$elfmmap > runchecktest "EXEC_STACK (mixed mode - file)" pass -- $test -f $file > runchecktest_errno EACCES "EXEC_STACK (mixed mode - otherfile)" fail -- > $test -f $otherfile > runchecktest "EXEC_STACK (mixed mode - sharedfile)" pass -- $test -f > $sharedfile > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH v1.1 2/2] libapparmor: Be consistent with the type used for buffer sizes
On 09/30/2016 02:28 PM, Seth Arnold wrote:
> On Fri, Sep 30, 2016 at 02:07:28PM -0500, Tyler Hicks wrote:
>> The features_struct.size variable is used to hold a buffer size and it
>> is also passed in as the size parameter to read(). It should be a size_t
>> instead of an int.
>>
>> A new helper function, features_buffer_remaining(), is created to handle
>> the two places where the remaining bytes in the features buffer are
>> calculated.
>>
>> This patch also changes the size parameter of load_features_dir() to a
>> size_t to match the same parameter of load_features_file() as well as
>> the features_struct.size change described above.
>>
>> Two casts were needed when comparing signed types to unsigned types.
>> These casts are safe because the signed value is checked for "< 0"
>> immediately before the casts.
>>
>> Signed-off-by: Tyler Hicks
>
> I'm not a big fan of the casts but I'm not sure C leaves us much choice.
> The pre-cast checks are nice.
I don't like the casts, either, but they're our only option if we don't
want -Wsign-compare warnings.
Thanks for the review! I suspect that you already reviewed the first
patch but didn't (n)ack it because you found the issue in the second
patch. If you have any comments on it, I'd appreciate them.
Tyler
>
> Acked-by: Seth Arnold
>
> Thanks
>
>
>> ---
>>
>> * Changes since v1:
>> - Subtract fst->buffer from fst->pos and ensure the result is not greater
>> than remaining before subtracting
>> - Move the remaining buffer calculation into a helper function
>> - Use the helper function in features_dir_cb(), which didn't have a sanity
>> check on the value of remaining
>>
>> libraries/libapparmor/src/features.c | 35
>> ++-
>> 1 file changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/libraries/libapparmor/src/features.c
>> b/libraries/libapparmor/src/features.c
>> index 088c4ea..c656c37 100644
>> --- a/libraries/libapparmor/src/features.c
>> +++ b/libraries/libapparmor/src/features.c
>> @@ -23,6 +23,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -42,7 +43,7 @@ struct aa_features {
>>
>> struct features_struct {
>> char *buffer;
>> -int size;
>> +size_t size;
>> char *pos;
>> };
>>
>> @@ -51,17 +52,30 @@ struct component {
>> size_t len;
>> };
>>
>> -static int features_snprintf(struct features_struct *fst, const char *fmt,
>> ...)
>> +static int features_buffer_remaining(struct features_struct *fst,
>> + size_t *remaining)
>> {
>> -va_list args;
>> -int i, remaining = fst->size - (fst->pos - fst->buffer);
>> +ptrdiff_t offset = fst->pos - fst->buffer;
>>
>> -if (remaining < 0) {
>> +if (offset < 0 || fst->size < (size_t)offset) {
>> errno = EINVAL;
>> -PERROR("Invalid features buffer offset\n");
>> +PERROR("Invalid features buffer offset (%td)\n", offset);
>> return -1;
>> }
>>
>> +*remaining = fst->size - offset;
>> +return 0;
>> +}
>> +
>> +static int features_snprintf(struct features_struct *fst, const char *fmt,
>> ...)
>> +{
>> +va_list args;
>> +int i;
>> +size_t remaining;
>> +
>> +if (features_buffer_remaining(fst, &remaining) == -1)
>> +return -1;
>> +
>> va_start(args, fmt);
>> i = vsnprintf(fst->pos, remaining, fmt, args);
>> va_end(args);
>> @@ -70,7 +84,7 @@ static int features_snprintf(struct features_struct *fst,
>> const char *fmt, ...)
>> errno = EIO;
>> PERROR("Failed to write to features buffer\n");
>> return -1;
>> -} else if (i >= remaining) {
>> +} else if ((size_t)i >= remaining) {
>> errno = ENOBUFS;
>> PERROR("Feature buffer full.");
>> return -1;
>> @@ -157,7 +171,10 @@ static int features_dir_cb(int dirfd, const char *name,
>> struct stat *st,
>>
>> if (S_ISREG(st->st_mode)) {
>> ssize_t len;
>> -int remaining = fst->size - (fst->pos - fst->buffer);
>> +size_t remaining;
>> +
>> +if (features_buffer_remaining(fst, &remaining) == -1)
>> +return -1;
>>
>> len = load_features_file(dirfd, name, fst->pos, remaining);
>> if (len < 0)
>> @@ -176,7 +193,7 @@ static int features_dir_cb(int dirfd, const char *name,
>> struct stat *st,
>> }
>>
>> static ssize_t load_features_dir(int dirfd, const char *path,
>> - char *buffer, int size)
>> + char *buffer, size_t size)
>> {
>> struct features_struct fst = { buffer, size, buffer };
>>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH v1.1 2/2] libapparmor: Be consistent with the type used for buffer sizes
The features_struct.size variable is used to hold a buffer size and it
is also passed in as the size parameter to read(). It should be a size_t
instead of an int.
A new helper function, features_buffer_remaining(), is created to handle
the two places where the remaining bytes in the features buffer are
calculated.
This patch also changes the size parameter of load_features_dir() to a
size_t to match the same parameter of load_features_file() as well as
the features_struct.size change described above.
Two casts were needed when comparing signed types to unsigned types.
These casts are safe because the signed value is checked for "< 0"
immediately before the casts.
Signed-off-by: Tyler Hicks
---
* Changes since v1:
- Subtract fst->buffer from fst->pos and ensure the result is not greater
than remaining before subtracting
- Move the remaining buffer calculation into a helper function
- Use the helper function in features_dir_cb(), which didn't have a sanity
check on the value of remaining
libraries/libapparmor/src/features.c | 35 ++-
1 file changed, 26 insertions(+), 9 deletions(-)
diff --git a/libraries/libapparmor/src/features.c
b/libraries/libapparmor/src/features.c
index 088c4ea..c656c37 100644
--- a/libraries/libapparmor/src/features.c
+++ b/libraries/libapparmor/src/features.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -42,7 +43,7 @@ struct aa_features {
struct features_struct {
char *buffer;
- int size;
+ size_t size;
char *pos;
};
@@ -51,17 +52,30 @@ struct component {
size_t len;
};
-static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
+static int features_buffer_remaining(struct features_struct *fst,
+size_t *remaining)
{
- va_list args;
- int i, remaining = fst->size - (fst->pos - fst->buffer);
+ ptrdiff_t offset = fst->pos - fst->buffer;
- if (remaining < 0) {
+ if (offset < 0 || fst->size < (size_t)offset) {
errno = EINVAL;
- PERROR("Invalid features buffer offset\n");
+ PERROR("Invalid features buffer offset (%td)\n", offset);
return -1;
}
+ *remaining = fst->size - offset;
+ return 0;
+}
+
+static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
+{
+ va_list args;
+ int i;
+ size_t remaining;
+
+ if (features_buffer_remaining(fst, &remaining) == -1)
+ return -1;
+
va_start(args, fmt);
i = vsnprintf(fst->pos, remaining, fmt, args);
va_end(args);
@@ -70,7 +84,7 @@ static int features_snprintf(struct features_struct *fst,
const char *fmt, ...)
errno = EIO;
PERROR("Failed to write to features buffer\n");
return -1;
- } else if (i >= remaining) {
+ } else if ((size_t)i >= remaining) {
errno = ENOBUFS;
PERROR("Feature buffer full.");
return -1;
@@ -157,7 +171,10 @@ static int features_dir_cb(int dirfd, const char *name,
struct stat *st,
if (S_ISREG(st->st_mode)) {
ssize_t len;
- int remaining = fst->size - (fst->pos - fst->buffer);
+ size_t remaining;
+
+ if (features_buffer_remaining(fst, &remaining) == -1)
+ return -1;
len = load_features_file(dirfd, name, fst->pos, remaining);
if (len < 0)
@@ -176,7 +193,7 @@ static int features_dir_cb(int dirfd, const char *name,
struct stat *st,
}
static ssize_t load_features_dir(int dirfd, const char *path,
-char *buffer, int size)
+char *buffer, size_t size)
{
struct features_struct fst = { buffer, size, buffer };
--
2.9.3
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH 2/2] libapparmor: Be consistent with the type used for buffer sizes
On 09/29/2016 09:30 PM, Seth Arnold wrote:
> On Thu, Sep 29, 2016 at 07:32:31PM -0500, Tyler Hicks wrote:
>> +size_t remaining = fst->size - (fst->pos - fst->buffer);
>>
>> if (remaining < 0) {
>
> I'm 90% sure this doesn't do what we want.
I'm at least 10% sure so that makes 100%.
I'll look at this again tomorrow and get something better sent out as a v2.
Tyler
>
> Thanks
>
>
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 2/2] libapparmor: Be consistent with the type used for buffer sizes
The features_struct.size variable is used to hold a buffer size and it
is also passed in as the size parameter to read(). It should be a size_t
instead of an int.
This patch also changes the size parameter of load_features_dir() to a
size_t to match the same parameter of load_features_file() as well as
the features_struct.size change described above.
Signed-off-by: Tyler Hicks
---
libraries/libapparmor/src/features.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/libraries/libapparmor/src/features.c
b/libraries/libapparmor/src/features.c
index 088c4ea..dd38627 100644
--- a/libraries/libapparmor/src/features.c
+++ b/libraries/libapparmor/src/features.c
@@ -42,7 +42,7 @@ struct aa_features {
struct features_struct {
char *buffer;
- int size;
+ size_t size;
char *pos;
};
@@ -54,7 +54,8 @@ struct component {
static int features_snprintf(struct features_struct *fst, const char *fmt, ...)
{
va_list args;
- int i, remaining = fst->size - (fst->pos - fst->buffer);
+ int i;
+ size_t remaining = fst->size - (fst->pos - fst->buffer);
if (remaining < 0) {
errno = EINVAL;
@@ -157,7 +158,7 @@ static int features_dir_cb(int dirfd, const char *name,
struct stat *st,
if (S_ISREG(st->st_mode)) {
ssize_t len;
- int remaining = fst->size - (fst->pos - fst->buffer);
+ size_t remaining = fst->size - (fst->pos - fst->buffer);
len = load_features_file(dirfd, name, fst->pos, remaining);
if (len < 0)
@@ -176,7 +177,7 @@ static int features_dir_cb(int dirfd, const char *name,
struct stat *st,
}
static ssize_t load_features_dir(int dirfd, const char *path,
-char *buffer, int size)
+char *buffer, size_t size)
{
struct features_struct fst = { buffer, size, buffer };
--
2.9.3
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 1/2] libapparmor: Fix overflowed return value
The load_features_file() function returned an int but calculated the
value by subtracting two pointers. On 64 bit systems, that results in a
64 bit value being represented as a 32 bit type.
Coverity CID #55992
Signed-off-by: Tyler Hicks
---
libraries/libapparmor/src/features.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/libraries/libapparmor/src/features.c
b/libraries/libapparmor/src/features.c
index 4cec6cb..088c4ea 100644
--- a/libraries/libapparmor/src/features.c
+++ b/libraries/libapparmor/src/features.c
@@ -92,8 +92,8 @@ static int features_snprintf(struct features_struct *fst,
const char *fmt, ...)
* ENOBUFS indicating that @buffer was not large enough to contain all of the
* file contents.
*/
-static int load_features_file(int dirfd, const char *path,
- char *buffer, size_t size)
+static ssize_t load_features_file(int dirfd, const char *path,
+ char *buffer, size_t size)
{
autoclose int file = -1;
char *pos = buffer;
@@ -156,7 +156,7 @@ static int features_dir_cb(int dirfd, const char *name,
struct stat *st,
return -1;
if (S_ISREG(st->st_mode)) {
- int len;
+ ssize_t len;
int remaining = fst->size - (fst->pos - fst->buffer);
len = load_features_file(dirfd, name, fst->pos, remaining);
@@ -175,8 +175,8 @@ static int features_dir_cb(int dirfd, const char *name,
struct stat *st,
return 0;
}
-static int load_features_dir(int dirfd, const char *path,
-char *buffer, int size)
+static ssize_t load_features_dir(int dirfd, const char *path,
+char *buffer, int size)
{
struct features_struct fst = { buffer, size, buffer };
@@ -369,7 +369,7 @@ int aa_features_new(aa_features **features, int dirfd,
const char *path)
{
struct stat stat_file;
aa_features *f;
- int retval;
+ ssize_t retval;
*features = NULL;
--
2.9.3
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
[apparmor] [PATCH 0/2] Fix type issues in libapparmor's feature file handling
A recent Coverity scan pointed out an integer overflow issue in libapparmor's internal load_features_file() function. That issue is fixed in the first patch. The second patch is a cleanup to consistently use size_t in a number of areas dealing with buffer sizes. Tyler -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [PATCH] tests: Fix exec_stack.sh errors under 4.8 and newer kernels
On 09/28/2016 09:45 PM, Seth Arnold wrote:
> On Wed, Sep 28, 2016 at 09:05:09PM -0500, Tyler Hicks wrote:
>> https://launchpad.net/bugs/1628745
>>
>> The following upstream kernel commit changed the semantics of the exec
>> permission check in the 4.8 kernel:
>>
>> commit 9f834ec18defc369d73ccf9e87a2790bfa05bf46
>> Author: Linus Torvalds
>> Date: Mon Aug 22 16:41:46 2016 -0700
>>
>> binfmt_elf: switch to new creds when switching to new mm
>>
>> That change means that the target profile of an exec transition must
>> have permission to map the binary being executed. This patch fixes
>> regression test failures while the exec_stack.sh test is running against
>> 4.8 and newer kernels by granting mapping permission to the target
>> profile.
>>
>> Signed-off-by: Tyler Hicks
>
> This looks good as-is but I think we should also be alerted in the future
> if AppArmor fails to enforce this requirement. What would you think about
> duplicating these tests -- one with these changes, and then the originals
> but with the segmentation violation as the expected outcome? (Made ugly of
> course by this change being conditional on kernel versions.. so not as
> simple as I described it, but I hope you get the idea.)
Thanks for the review. I think it is a good idea to have tests that
verify this particular behavior. However, I don't think exec_stack.sh is
the right place to test for that since that test script is focused on
stacking across exec and this change in behavior has nothing to do with
stacking. We just got lucky (?) that exec_stack.sh had some policy that
triggered the bug. The exec.sh test script is probably the right place
to check for this.
Tyler
>
> Acked-by: Seth Arnold
>
> Thanks
>
>> ---
>> tests/regression/apparmor/exec_stack.sh | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/regression/apparmor/exec_stack.sh
>> b/tests/regression/apparmor/exec_stack.sh
>> index 2423dea..069e658 100755
>> --- a/tests/regression/apparmor/exec_stack.sh
>> +++ b/tests/regression/apparmor/exec_stack.sh
>> @@ -66,7 +66,7 @@ runchecktest "EXEC_STACK (not stacked - bad mode)" fail -l
>> "$test" -m complain
>>
>> # Verify file access and contexts by 2 stacked profiles
>> genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
>> -image=$othertest addimage:$test $otherok $sharedok $getcon $test:r
>> +image=$othertest addimage:$test $otherok $sharedok $getcon $test:rm
>> runchecktest_errno EACCES "EXEC_STACK (2 stacked - file)" fail -- $test -f
>> $file
>> runchecktest_errno EACCES "EXEC_STACK (2 stacked - otherfile)" fail --
>> $test -f $otherfile
>> runchecktest_errno EACCES "EXEC_STACK (2 stacked - thirdfile)" fail --
>> $test -f $thirdfile
>> @@ -79,7 +79,7 @@ runchecktest "EXEC_STACK (2 stacked - bad mode)" fail --
>> $test -l "${test}//&${t
>> # Verify file access and contexts by 3 stacked profiles
>> genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
>> image=$othertest addimage:$test $otherok $sharedok $getcon $test:"rix
>> -> &$thirdtest" -- \
>> -image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:r
>> +image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:rm
>> runchecktest_errno EACCES "EXEC_STACK (3 stacked - file)" fail -- $test --
>> $test -f $file
>> runchecktest_errno EACCES "EXEC_STACK (3 stacked - otherfile)" fail --
>> $test -- $test -f $otherfile
>> runchecktest_errno EACCES "EXEC_STACK (3 stacked - thirdfile)" fail --
>> $test -- $test -f $thirdfile
>> @@ -89,7 +89,7 @@ runchecktest "EXEC_STACK (3 stacked - okcon)" pass --
>> $test -- $test -l "${third
>>
>> genprofile -I $sharedok $stackotherok $stackthirdok $test:"rix ->
>> &$othertest" -- \
>> image=$othertest addimage:$test $sharedok $stackthirdok $test:"rix ->
>> &$thirdtest" -- \
>> -image=$thirdtest addimage:$test $sharedok $stackthirdok $test:r
>> +image=$thirdtest addimage:$test $sharedok $stackthirdok $test:rm
>> # Triggered an AppArmor WARN in the initial stacking patch set
>> runchecktest "EXEC_STACK (3 stacked - old AA WARN)" pass -p $othertest --
>> $test -p $thirdtest -f $sharedfile
>>
>> @@ -120,7 +120,7 @@ runchecktest "EXEC_STACK (stacked with namespaced
>> profile - okcon)" pass -- $tes
>>
[apparmor] [PATCH] tests: Fix exec_stack.sh errors under 4.8 and newer kernels
https://launchpad.net/bugs/1628745
The following upstream kernel commit changed the semantics of the exec
permission check in the 4.8 kernel:
commit 9f834ec18defc369d73ccf9e87a2790bfa05bf46
Author: Linus Torvalds
Date: Mon Aug 22 16:41:46 2016 -0700
binfmt_elf: switch to new creds when switching to new mm
That change means that the target profile of an exec transition must
have permission to map the binary being executed. This patch fixes
regression test failures while the exec_stack.sh test is running against
4.8 and newer kernels by granting mapping permission to the target
profile.
Signed-off-by: Tyler Hicks
---
tests/regression/apparmor/exec_stack.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/regression/apparmor/exec_stack.sh
b/tests/regression/apparmor/exec_stack.sh
index 2423dea..069e658 100755
--- a/tests/regression/apparmor/exec_stack.sh
+++ b/tests/regression/apparmor/exec_stack.sh
@@ -66,7 +66,7 @@ runchecktest "EXEC_STACK (not stacked - bad mode)" fail -l
"$test" -m complain
# Verify file access and contexts by 2 stacked profiles
genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
- image=$othertest addimage:$test $otherok $sharedok $getcon $test:r
+ image=$othertest addimage:$test $otherok $sharedok $getcon $test:rm
runchecktest_errno EACCES "EXEC_STACK (2 stacked - file)" fail -- $test -f
$file
runchecktest_errno EACCES "EXEC_STACK (2 stacked - otherfile)" fail -- $test
-f $otherfile
runchecktest_errno EACCES "EXEC_STACK (2 stacked - thirdfile)" fail -- $test
-f $thirdfile
@@ -79,7 +79,7 @@ runchecktest "EXEC_STACK (2 stacked - bad mode)" fail --
$test -l "${test}//&${t
# Verify file access and contexts by 3 stacked profiles
genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
image=$othertest addimage:$test $otherok $sharedok $getcon $test:"rix
-> &$thirdtest" -- \
- image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:r
+ image=$thirdtest addimage:$test $thirdok $sharedok $getcon $test:rm
runchecktest_errno EACCES "EXEC_STACK (3 stacked - file)" fail -- $test --
$test -f $file
runchecktest_errno EACCES "EXEC_STACK (3 stacked - otherfile)" fail -- $test
-- $test -f $otherfile
runchecktest_errno EACCES "EXEC_STACK (3 stacked - thirdfile)" fail -- $test
-- $test -f $thirdfile
@@ -89,7 +89,7 @@ runchecktest "EXEC_STACK (3 stacked - okcon)" pass -- $test
-- $test -l "${third
genprofile -I $sharedok $stackotherok $stackthirdok $test:"rix -> &$othertest"
-- \
image=$othertest addimage:$test $sharedok $stackthirdok $test:"rix ->
&$thirdtest" -- \
- image=$thirdtest addimage:$test $sharedok $stackthirdok $test:r
+ image=$thirdtest addimage:$test $sharedok $stackthirdok $test:rm
# Triggered an AppArmor WARN in the initial stacking patch set
runchecktest "EXEC_STACK (3 stacked - old AA WARN)" pass -p $othertest --
$test -p $thirdtest -f $sharedfile
@@ -120,7 +120,7 @@ runchecktest "EXEC_STACK (stacked with namespaced profile -
okcon)" pass -- $tes
# Verify file access and contexts in mixed mode
genprofile -I $fileok $sharedok $getcon $test:"ix -> &$othertest" -- \
- image=$othertest flag:complain addimage:$test $otherok $sharedok
$getcon $test:r
+ image=$othertest flag:complain addimage:$test $otherok $sharedok
$getcon $test:rm
runchecktest "EXEC_STACK (mixed mode - file)" pass -- $test -f $file
runchecktest_errno EACCES "EXEC_STACK (mixed mode - otherfile)" fail -- $test
-f $otherfile
runchecktest "EXEC_STACK (mixed mode - sharedfile)" pass -- $test -f
$sharedfile
--
2.7.4
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] fix python LibAppArmor import failures with swig > 3.0.8
On 09/14/2016 04:58 PM, Steve Beattie wrote: > On Wed, Sep 14, 2016 at 04:26:07PM -0500, Tyler Hicks wrote: >> On 09/14/2016 04:05 PM, Tyler Hicks wrote: >>> On 09/14/2016 03:32 PM, Steve Beattie wrote: >>>> On Wed, Sep 14, 2016 at 02:12:35PM -0500, Tyler Hicks wrote: >>>>> On 09/14/2016 01:52 PM, Christian Boltz wrote: >>>>>> Hello, >>>>>> >>>>>> renaming LibAppArmor.py to __init__.py breaks the import path >>>>>> calculation in swig (> 3.0.8)-generated python code, leading to an error >>>>>> message saying >>>>>> No module named '_LibAppArmor' >>>>>> >>>>>> Therefore this patch drops renaming the file. To stay compatible with the >>>>>> import LibAppArmor.$function_name >>>>>> syntax, add an __init__.py that does >>>>>> from LibAppArmor.LibAppArmor import * >>>>>> >>>>>> References: https://bugzilla.opensuse.org/show_bug.cgi?id=987607 >>>>>> >>>>>> >>>>>> Also adjust .bzrignore for this change. >>>>>> >>>>>> >>>>>> >>>>>> I propose this patch for trunk and 2.10. >>>>>> I'm undecided about 2.9 - technically it shares this bug, but I'd expect >>>>>> that 2.9 users don't use the latest swig ;-) - opinions? >>>>> >>>>> Acked-by: Tyler Hicks >>>>> >>>>> Please apply to 2.9, as well. IIRC, this is one of the errors I hit when >>>>> I run `make check` on the utils/ dir from Ubuntu 16.04 (with swig >>>>> 3.0.8-0ubuntu3) development machine so it'd be nice if that's fixed in >>>>> all of our stable branches. Thanks for fixing it! >>>> >>>> Also, no, it does not fix the make check issue in the utils/ directory: >>>> >>>> === test-pivot_root_parse.py === >>>> Traceback (most recent call last): >>>> File "test-pivot_root_parse.py", line 12, in >>>> import apparmor.aa as aa >>>> File "/home/steve/bzr/apparmor-master/utils/apparmor/aa.py", line 29, in >>>> >>>> import apparmor.logparser >>>> File "/home/steve/bzr/apparmor-master/utils/apparmor/logparser.py", line >>>> 19, in >>>> import LibAppArmor >>>> File >>>> "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/__init__.py", >>>> line 1, in >>>> from LibAppArmor.LibAppArmor import * >>>> File >>>> "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/LibAppArmor.py", >>>> line 28, in >>>> _LibAppArmor = swig_import_helper() >>>> File >>>> "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/LibAppArmor.py", >>>> line 20, in swig_import_helper >>>> import _LibAppArmor >>>> ImportError: No module named _LibAppArmor >>>> >>> >>> Yeah, I should have tested this myself before I gave the ack. I see a >>> little bit different error. Maybe because I'm not forcing python3 to be >>> used? >>> >>> $ (cd libraries/libapparmor/ && ./autogen.sh && ./configure && make) \ >> >> Bah, it would help if I configured libapparmor --with-python. Doesn't >> matter though because I see the same error. I think we've previously >> determined that my "problem" is because I'm trying to test the in-tree >> code and don't have a python libapparmor module installed system-wide. > > Try 'PYTHON_VERSIONS=/usr/bin/python make check'. Unfortunately, pyalldo > tries all pythons even when PYTHON is set. I still get the same error. We stumbled upon something that worked once while discussing it in #apparmor. I don't want to derail this thread with my problem. I only mentioned it because I initially thought the patch would solve it. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] fix python LibAppArmor import failures with swig > 3.0.8
On 09/14/2016 04:05 PM, Tyler Hicks wrote: > On 09/14/2016 03:32 PM, Steve Beattie wrote: >> On Wed, Sep 14, 2016 at 02:12:35PM -0500, Tyler Hicks wrote: >>> On 09/14/2016 01:52 PM, Christian Boltz wrote: >>>> Hello, >>>> >>>> renaming LibAppArmor.py to __init__.py breaks the import path >>>> calculation in swig (> 3.0.8)-generated python code, leading to an error >>>> message saying >>>> No module named '_LibAppArmor' >>>> >>>> Therefore this patch drops renaming the file. To stay compatible with the >>>> import LibAppArmor.$function_name >>>> syntax, add an __init__.py that does >>>> from LibAppArmor.LibAppArmor import * >>>> >>>> References: https://bugzilla.opensuse.org/show_bug.cgi?id=987607 >>>> >>>> >>>> Also adjust .bzrignore for this change. >>>> >>>> >>>> >>>> I propose this patch for trunk and 2.10. >>>> I'm undecided about 2.9 - technically it shares this bug, but I'd expect >>>> that 2.9 users don't use the latest swig ;-) - opinions? >>> >>> Acked-by: Tyler Hicks >>> >>> Please apply to 2.9, as well. IIRC, this is one of the errors I hit when >>> I run `make check` on the utils/ dir from Ubuntu 16.04 (with swig >>> 3.0.8-0ubuntu3) development machine so it'd be nice if that's fixed in >>> all of our stable branches. Thanks for fixing it! >> >> Also, no, it does not fix the make check issue in the utils/ directory: >> >> === test-pivot_root_parse.py === >> Traceback (most recent call last): >> File "test-pivot_root_parse.py", line 12, in >> import apparmor.aa as aa >> File "/home/steve/bzr/apparmor-master/utils/apparmor/aa.py", line 29, in >> >> import apparmor.logparser >> File "/home/steve/bzr/apparmor-master/utils/apparmor/logparser.py", line >> 19, in >> import LibAppArmor >> File >> "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/__init__.py", >> line 1, in >> from LibAppArmor.LibAppArmor import * >> File >> "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/LibAppArmor.py", >> line 28, in >> _LibAppArmor = swig_import_helper() >> File >> "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/LibAppArmor.py", >> line 20, in swig_import_helper >> import _LibAppArmor >> ImportError: No module named _LibAppArmor >> > > Yeah, I should have tested this myself before I gave the ack. I see a > little bit different error. Maybe because I'm not forcing python3 to be > used? > > $ (cd libraries/libapparmor/ && ./autogen.sh && ./configure && make) \ Bah, it would help if I configured libapparmor --with-python. Doesn't matter though because I see the same error. I think we've previously determined that my "problem" is because I'm trying to test the in-tree code and don't have a python libapparmor module installed system-wide. Tyler > > /dev/null && (cd binutils/ && make) > /dev/null && \ > (cd parser/ && make) > /dev/null && (cd utils && make) > /dev/null > $ (cd utils && make check) > ... > === test-pivot_root_parse.py === > Traceback (most recent call last): > File "test-pivot_root_parse.py", line 12, in > import apparmor.aa as aa > File "/var/scm/apparmor.git/utils/apparmor/aa.py", line 29, in > import apparmor.logparser > File "/var/scm/apparmor.git/utils/apparmor/logparser.py", line 19, in > > import LibAppArmor > ImportError: No module named LibAppArmor > Makefile:65: recipe for target 'check' failed > make[1]: *** [check] Error 1 > make[1]: Leaving directory '/var/scm/apparmor.git/utils/test' > Makefile:92: recipe for target 'check' failed > make: *** [check] Error 2 > > It sounds like I should rescind my ack until we get some more discussion > around this patch. > > Tyler > > > > > signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] fix python LibAppArmor import failures with swig > 3.0.8
On 09/14/2016 03:32 PM, Steve Beattie wrote: > On Wed, Sep 14, 2016 at 02:12:35PM -0500, Tyler Hicks wrote: >> On 09/14/2016 01:52 PM, Christian Boltz wrote: >>> Hello, >>> >>> renaming LibAppArmor.py to __init__.py breaks the import path >>> calculation in swig (> 3.0.8)-generated python code, leading to an error >>> message saying >>> No module named '_LibAppArmor' >>> >>> Therefore this patch drops renaming the file. To stay compatible with the >>> import LibAppArmor.$function_name >>> syntax, add an __init__.py that does >>> from LibAppArmor.LibAppArmor import * >>> >>> References: https://bugzilla.opensuse.org/show_bug.cgi?id=987607 >>> >>> >>> Also adjust .bzrignore for this change. >>> >>> >>> >>> I propose this patch for trunk and 2.10. >>> I'm undecided about 2.9 - technically it shares this bug, but I'd expect >>> that 2.9 users don't use the latest swig ;-) - opinions? >> >> Acked-by: Tyler Hicks >> >> Please apply to 2.9, as well. IIRC, this is one of the errors I hit when >> I run `make check` on the utils/ dir from Ubuntu 16.04 (with swig >> 3.0.8-0ubuntu3) development machine so it'd be nice if that's fixed in >> all of our stable branches. Thanks for fixing it! > > Also, no, it does not fix the make check issue in the utils/ directory: > > === test-pivot_root_parse.py === > Traceback (most recent call last): > File "test-pivot_root_parse.py", line 12, in > import apparmor.aa as aa > File "/home/steve/bzr/apparmor-master/utils/apparmor/aa.py", line 29, in > > import apparmor.logparser > File "/home/steve/bzr/apparmor-master/utils/apparmor/logparser.py", line > 19, in > import LibAppArmor > File > "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/__init__.py", > line 1, in > from LibAppArmor.LibAppArmor import * > File > "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/LibAppArmor.py", > line 28, in > _LibAppArmor = swig_import_helper() > File > "/home/steve/bzr/apparmor-master/libraries/libapparmor/swig/python/build/lib.linux-x86_64-3.5/LibAppArmor/LibAppArmor.py", > line 20, in swig_import_helper > import _LibAppArmor > ImportError: No module named _LibAppArmor > Yeah, I should have tested this myself before I gave the ack. I see a little bit different error. Maybe because I'm not forcing python3 to be used? $ (cd libraries/libapparmor/ && ./autogen.sh && ./configure && make) \ > /dev/null && (cd binutils/ && make) > /dev/null && \ (cd parser/ && make) > /dev/null && (cd utils && make) > /dev/null $ (cd utils && make check) ... === test-pivot_root_parse.py === Traceback (most recent call last): File "test-pivot_root_parse.py", line 12, in import apparmor.aa as aa File "/var/scm/apparmor.git/utils/apparmor/aa.py", line 29, in import apparmor.logparser File "/var/scm/apparmor.git/utils/apparmor/logparser.py", line 19, in import LibAppArmor ImportError: No module named LibAppArmor Makefile:65: recipe for target 'check' failed make[1]: *** [check] Error 1 make[1]: Leaving directory '/var/scm/apparmor.git/utils/test' Makefile:92: recipe for target 'check' failed make: *** [check] Error 2 It sounds like I should rescind my ack until we get some more discussion around this patch. Tyler signature.asc Description: OpenPGP digital signature -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
Re: [apparmor] [patch] fix python LibAppArmor import failures with swig > 3.0.8
On 09/14/2016 01:52 PM, Christian Boltz wrote:
> Hello,
>
> renaming LibAppArmor.py to __init__.py breaks the import path
> calculation in swig (> 3.0.8)-generated python code, leading to an error
> message saying
> No module named '_LibAppArmor'
>
> Therefore this patch drops renaming the file. To stay compatible with the
> import LibAppArmor.$function_name
> syntax, add an __init__.py that does
> from LibAppArmor.LibAppArmor import *
>
> References: https://bugzilla.opensuse.org/show_bug.cgi?id=987607
>
>
> Also adjust .bzrignore for this change.
>
>
>
> I propose this patch for trunk and 2.10.
> I'm undecided about 2.9 - technically it shares this bug, but I'd expect
> that 2.9 users don't use the latest swig ;-) - opinions?
Acked-by: Tyler Hicks
Please apply to 2.9, as well. IIRC, this is one of the errors I hit when
I run `make check` on the utils/ dir from Ubuntu 16.04 (with swig
3.0.8-0ubuntu3) development machine so it'd be nice if that's fixed in
all of our stable branches. Thanks for fixing it!
Tyler
>
>
> Note: you'll need to cleanup your libraries/libapparmor/swig/python/
> directory manually before applying this patch ("make clean" isn't enough,
> so check "bzr ignored"), and regenerate the autogenerated files with
> autogen.sh and configure afterwards.
>
> If there's a "superclean" make target I missed, please tell me ;-)
>
>
>
> [ libapparmor-fix-import-path.diff ]
>
> Index: libraries/libapparmor/swig/python/Makefile.am
> ===
> --- libraries/libapparmor/swig/python/Makefile.am.orig 2014-01-06
> 23:08:55.0 +0100
> +++ libraries/libapparmor/swig/python/Makefile.am 2016-08-26
> 18:03:52.526582753 +0200
> @@ -6,9 +6,8 @@ SUBDIRS = test
>
> libapparmor_wrap.c: $(srcdir)/../SWIG/libapparmor.i
> $(SWIG) -python -I$(srcdir)/../../include -module LibAppArmor -o $@
> $(srcdir)/../SWIG/libapparmor.i
> - mv LibAppArmor.py __init__.py
>
> -MOSTLYCLEANFILES=libapparmor_wrap.c __init__.py
> +MOSTLYCLEANFILES=libapparmor_wrap.c LibAppArmor.py
>
> all-local: libapparmor_wrap.c setup.py
> if test ! -f libapparmor_wrap.c; then cp $(srcdir)/libapparmor_wrap.c
> . ; fi
> Index: libraries/libapparmor/swig/python/__init__.py
> ===
> --- /dev/null 1970-01-01 00:00:00.0 +
> +++ libraries/libapparmor/swig/python/__init__.py 2016-08-26
> 18:03:16.790763701 +0200
> @@ -0,0 +1 @@
> +from LibAppArmor.LibAppArmor import *
> === modified file '.bzrignore'
> --- .bzrignore 2015-10-21 19:40:35 +
> +++ .bzrignore 2016-09-14 18:34:04 +
> @@ -88,7 +88,7 @@
> libraries/libapparmor/swig/perl/blib
> libraries/libapparmor/swig/perl/libapparmor_wrap.c
> libraries/libapparmor/swig/perl/pm_to_blib
> -libraries/libapparmor/swig/python/__init__.py
> +libraries/libapparmor/swig/python/LibAppArmor.py
> libraries/libapparmor/swig/python/build/
> libraries/libapparmor/swig/python/libapparmor_wrap.c
> libraries/libapparmor/swig/python/Makefile
>
>
>
> Regards,
>
> Christian Boltz
>
>
>
signature.asc
Description: OpenPGP digital signature
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
