[apparmor] [PATCH 5/5] Revise dconf

John Johansen john.johansen at canonical.com
Fri Dec 16 18:01:47 UTC 2016


---
 libraries/libapparmor/include/sys/apparmor.h | 16 ++---
 libraries/libapparmor/src/kernel.c           | 95 +++++++++++++---------------
 parser/dconf.cc                              | 78 +++++++++++------------
 parser/dconf.h                               |  8 +--
 parser/parser_lex.l                          | 10 +--
 parser/parser_yacc.y                         | 33 ++++++++--
 parser/tst/equality.sh                       | 16 +++++
 parser/tst/simple_tests/dconf/bad_path_01.sd |  4 +-
 parser/tst/simple_tests/dconf/bad_path_02.sd |  4 +-
 parser/tst/simple_tests/dconf/bad_path_03.sd |  2 +-
 parser/tst/simple_tests/dconf/bad_path_04.sd |  2 +-
 parser/tst/simple_tests/dconf/ok_audit_01.sd |  3 +-
 parser/tst/simple_tests/dconf/ok_audit_02.sd |  8 +++
 parser/tst/simple_tests/dconf/ok_dir_01.sd   |  2 +-
 parser/tst/simple_tests/dconf/ok_dir_02.sd   |  8 +++
 parser/tst/simple_tests/dconf/ok_key_01.sd   |  2 +-
 16 files changed, 160 insertions(+), 131 deletions(-)
 create mode 100644 parser/tst/simple_tests/dconf/ok_audit_02.sd
 create mode 100644 parser/tst/simple_tests/dconf/ok_dir_02.sd

diff --git a/libraries/libapparmor/include/sys/apparmor.h b/libraries/libapparmor/include/sys/apparmor.h
index 6f79eff..ac4e2c0 100644
--- a/libraries/libapparmor/include/sys/apparmor.h
+++ b/libraries/libapparmor/include/sys/apparmor.h
@@ -63,9 +63,9 @@ __BEGIN_DECLS
 
 
 /* Permission flags for the AA_CLASS_DCONF mediation class */
-#define AA_DCONF_READ			(1 << 2)
-#define AA_DCONF_WRITE			(1 << 1)
-#define AA_DCONF_READWRITE		((AA_DCONF_READ) | (AA_DCONF_WRITE))
+#define AA_DCONF_READ			(AA_MAY_READ)
+#define AA_DCONF_WRITE			(AA_MAY_WRITE)
+#define AA_VALID_DCONF_PERMS		((AA_DCONF_READ) | (AA_DCONF_WRITE))
 
 
 /* Prototypes for apparmor state queries */
