mir/apparmor
2
0
Fork 0
mirror of https://gitlab.com/apparmor/apparmor synced 2025-08-31 14:25:52 +00:00
Code Issues Releases Wiki Activity

parser: fix failure to properly apply deny clearing in perms accumulation

Browse Source 
The internal permission accumulation is currently broken in that
the ordering of rules matter to whether deny is clearing accumulated
perms.

If a deny node comes before an allow node the deny bits will get set
but the following allow bits won't get cleared by the deny node.

This isn't currently an actual issue for mediation as the deny
bit will be applied at one of
  1. apply_and_clear_deny
  2. permission remapping
  3. run time mediation

but it does result in the internal state having sometimes having both
allow and deny bits set, dependent on order of computation, resulting
in state machines with different sizes because minimization
partitioning is based on the internal permissions.

This means that dfa minimization may not result in a truly minimal
state machine, and even worse can cause inconsistenty and failure in
tests that rely on internal state like the equality and minimization
test, as seen in https://gitlab.com/apparmor/apparmor/-/issues/513

The failure was due to musl stl sets implementation producing a
different ordering of the nodes than glibc. So when the permissions
where accumulated the internal set of permissions were different.

Fix this by giving the different node classes their own internal priority.
This will ensure the bits are properly cleared for that priority before
accumulating.

Note: other ways of fixing.

1. Fixup internal accumulation to use accumulating perms of "higher"
   priority as part of the mask (deny and allow mask prompt).
2. Do a hard masking apply at the end after all bits have been accumulated
   (ie, in accept_perms after the for loop).

the priority route was chosen because it is a little smaller and
scales better if we get new Node types we have to deal with
(eg. planned complain node).

BugLink: https://gitlab.com/apparmor/apparmor/-/issues/513
Fixes: 1ebd99115 ("parser: change priority so that it accumulates based on permissions")
Signed-off-by: John Johansen <john.johansen@canonical.com>
(cherry picked from commit 06e349345e)
Signed-off-by: John Johansen <john.johansen@canonical.com>
This commit is contained in:
John Johansen
2025-04-25 07:44:10 -07:00
parent 67f51a4502
commit 981b08e9f9
1 changed files with 42 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
parser/libapparmor_re/hfa.cc
View File