@@ -145,14 +145,8 @@ typedef struct {
 
 typedef struct {
 	char *data;			/* free data */
-	size_t rn;			/* number of rpaths */
-	size_t rwn;			/* number of rwpaths */
-	size_t arn;			/* number of arpaths */
-	size_t arwn;			/* number of arwpaths */
-	const char **rpaths;		/* read-only paths in data */
-	const char **rwpaths;		/* read-write paths in data */
-	const char **arpaths;		/* audit read-only paths in data */
-	const char **arwpaths;		/* audit read-write paths in data */
+	size_t n;			/* number of paths */
+	const char **paths;		/* paths in data */
 } aa_dconf_info;
 
 extern int aa_query_label_data(const char *label, const char *key,
diff --git a/libraries/libapparmor/src/kernel.c b/libraries/libapparmor/src/kernel.c
index 349290d..88fd692 100644
--- a/libraries/libapparmor/src/kernel.c
+++ b/libraries/libapparmor/src/kernel.c
@@ -1260,70 +1260,64 @@ int aa_query_dconf_info(const char *label, aa_dconf_info *info)
 {
 	aa_label_data_info data_info;
 	const char *p;
-	uint32_t tmp;
-	int i;
+	uint32_t tmp, size;
+	int i, n, res = -1;
 
 	memset(info, 0, sizeof (*info));
 
 	if (aa_query_label_data(label, "dconf", &data_info))
 		return -1;
 
-	if (data_info.n < 1)
-		return -1;
-
+	/* transfer the data blob */
 	info->data = data_info.data;
-	p = data_info.ents[0].entry;
-
-	memcpy(&tmp, p, sizeof(tmp));
-	p += sizeof(tmp);
-	info->rn = le32toh(tmp);
-	memcpy(&tmp, p, sizeof(tmp));
-	p += sizeof(tmp);
-	info->rwn = le32toh(tmp);
-	memcpy(&tmp, p, sizeof(tmp));
-	p += sizeof(tmp);
-	info->arn = le32toh(tmp);
-	memcpy(&tmp, p, sizeof(tmp));
-	p += sizeof(tmp);
-	info->arwn = le32toh(tmp);
-
-	info->rpaths = malloc(info->rn * sizeof(*info->rpaths));
-	info->rwpaths = malloc(info->rwn * sizeof(*info->rwpaths));
-	info->arpaths = malloc(info->arn * sizeof(*info->arpaths));
-	info->arwpaths = malloc(info->arwn * sizeof(*info->arwpaths));
-
-	for (i = 0; i < info->rn; i++) {
-		memcpy(&tmp, p, sizeof(tmp));
-		p += sizeof(tmp);
-		info->rpaths[i] = p;
-		p += le32toh(tmp);
-	}
+	data_info.data = NULL;
 
-	for (i = 0; i < info->rwn; i++) {
-		memcpy(&tmp, p, sizeof(tmp));
-		p += sizeof(tmp);
-		info->rwpaths[i] = p;
-		p += le32toh(tmp);
+	/* Find how many paths there are, one data ent per profile, each
+	 * profile can have multiple paths
+	 */
+	for (info->n = i = 0; i < data_info.n; i++) {
+		p = data_info.ents[i].entry;
+		while (p < data_info.ents[i].entry + data_info.ents[i].size) {
+			memcpy(&tmp, p, sizeof(tmp));
+			size = le32toh(tmp);
+			p += size + sizeof(tmp);
+			/* must be null terminated */
+			if (*(p-1) != 0) {
+				errno = -EPROTO;
+				goto out;
+			}
+			info->n++;
+		}
+		/* verify set of strings is exactly the size of the data */
+		if (p - data_info.ents[i].entry != data_info.ents[i].size) {
+			errno = -ERANGE;
+			goto out;
+		}
 	}
 
-	for (i = 0; i < info->arn; i++) {
-		memcpy(&tmp, p, sizeof(tmp));
-		p += sizeof(tmp);
-		info->arpaths[i] = p;
-		p += le32toh(tmp);
+	info->paths = malloc(info->n * sizeof(*info->paths));
+	if (!info->paths) {
+		errno = -ENOMEM;
+		goto out;
 	}
 
-	for (i = 0; i < info->arwn; i++) {
-		memcpy(&tmp, p, sizeof(tmp));
-		p += sizeof(tmp);
-		info->arwpaths[i] = p;
-		p += le32toh(tmp);
+	/* setups paths */
+	for (n = i = 0; i < data_info.n; i++) {
+		p = data_info.ents[i].entry;
+		while (p < data_info.ents[i].entry + data_info.ents[i].size) {
+			memcpy(&tmp, p, sizeof(tmp));
+			size = le32toh(tmp);
+			p += sizeof(tmp);
+			info->paths[n++] = p;
+			p += size;
+		}
 	}
+	res = 0;
 
-	data_info.data = NULL;
+out:
 	aa_clear_label_data(&data_info);
 
-	return 0;
+	return res;
 }
 
 /**
@@ -1334,10 +1328,7 @@ int aa_query_dconf_info(const char *label, aa_dconf_info *info)
  */
 void aa_clear_dconf_info(aa_dconf_info *info)
 {
-	free(info->arwpaths);
-	free(info->arpaths);
-	free(info->rwpaths);
-	free(info->rpaths);
+	free(info->paths);
 	free(info->data);
 
 	memset(info, 0, sizeof(*info));
diff --git a/parser/dconf.cc b/parser/dconf.cc
index 669bf80..06afd02 100644
--- a/parser/dconf.cc
+++ b/parser/dconf.cc
@@ -24,32 +24,12 @@
 #include <sstream>
 #include <iomanip>
 
-dconfDataEnt::~dconfDataEnt()
-{
-}
 
 void dconfDataEnt::serialize(std::ostringstream &buf)
 {
 	ostringstream tmp;
 
-	sd_write32(tmp, rpaths.size());
-	sd_write32(tmp, rwpaths.size());
-	sd_write32(tmp, arpaths.size());
-	sd_write32(tmp, arwpaths.size());
-
-	for (set<std::string>::iterator i = rpaths.begin(); i != rpaths.end(); i++) {
-		sd_write32(tmp, i->size() + 1);
-		tmp.write(i->c_str(), i->size() + 1);
-	}
-	for (set<std::string>::iterator i = rwpaths.begin(); i != rwpaths.end(); i++) {
-		sd_write32(tmp, i->size() + 1);
-		tmp.write(i->c_str(), i->size() + 1);
-	}
-	for (set<std::string>::iterator i = arpaths.begin(); i != arpaths.end(); i++) {
-		sd_write32(tmp, i->size() + 1);
-		tmp.write(i->c_str(), i->size() + 1);
-	}
-	for (set<std::string>::iterator i = arwpaths.begin(); i != arwpaths.end(); i++) {
+	for (set<std::string>::iterator i = paths.begin(); i != paths.end(); i++) {
 		sd_write32(tmp, i->size() + 1);
 		tmp.write(i->c_str(), i->size() + 1);
 	}
@@ -75,7 +55,7 @@ std::ostream &dconf_rule::dump(std::ostream &os)
 
 	os << "dconf (";
 
-	switch (mode & AA_DCONF_READWRITE) {
+	switch (mode & AA_VALID_DCONF_PERMS) {
 	case AA_DCONF_READ:
 		os << "read";
 		break;
@@ -90,6 +70,18 @@ std::ostream &dconf_rule::dump(std::ostream &os)
 
 int dconf_rule::expand_variables(void)
 {
+	const char *pstr;
+	int error = expand_entry_variables(&path);
+	if (error)
+		return error;
+	/* do additional semantic checks. TODO these should have their own hook */
+	if (path[0] != '/')
+		return -1;
+	for (pstr = path; *pstr; pstr++) {
+		if (*pstr == '/' && *(pstr + 1) == '/')
+			return -1;
+	}
+
 	return 0;
 }
 
@@ -108,37 +100,43 @@ void dconf_rule::gen_data(Profile &prof)
 			return;
 	}
 
-	if (mode & AA_DCONF_WRITE) {
-		if (audit)
-			dconf->arwpaths.insert(path);
-		else
-			dconf->rwpaths.insert(path);
-	} else {
-		if (audit)
-			dconf->arpaths.insert(path);
-		else
-			dconf->rpaths.insert(path);
-	}
+	/* TODO: generate an approximate watch point from the rule.
+	 * ATM just use a global root
+	 */
+	//dconf->paths.insert(path);
+	dconf->paths.insert("/");
 }
 
 int dconf_rule::gen_policy_re(Profile &prof)
 {
+	std::string buf;
 	std::ostringstream rule;
+	pattern_t ptype;
+	int pos;
 
-	if ((mode & AA_DCONF_READWRITE) == 0)
+	/* don't generate policy for rules without permissions */
+	if ((mode & AA_VALID_DCONF_PERMS) == 0)
 		return RULE_OK;
 
-	rule << "\\x" << std::setfill('0') << std::setw(2) << std::hex << AA_CLASS_DCONF;
+	ptype = convert_aaregex_to_pcre(path, 0, glob_default, buf, &pos);
+	if (ptype == ePatternInvalid)
+		return RULE_ERROR;
 
-	if (path[strlen(path) - 1] == '/')
-		rule << path << "[^\\x00]*";
+	rule << "\\x" << std::setfill('0') << std::setw(2) << std::hex << AA_CLASS_DCONF;
+	
+	/* TODO: determine whether we really want to auto convert trailing
+	 * / into subtree match. It fits with dconf style but not really
+	 * with apparmor.
+	 */
+	if (buf[buf.length() - 1] == '/')
+		rule << buf << "[^\\x00]*";
 	else
-		rule << path;
+		rule << buf;
 
 	if (!prof.policy.rules->add_rule(rule.str().c_str(),
 					 0,
-					 mode & AA_DCONF_READWRITE,
-					 audit ? mode & AA_DCONF_READWRITE : 0,
+					 mode & AA_VALID_DCONF_PERMS,
+					 audit ? mode & AA_VALID_DCONF_PERMS : 0,
 					 dfaflags))
 		return RULE_ERROR;
 
diff --git a/parser/dconf.h b/parser/dconf.h
index 0477f06..c8c72c5 100644
--- a/parser/dconf.h
+++ b/parser/dconf.h
@@ -24,12 +24,8 @@
 
 class dconfDataEnt: public DataEnt {
 public:
-	set<std::string> rpaths;
-	set<std::string> rwpaths;
-	set<std::string> arpaths;
-	set<std::string> arwpaths;
-
-	virtual ~dconfDataEnt();
+	set<std::string> paths;
+	virtual ~dconfDataEnt() { };
 
 	void serialize(std::ostringstream &buf);
 };
diff --git a/parser/parser_lex.l b/parser/parser_lex.l
index 82cd84c..7b95d97 100644
--- a/parser/parser_lex.l
+++ b/parser/parser_lex.l
@@ -215,6 +215,7 @@ ID_NOEQ		{ID_CHARS_NOEQ}|(,{ID_CHARS_NOEQ})
 IDS_NOEQ	{LEADING_ID_CHARS_NOEQ}{ID_NOEQ}*
 ALLOWED_QUOTED_ID 	[^\0"]|\\\"
 QUOTED_ID	\"{ALLOWED_QUOTED_ID}*\"
+ID_CHARS_NOPAREN	[^ \t\n"!,(]
 
 IP		{NUMBER}\.{NUMBER}\.{NUMBER}\.{NUMBER}
 
@@ -229,9 +230,6 @@ BOOL_VARIABLE	$(\{{VARIABLE_NAME}\}|{VARIABLE_NAME})
 LABEL		(\/|{SET_VARIABLE}{POST_VAR_ID}|{COLON}|{AMPERSAND}){ID}*
 QUOTED_LABEL	\"(\/|{SET_VAR_PREFIX}|{COLON}|{AMPERSAND})([^\0"]|\\\")*\"
 
-DPATHCOMP	[^[:space:]/]+
-DPATHNAME	{SLASH}({DPATHCOMP}{SLASH})*{DPATHCOMP}?
-
 OPEN_PAREN 	\(
 CLOSE_PAREN	\)
 COMMA		\,
@@ -508,7 +506,7 @@ LT_EQUAL	<=
 	tracedby	{ RETURN_TOKEN(TOK_TRACEDBY); }
 }
 
-<DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
+<DCONF_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
 	read		{ RETURN_TOKEN(TOK_READ); }
 	write		{ RETURN_TOKEN(TOK_WRITE); }
 	{OPEN_PAREN}	{
@@ -521,9 +519,7 @@ LT_EQUAL	<=
 }
 
 <DCONF_MODE>{
-	r	{ RETURN_TOKEN(TOK_READ); }
-	rw	{ RETURN_TOKEN(TOK_READWRITE); }
-	{DPATHNAME}	{
+	({ID_CHARS_NOPAREN}{ID}*|{QUOTED_ID}) {
 		yylval.id = processid(yytext, yyleng);
 		RETURN_TOKEN(TOK_ID);
 	}
diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
index 75e9387..7e6c054 100644
--- a/parser/parser_yacc.y
+++ b/parser/parser_yacc.y
@@ -149,7 +149,6 @@ void add_local_entry(Profile *prof);
 %token TOK_BIND
 %token TOK_READ
 %token TOK_WRITE
-%token TOK_READWRITE
 %token TOK_EAVESDROP
 %token TOK_PEER
 %token TOK_TRACE
@@ -1325,17 +1324,41 @@ dbus_rule: TOK_DBUS opt_dbus_perm opt_conds opt_cond_list TOK_END_OF_RULE
 	}
 
 dconf_perm: TOK_READ { $$ = AA_DCONF_READ; }
-	| TOK_READWRITE { $$ = AA_DCONF_READWRITE; }
+	| TOK_WRITE { $$ = AA_DCONF_WRITE; }
+	| TOK_VALUE
+	{
+		if (!$1)
+			$$ = 0;
+		else if (strcmp($1, "read") == 0)
+			$$ = AA_DCONF_READ;
+		else if (strcmp($1, "write") == 0)
+			$$ = AA_DCONF_WRITE;
+		else
+			parse_X_mode("dconf", AA_DCONF_READ | AA_DCONF_WRITE,
+				     $1, &$$, 1);
+		free($1);
+	}
+	| TOK_MODE
+	{
+		parse_X_mode("dconf", AA_DCONF_READ | AA_DCONF_WRITE, $1, &$$,
+			     1);
+		free($1);
+	}
 
 dconf_perms: { /* nothing */ $$ = 0; }
 	| dconf_perms dconf_perm { $$ = $1 | $2; }
+	| dconf_perms TOK_COMMA dconf_perm { $$ = $1 | $3; }
 
-opt_dconf_perms: { /* nothing */ $$ = AA_DCONF_READWRITE; /* default read-write */ }
+opt_dconf_perms: { /* nothing */ $$ = AA_VALID_DCONF_PERMS; }
 	| dconf_perms dconf_perm { $$ = $1 | $2; }
+	| TOK_OPENPAREN dconf_perms TOK_CLOSEPAREN { $$ = $2; }
 
-dconf_rule: TOK_DCONF TOK_ID opt_dconf_perms TOK_END_OF_RULE
+dconf_rule: TOK_DCONF opt_dconf_perms opt_id TOK_END_OF_RULE
 	{
-		dconf_rule *ent = new dconf_rule($2, $3);
+		dconf_rule *ent;
+		if ((($2) & AA_DCONF_WRITE) && !(($2) & AA_DCONF_READ))
+			yyerror(_("dconf write permission must be accompanied by read permission"));
+		ent = new dconf_rule($3 ? $3 : strdup("/**"), $2);
 		if (!ent) {
 			yyerror(_("Memory allocation error."));
 		}
diff --git a/parser/tst/equality.sh b/parser/tst/equality.sh
index 029eec4..3b7cfa0 100755
--- a/parser/tst/equality.sh
+++ b/parser/tst/equality.sh
@@ -265,6 +265,22 @@ verify_binary_equality "dbus minimization found in dbus abstractions" \
                    peer=(name=org.freedesktop.DBus),
 	      dbus send bus=session, }"
 
+verify_binary_equality "dconf read" \
+	"/t { dconf r /, }" \
+	"/t { dconf read /, }"
+
+verify_binary_equality "dconf read-write" \
+	"/t { dconf /, }" \
+	"/t { dconf rw /, }" \
+	"/t { dconf wr /, }" \
+	"/t { dconf (read write) /, }" \
+	"/t { dconf (write read) /, }" \
+	"/t { dconf (read, write) /, }"
+
+verify_binary_equality "dconf missing options" \
+	"/t { dconf /**, }" \
+	"/t { dconf, }"
+
 # Rules compatible with audit, deny, and audit deny
 # note: change_profile does not support audit/allow/deny atm
 for rule in "capability" "capability mac_admin" \
diff --git a/parser/tst/simple_tests/dconf/bad_path_01.sd b/parser/tst/simple_tests/dconf/bad_path_01.sd
index c78d407..ee703df 100644
--- a/parser/tst/simple_tests/dconf/bad_path_01.sd
+++ b/parser/tst/simple_tests/dconf/bad_path_01.sd
@@ -1,8 +1,8 @@
 #
-#=DESCRIPTION dconf rule missing path
+#=DESCRIPTION dconf rule path doesn't start with /
 #=EXRESULT FAIL
 #
 
 profile bad_path {
-  dconf r,
+  dconf r abc,
 }
diff --git a/parser/tst/simple_tests/dconf/bad_path_02.sd b/parser/tst/simple_tests/dconf/bad_path_02.sd
index e0ccc03..2e42492 100644
--- a/parser/tst/simple_tests/dconf/bad_path_02.sd
+++ b/parser/tst/simple_tests/dconf/bad_path_02.sd
@@ -1,8 +1,8 @@
 #
-#=DESCRIPTION dconf rule missing leading slash
+#=DESCRIPTION dconf  trailing perm
 #=EXRESULT FAIL
 #
 
 profile bad_path {
-  dconf a/b/c r,
+  dconf /a/b/c r,
 }
diff --git a/parser/tst/simple_tests/dconf/bad_path_03.sd b/parser/tst/simple_tests/dconf/bad_path_03.sd
index d9c93c9..daf746a 100644
--- a/parser/tst/simple_tests/dconf/bad_path_03.sd
+++ b/parser/tst/simple_tests/dconf/bad_path_03.sd
@@ -4,5 +4,5 @@
 #
 
 profile bad_path {
-  dconf /a//b r,
+  dconf r /a//b,
 }
diff --git a/parser/tst/simple_tests/dconf/bad_path_04.sd b/parser/tst/simple_tests/dconf/bad_path_04.sd
index a870c12..d9e086b 100644
--- a/parser/tst/simple_tests/dconf/bad_path_04.sd
+++ b/parser/tst/simple_tests/dconf/bad_path_04.sd
@@ -4,5 +4,5 @@
 #
 
 profile bad_path {
-  dconf /a /b r,
+  dconf r /a /b,
 }
diff --git a/parser/tst/simple_tests/dconf/ok_audit_01.sd b/parser/tst/simple_tests/dconf/ok_audit_01.sd
index d2e63f2..6b2087c 100644
--- a/parser/tst/simple_tests/dconf/ok_audit_01.sd
+++ b/parser/tst/simple_tests/dconf/ok_audit_01.sd
@@ -4,6 +4,5 @@
 #
 
 profile ok_audit {
-  audit dconf /a/b/c r,
-  audit dconf /d/e/f rw,
+  audit dconf r /a/b/c,
 }
diff --git a/parser/tst/simple_tests/dconf/ok_audit_02.sd b/parser/tst/simple_tests/dconf/ok_audit_02.sd
new file mode 100644
index 0000000..786557d
--- /dev/null
+++ b/parser/tst/simple_tests/dconf/ok_audit_02.sd
@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION dconf rule with audit
+#=EXRESULT PASS
+#
+
+profile ok_audit {
+  audit dconf rw /d/e/f,
+}
diff --git a/parser/tst/simple_tests/dconf/ok_dir_01.sd b/parser/tst/simple_tests/dconf/ok_dir_01.sd
index aa1f487..29c987a 100644
--- a/parser/tst/simple_tests/dconf/ok_dir_01.sd
+++ b/parser/tst/simple_tests/dconf/ok_dir_01.sd
@@ -4,5 +4,5 @@
 #
 
 profile ok_audit {
-  dconf /a/b/c/ r,
+  dconf r /a/b/c/,
 }
diff --git a/parser/tst/simple_tests/dconf/ok_dir_02.sd b/parser/tst/simple_tests/dconf/ok_dir_02.sd
new file mode 100644
index 0000000..52dbe5f
--- /dev/null
+++ b/parser/tst/simple_tests/dconf/ok_dir_02.sd
@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION dconf rule for dir
+#=EXRESULT PASS
+#
+
+profile ok_audit {
+  dconf rw /a/b/c/,
+}
diff --git a/parser/tst/simple_tests/dconf/ok_key_01.sd b/parser/tst/simple_tests/dconf/ok_key_01.sd
index 5ec92fa..b6c6f3f 100644
--- a/parser/tst/simple_tests/dconf/ok_key_01.sd
+++ b/parser/tst/simple_tests/dconf/ok_key_01.sd
@@ -4,5 +4,5 @@
 #
 
 profile ok_audit {
-  dconf /a/b/c r,
+  dconf r /a/b/c,
 }
-- 
2.9.3





More information about the AppArmor mailing list