@@ -1391,9 +1391,31 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
MatchFlag *match, perms_t &perms, perms_t &exact,
bool filedfa)
{
if (priority[i] > match->priority) {
// scaling priority *4
int pri = match->priority<<2;
/* use priority to get proper ordering and application of the type
* of match flag.
*
* Note: this is the last use of priority, it is dropped and not
* used in the backend.
*/
if (match->is_type(NODE_TYPE_DENYMATCHFLAG))
pri += 3;
// exact match must be same priority as allow as its audit
// flags has the same priority.
// current no ALLOWMATCHFLAG it is just absence of other flags
// so it has to be second last in this list, using !last
// until this gets fixed
else if (match->is_type(NODE_TYPE_EXACTMATCHFLAG) ||
(!match->is_type(NODE_TYPE_PROMPTMATCHFLAG)))
pri += 2;
else if (match->is_type(NODE_TYPE_PROMPTMATCHFLAG))
pri += 1;
if (priority[i] > pri) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " > " << match->priority << " SKIPPING " << hex << (match->perms) << "/" << (match->audit) << dec << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " > " << pri << " SKIPPING " << hex << (match->perms) << "/" << (match->audit) << dec << "\n";
return 0;
}
@@ -1409,8 +1431,8 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
if (match->perms & AA_EXEC_INHERIT) {
xmask |= AA_USER_EXEC_MMAP;
//USER_EXEC_MAP = 6
if (priority[6] < match->priority)
priority[6] = match->priority;
if (priority[6] < pri)
priority[6] = pri;
}
amask = mask | xmask;
} else if (mask & AA_OTHER_EXEC) {
@@ -1419,8 +1441,8 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
if (match->perms & AA_OTHER_EXEC_INHERIT) {
xmask |= AA_OTHER_EXEC_MMAP;
//OTHER_EXEC_MAP = 20
if (priority[20] < match->priority)
priority[20] = match->priority;
if (priority[20] < pri)
priority[20] = pri;
}
amask = mask | xmask;
} else if (((mask & AA_USER_EXEC_MMAP) &&
@@ -1429,17 +1451,17 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
(match->perms & AA_OTHER_EXEC_INHERIT))) {
// if exec && ix we handled mmp above
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " SKIPPING mmap unmasked " << hex << match->perms << "/" << match->audit << " masked " << (match->perms & amask) << "/" << (match->audit & amask) << " data " << (perms.allow & mask) << "/" << (perms.audit & mask) << " exact " << (exact.allow & mask) << "/" << (exact.audit & mask) << dec << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " SKIPPING mmap unmasked " << hex << match->perms << "/" << match->audit << " masked " << (match->perms & amask) << "/" << (match->audit & amask) << " data " << (perms.allow & mask) << "/" << (perms.audit & mask) << " exact " << (exact.allow & mask) << "/" << (exact.audit & mask) << dec << "\n";
return 0;
}
}
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " vs. " << match->priority << " mask: " << hex << mask << " xmask: " << xmask << " amask: " << amask << dec << "\n";
if (priority[i] < match->priority) {
cerr << " " << match << "[" << i << "]=" << priority[i] << " vs. " << pri << " mask: " << hex << mask << " xmask: " << xmask << " amask: " << amask << dec << "\n";
if (priority[i] < pri) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " < " << match->priority << " clearing " << hex << (perms.allow & amask) << "/" << (perms.audit & amask) << " -> " << dec;
priority[i] = match->priority;
cerr << " " << match << "[" << i << "]=" << priority[i] << " < " << pri << " clearing " << hex << (perms.allow & amask) << "/" << (perms.audit & amask) << " -> " << dec;
priority[i] = pri;
perms.clear_bits(amask);
exact.clear_bits(amask);
if (opts.dump & DUMP_DFA_PERMS)
@@ -1449,7 +1471,7 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
// the if conditions in order of permission priority
if (match->is_type(NODE_TYPE_DENYMATCHFLAG)) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " deny " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " deny " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
perms.deny |= match->perms & amask;
perms.quiet |= match->audit & amask;
@@ -1459,11 +1481,11 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
} else if (match->is_type(NODE_TYPE_EXACTMATCHFLAG)) {
/* exact match only asserts dominance on the XTYPE */
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " exact " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " exact " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
if (filedfa &&
!is_merged_x_consistent(exact.allow, match->perms & amask)) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " exact match conflict" << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " exact match conflict" << "\n";
return 1;
}
exact.allow |= match->perms & amask;
@@ -1484,11 +1506,11 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
// allow perms, if exact has been encountered will already be set
// if overlaps x here, don't conflict, because exact will override
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " allow " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " allow " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
if (filedfa && !(exact.allow & mask) &&
!is_merged_x_consistent(perms.allow, match->perms & amask)) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " allow match conflict" << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " allow match conflict" << "\n";
return 1;
}
// mask off if XTYPE in xmatch
@@ -1502,11 +1524,11 @@ static int pri_update_perm(optflags const &opts, vector<int> &priority, int i,
}
} else { // if (match->is_type(NODE_TYPE_PROMPTMATCHFLAG)) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " prompt " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " prompt " << hex << (match->perms & amask) << "/" << (match->audit & amask) << dec << "\n";
if (filedfa && !((exact.allow | perms.allow) & mask) &&
!is_merged_x_consistent(perms.allow, match->perms & amask)) {
if (opts.dump & DUMP_DFA_PERMS)
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << match->priority << " prompt match conflict" << "\n";
cerr << " " << match << "[" << i << "]=" << priority[i] << " <= " << pri << " prompt match conflict" << "\n";
return 1;
}
if ((exact.allow | exact.audit | perms.allow | perms.audit) & mask) {
@@ -1532,7 +1554,8 @@ int accept_perms(optflags const &opts, NodeVec *state, perms_t &perms,
{
int error = 0;
perms_t exact;
std::vector<int> priority(sizeof(perm32_t)*8, MIN_INTERNAL_PRIORITY); // 32 but wan't tied to perm32_t
// scaling priority by *4
std::vector<int> priority(sizeof(perm32_t)*8, MIN_INTERNAL_PRIORITY<<2); // 32 but wan't tied to perm32_t
perms.clear();
if (!state)