From 891fa8079b446e51e1f86583c6972249bbea053d Mon Sep 17 00:00:00 2001 From: Elena Reshetova Date: Fri, 5 Jul 2013 14:48:42 +0300 Subject: Security plugin: allowing multiple domains definition - allowing multiple domains definition per manifest - fixing indirect include of config.h - restricting adding new sw source with the same key info --- plugins/msm-plugin.c | 15 ++--- plugins/msm.h | 9 ++- plugins/msmmanifest.c | 106 +++++++++++++++++++++------------ plugins/msmxattr.c | 162 +++++++++++++++++++++++++++----------------------- 4 files changed, 170 insertions(+), 122 deletions(-) diff --git a/plugins/msm-plugin.c b/plugins/msm-plugin.c index cc79bd00e..f9213293b 100644 --- a/plugins/msm-plugin.c +++ b/plugins/msm-plugin.c @@ -25,9 +25,6 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA * 02110-1301 USA */ - -#include "plugin.h" -#include "debug.h" #include #include @@ -585,16 +582,16 @@ rpmRC PLUGINHOOK_PSM_PRE_FUNC(rpmte te) goto fail; } } - if (ctx->mfx->define) { - if (ctx->mfx->define->name) - smackLabel = 1; - ret = msmSetupDefine(ctx->smack_accesses, ctx->mfx); + if (ctx->mfx->defines) { + ret = msmSetupDefines(ctx->smack_accesses, ctx->mfx); if (ret) { rpmlog(RPMLOG_ERR, "AC domain setup failed for %s\n", rpmteN(ctx->te)); msmCancelPackage(ctx->mfx->name); goto fail; - } + } else { + smackLabel = 1; + } } if (ctx->mfx->request) { if (ctx->mfx->request->ac_domain) @@ -776,7 +773,7 @@ rpmRC PLUGINHOOK_PSM_POST_FUNC(rpmte te, int rpmrc) } else { rpmlog(RPMLOG_DEBUG, "removing %s manifest data\n", rpmteN(ctx->te)); - if (ctx->mfx->define || ctx->mfx->provides || ctx->mfx->sw_sources) { + if (ctx->mfx->defines || ctx->mfx->provides || ctx->mfx->sw_sources) { msmRemoveRules(ctx->smack_accesses, ctx->mfx, SmackEnabled); } msmRemoveConfig(ctx->mfx); diff --git a/plugins/msm.h b/plugins/msm.h index ffaddd3ec..4a0423d9a 100644 --- a/plugins/msm.h +++ b/plugins/msm.h @@ -52,6 +52,9 @@ #define DBUS_METHOD 4 #define DBUS_SIGNAL 5 +#include "plugin.h" +#include "debug.h" + #include #include #include @@ -248,6 +251,8 @@ typedef struct define_x { struct d_request_x *d_requests; struct d_permit_x *d_permits; struct d_provide_x *d_provides; + struct define_x *prev; + struct define_x *next; } define_x; typedef struct package_x { @@ -285,7 +290,7 @@ typedef struct manifest_x { /*package manifest */ struct provide_x *provides; /* assign section */ struct request_x *request; /* request section */ struct sw_source_x *sw_sources; /*defined software sources(non-NULL only for configuration manifests)*/ - struct define_x *define; /* define section */ + struct define_x *defines; /* define section(s) */ struct file_x *files; /* installed files */ } manifest_x; @@ -353,7 +358,7 @@ int msmSetupPackages(struct smack_accesses *smack_accesses, package_x *packages, * @param mfx package manifest * @return 0 on success, else -1 */ -int msmSetupDefine(struct smack_accesses *smack_accesses, manifest_x *mfx); +int msmSetupDefines(struct smack_accesses *smack_accesses, manifest_x *mfx); /** \ingroup msm * Setup smack rules according to the manifest diff --git a/plugins/msmmanifest.c b/plugins/msmmanifest.c index 7eae7c599..c7139237e 100644 --- a/plugins/msmmanifest.c +++ b/plugins/msmmanifest.c @@ -822,7 +822,24 @@ static int msmProcessDefine(xmlTextReaderPtr reader, define_x *define, manifest_ return ret; } -static int msmProcessKeyinfo(xmlTextReaderPtr reader, origin_x *origin) +static int findSWSourceByKey(sw_source_x *sw_source, void *param) +{ + origin_x *origin; + keyinfo_x *keyinfo; + keyinfo_x *current_keyinfo = (keyinfo_x*)param; + + for (origin = sw_source->origins; origin; origin = origin->prev) { + for (keyinfo = origin->keyinfos; keyinfo; keyinfo = keyinfo->prev) { + if (strncmp((const char*)current_keyinfo->keydata, (const char*)keyinfo->keydata, + strlen((const char*)current_keyinfo->keydata)) == 0 + && (current_keyinfo->keylen == keyinfo->keylen)) + return 0; + } + } + return 1; +} + +static int msmProcessKeyinfo(xmlTextReaderPtr reader, origin_x *origin, sw_source_x *parent) { const xmlChar *keydata; keyinfo_x *keyinfo; @@ -839,6 +856,14 @@ static int msmProcessKeyinfo(xmlTextReaderPtr reader, origin_x *origin) rpmlog(RPMLOG_ERR, "Failed to decode keyinfo %s, %d\n", keydata, ret); ret = -1; } + if (!ret) { + // check that keys aren't matching + sw_source_x *old = msmSWSourceTreeTraversal(parent, findSWSourceByKey, (void *)keyinfo, NULL); + if (old) { + rpmlog(RPMLOG_ERR, "SW source with this key has already been installed\n"); + return -1; + } + } LISTADD(origin->keyinfos, keyinfo); } else return -1; @@ -868,7 +893,7 @@ static access_x *msmProcessAccess(xmlTextReaderPtr reader, origin_x *origin) return NULL; } -static int msmProcessOrigin(xmlTextReaderPtr reader, origin_x *origin) +static int msmProcessOrigin(xmlTextReaderPtr reader, origin_x *origin, sw_source_x *parent) { const xmlChar *node, *type; int ret, depth; @@ -882,7 +907,7 @@ static int msmProcessOrigin(xmlTextReaderPtr reader, origin_x *origin) node = xmlTextReaderConstName(reader); if (!node) return -1; if (!strcmp(ASCII(node), "keyinfo")) { - ret = msmProcessKeyinfo(reader, origin); + ret = msmProcessKeyinfo(reader, origin, parent); } else if (!strcmp(ASCII(node), "access")) { access_x *access = msmProcessAccess(reader, origin); if (access) { @@ -1022,7 +1047,7 @@ static int msmProcessSWSource(xmlTextReaderPtr reader, sw_source_x *sw_source, c origin_x *origin = calloc(1, sizeof(origin_x)); if (origin) { LISTADD(sw_source->origins, origin); - ret = msmProcessOrigin(reader, origin); + ret = msmProcessOrigin(reader, origin, sw_source->parent); } else return -1; } else if (!strcmp(ASCII(node), "package")) { /* config processing */ @@ -1093,7 +1118,7 @@ static int msmProcessMsm(xmlTextReaderPtr reader, manifest_x *mfx, sw_source_x * { const xmlChar *node; int ret, depth; - int assignPresent = 0, requestPresent = 0, definePresent = 0, attributesPresent = 0; /* there must be only one section per manifest */ + int assignPresent = 0, requestPresent = 0, attributesPresent = 0; /* there must be only one section per manifest */ mfx->sw_source = current; rpmlog(RPMLOG_DEBUG, "manifest\n"); @@ -1121,14 +1146,10 @@ static int msmProcessMsm(xmlTextReaderPtr reader, manifest_x *mfx, sw_source_x * attributesPresent = 1; ret = msmProcessAttributes(reader, mfx); } else if (!strcmp(ASCII(node), "define")) { - if (definePresent) { - rpmlog(RPMLOG_ERR, "A second request section in manifest isn't allowed. Abort installation.\n"); - return -1; - } - definePresent = 1; - mfx->define = calloc(1, sizeof(define_x)); - if (mfx->define) { - ret = msmProcessDefine(reader, mfx->define, mfx, current); + define_x *define = calloc(1, sizeof(define_x)); + if (define) { + LISTADD(mfx->defines, define); + ret = msmProcessDefine(reader, define, mfx, current); } else return -1; } else if (!strcmp(ASCII(node), "request")) { if (requestPresent) { @@ -1416,14 +1437,42 @@ static d_provide_x *msmFreeDProvide(d_provide_x *d_provide) return next; } +static define_x *msmFreeDefine(define_x *define) +{ + define_x *next = define->next; + d_request_x *d_request; + d_permit_x *d_permit; + d_provide_x *d_provide; + + msmFreePointer((void**)&define->name); + msmFreePointer((void**)&define->policy); + msmFreePointer((void**)&define->plist); + + if (define->d_requests) { + LISTHEAD(define->d_requests, d_request); + for (; d_request; d_request = msmFreeDRequest(d_request)); + } + rpmlog(RPMLOG_DEBUG, "after freeing define requests\n"); + if (define->d_permits) { + LISTHEAD(define->d_permits, d_permit); + for (; d_permit; d_permit = msmFreeDPermit(d_permit)); + } + rpmlog(RPMLOG_DEBUG, "after freeing define permits\n"); + if (define->d_provides) { + LISTHEAD(define->d_provides, d_provide); + for (; d_provide; d_provide = msmFreeDProvide(d_provide)); + } + rpmlog(RPMLOG_DEBUG, "after freeing provides\n"); + msmFreePointer((void**)&define); + return next; +} + manifest_x* msmFreeManifestXml(manifest_x* mfx) { provide_x *provide; file_x *file; sw_source_x *sw_source; - d_request_x *d_request; - d_permit_x *d_permit; - d_provide_x *d_provide; + define_x *define; rpmlog(RPMLOG_DEBUG, "in msmFreeManifestXml\n"); if (mfx) { @@ -1443,27 +1492,10 @@ manifest_x* msmFreeManifestXml(manifest_x* mfx) } msmFreePointer((void**)&mfx->name); rpmlog(RPMLOG_DEBUG, "after freeing name\n"); - if (mfx->define) { - msmFreePointer((void**)&mfx->define->name); - msmFreePointer((void**)&mfx->define->policy); - msmFreePointer((void**)&mfx->define->plist); - if (mfx->define->d_requests) { - LISTHEAD(mfx->define->d_requests, d_request); - for (; d_request; d_request = msmFreeDRequest(d_request)); - } - rpmlog(RPMLOG_DEBUG, "after freeing define requests\n"); - if (mfx->define->d_permits) { - LISTHEAD(mfx->define->d_permits, d_permit); - for (; d_permit; d_permit = msmFreeDPermit(d_permit)); - } - rpmlog(RPMLOG_DEBUG, "after freeing define permits\n"); - if (mfx->define->d_provides) { - LISTHEAD(mfx->define->d_provides, d_provide); - for (; d_provide; d_provide = msmFreeDProvide(d_provide)); - } - rpmlog(RPMLOG_DEBUG, "after freeing provides\n"); - msmFreePointer((void**)&mfx->define); - } + if (mfx->defines) { + LISTHEAD(mfx->defines, define); + for (; define; define = msmFreeDefine(define)); + } rpmlog(RPMLOG_DEBUG, "after freeing defines \n"); msmFreePointer((void**)&mfx); } diff --git a/plugins/msmxattr.c b/plugins/msmxattr.c index fc8af6b4f..5af6ed046 100644 --- a/plugins/msmxattr.c +++ b/plugins/msmxattr.c @@ -101,12 +101,15 @@ static int msmCheckLabelProvisioning(manifest_x *mfx, const char* label) { d_provide_x *provide = NULL; + define_x *define = NULL; - if ((mfx) && (label) && (mfx->define) && (mfx->define->d_provides)) { - for (provide = mfx->define->d_provides; provide; provide = provide->prev) { - if (strcmp(provide->label_name, label) == 0) - return 0; - } + if ((mfx) && (label) && (mfx->defines)) { + for (define = mfx->defines; define; define = define->prev) { + for (provide = define->d_provides; provide; provide = provide->prev) { + if (strcmp(provide->label_name, label) == 0) + return 0; + } + } } rpmlog(RPMLOG_ERR, "Label %s hasn't been provided in the manifest\n", label); return -1; @@ -713,6 +716,7 @@ static int msmCheckDomainJoinPossibility(manifest_x *mfx, ac_domain_x *defined_a int msmSetupRequests(manifest_x *mfx) { ac_domain_x *defined_ac_domain = NULL; + define_x *define = NULL; if ((!mfx) || (!mfx->request) || (!mfx->request->ac_domain)) return -1; @@ -727,11 +731,15 @@ int msmSetupRequests(manifest_x *mfx) #endif } //now check that the package can join the requested AC domain - if (mfx->define){ - rpmlog(RPMLOG_DEBUG, "mfx->define->name %s mfx->request->ac_domain %s\n", mfx->define->name, mfx->request->ac_domain); - if (strcmp(mfx->define->name, mfx->request->ac_domain) == 0) - //ac domain is requested from the same package where it was define. This case is always allowed - return 0; + if (mfx->defines){ + LISTHEAD(mfx->defines, define); + while(define) { + rpmlog(RPMLOG_DEBUG, "define->name %s mfx->request->ac_domain %s\n", define->name, mfx->request->ac_domain); + if (strcmp(define->name, mfx->request->ac_domain) == 0) + //ac domain is requested from the same package where it was define. This case is always allowed + return 0; + define = define->next; + } } //need to check if developer allowed other packages to join this domain if (msmCheckDomainJoinPossibility(mfx, defined_ac_domain) < 0) { @@ -819,6 +827,7 @@ int msmSetupDBusPolicies(package_x *package, manifest_x *mfx) static int msmCheckDomainRequestOrPermit(manifest_x *mfx, const char* domain) { ac_domain_x *defined_ac_domain = NULL; + define_x *define = NULL; char* name = NULL; if ((!mfx) || (!domain)) @@ -838,14 +847,18 @@ static int msmCheckDomainRequestOrPermit(manifest_x *mfx, const char* domain) } //now check that this ac_domain can be requested - if ((mfx->define) && (mfx->define->name)) { - rpmlog(RPMLOG_DEBUG, "mfx->define->name %s domain %s\n", mfx->define->name, name); - if (strcmp(mfx->define->name, name) == 0) { - // AC domain access is requested or permitted from the same package where it was defined. - // This case is always allowed - msmFreePointer((void**)&name); - return 0; - } + if (mfx->defines) { + LISTHEAD(mfx->defines, define); + while (define) { + rpmlog(RPMLOG_DEBUG, "define->name %s domain %s\n", define->name, name); + if (strcmp(define->name, name) == 0) { + // AC domain access is requested or permitted from the same package where it was defined. + // This case is always allowed + msmFreePointer((void**)&name); + return 0; + } + define = define->next; + } } // no need to check if developer allowed other packages to request/permit this domain @@ -862,68 +875,81 @@ static int msmCheckDomainRequestOrPermit(manifest_x *mfx, const char* domain) } } -int msmSetupDefine(struct smack_accesses *smack_accesses, manifest_x *mfx) +int msmSetupDefines(struct smack_accesses *smack_accesses, manifest_x *mfx) { d_request_x *d_request; + define_x *define; d_permit_x *d_permit; ac_domain_x * defined_ac_domain = NULL; int ret; - if ( (!mfx) || (!mfx->define) || (!mfx->define->name)) { - rpmlog(RPMLOG_ERR, "Failed to setup define with empty name\n"); - return -1; + if ( (!mfx) || (!mfx->defines)) { + rpmlog(RPMLOG_ERR, "Failed to setup define\n"); + return -1; } - /* need to check if domain hasn't been already defined by other package */ + LISTHEAD(mfx->defines, define); - HASH_FIND(hh, all_ac_domains, mfx->define->name, strlen(mfx->define->name), defined_ac_domain); - if ((defined_ac_domain) && (defined_ac_domain->pkg_name)) { // this domain has been previously defined - if (strcmp(defined_ac_domain->pkg_name, mfx->name) != 0) { - rpmlog(RPMLOG_ERR, "Attempt to define a domain name %s that has been already defined by package %s\n", - mfx->define->name, defined_ac_domain->pkg_name); + while (define) { + define_x *next = define->next; + if (!define->name) { + rpmlog(RPMLOG_ERR, "Attempt to define a domain with empty name. Abort\n"); return -1; } - } + /* need to check if domain hasn't been already defined by other package */ - if (mfx->define->d_requests) { - for (d_request = mfx->define->d_requests; d_request; d_request = d_request->prev) { - // first check if the current's package sw source can grant access to requested domain - if (msmCheckDomainRequestOrPermit(mfx, d_request->label_name) < 0) { -#ifdef ENABLE_DCHECKS + HASH_FIND(hh, all_ac_domains, define->name, strlen(define->name), defined_ac_domain); + if ((defined_ac_domain) && (defined_ac_domain->pkg_name)) { // this domain has been previously defined + if (strcmp(defined_ac_domain->pkg_name, mfx->name) != 0) { + rpmlog(RPMLOG_ERR, "Attempt to define a domain name %s that has been already defined by package %s\n", + define->name, defined_ac_domain->pkg_name); return -1; -#endif } - if (smack_accesses_add(smack_accesses, mfx->define->name, d_request->label_name, d_request->ac_type) < 0) { - rpmlog(RPMLOG_ERR, "Failed to set smack rules for domain requests\n"); - return -1; - } } - } - if (mfx->define->d_permits) { - for (d_permit = mfx->define->d_permits; d_permit; d_permit = d_permit->prev) { - // first check if the current's package sw source can grant access to permited domain - if (msmCheckDomainRequestOrPermit(mfx, d_permit->label_name) < 0) { + if (define->d_requests) { + for (d_request = define->d_requests; d_request; d_request = d_request->prev) { + // first check if the current's package sw source can grant access to requested domain + if (msmCheckDomainRequestOrPermit(mfx, d_request->label_name) < 0) { #ifdef ENABLE_DCHECKS - return -1; + return -1; #endif + } + if (smack_accesses_add(smack_accesses, define->name, d_request->label_name, d_request->ac_type) < 0) { + rpmlog(RPMLOG_ERR, "Failed to set smack rules for domain requests\n"); + return -1; + } } - if (!d_permit->to_label_name) - ret = smack_accesses_add(smack_accesses, d_permit->label_name, mfx->define->name, d_permit->ac_type); - else { - if (msmCheckLabelProvisioning(mfx, d_permit->to_label_name) < 0) { + } + + if (define->d_permits) { + for (d_permit = define->d_permits; d_permit; d_permit = d_permit->prev) { + // first check if the current's package sw source can grant access to permited domain + if (msmCheckDomainRequestOrPermit(mfx, d_permit->label_name) < 0) { #ifdef ENABLE_DCHECKS return -1; #endif } - ret = smack_accesses_add(smack_accesses, d_permit->label_name, d_permit->to_label_name, d_permit->ac_type); + if (!d_permit->to_label_name) + ret = smack_accesses_add(smack_accesses, d_permit->label_name, define->name, d_permit->ac_type); + else { + if (msmCheckLabelProvisioning(mfx, d_permit->to_label_name) < 0) { +#ifdef ENABLE_DCHECKS + return -1; +#endif + } + ret = smack_accesses_add(smack_accesses, d_permit->label_name, d_permit->to_label_name, d_permit->ac_type); + } + if (ret < 0) { + rpmlog(RPMLOG_ERR, "Failed to set smack rules for domain permits\n"); + return -1; + } } - if (ret < 0) { - rpmlog(RPMLOG_ERR, "Failed to set smack rules for domain permits\n"); - return -1; - } - } - } + } + + define = next; + } + return 0; } @@ -1019,9 +1045,8 @@ int msmSetupPackages(struct smack_accesses *smack_accesses, package_x *packages, char *p_rankkey, *c_rankkey; for (package = packages; package; package = package->prev) { package_x *current_p; - rpmlog(RPMLOG_DEBUG, "before HASH_FIND, package->name %s\n", package->name); + rpmlog(RPMLOG_DEBUG, "before HASH_FIND, package->name %s\n", package->name); HASH_FIND(hh, allpackages, package->name, strlen(package->name), current_p); - rpmlog(RPMLOG_DEBUG, "after HASH_FIND\n"); if (current_p) { if (!current_p->sw_source) { return -1; @@ -1203,13 +1228,12 @@ int msmSetFileXAttributes(manifest_x *mfx, const char* filepath, magic_t cookie) if (exec_label) { execLabeldefined = 1; if ((strcmp(exec_label, "none") == 0) - || ( (mfx->request) && (mfx->request->ac_domain) && (strcmp(exec_label, mfx->request->ac_domain) == 0)) - || ( (mfx->define) && (mfx->define->name) && (strcmp(exec_label, mfx->define->name) == 0))) { + || ( (mfx->request) && (mfx->request->ac_domain) && (strcmp(exec_label, mfx->request->ac_domain) == 0))) { // these labels are allowed } else { // ignore all other exec labels, because they aren't allowed for security reasons exec_label = NULL; - rpmlog(RPMLOG_DEBUG, "It isn't allowed to label the file with smack64label other than ac domain or \"none\" value\n"); + rpmlog(RPMLOG_DEBUG, "It isn't allowed to label the file with smack64label other than requested ac domain or \"none\" value\n"); rpmlog(RPMLOG_DEBUG, "The default ac domain label will be used instead\n"); } } @@ -1225,18 +1249,8 @@ int msmSetFileXAttributes(manifest_x *mfx, const char* filepath, magic_t cookie) if (!label) label = isolatedLabel; if (!exec_label) exec_label = isolatedLabel; } - } else if (mfx->define) { // AC domain defined in manifest - if (mfx->define->name) { - if (!label) label = mfx->define->name; - if (!exec_label) exec_label = mfx->define->name; - } else { - rpmlog(RPMLOG_DEBUG, "Define for AC domain is empty. Can't identify default file label\n"); - rpmlog(RPMLOG_DEBUG, "File will be labelled with the label \"Isolated\"\n"); - if (!label) label = isolatedLabel; - if (!exec_label) exec_label = isolatedLabel; - } - } else { // no request or definition of domain - rpmlog(RPMLOG_DEBUG, "Both define and request sections are empty. Can't identify default file label\n"); + } else { // no request of domain + rpmlog(RPMLOG_DEBUG, "The request section is missing. Can't identify default file label\n"); rpmlog(RPMLOG_DEBUG, "File will be labelled with the label \"Isolated\"\n"); if (!label) label = isolatedLabel; if (!exec_label) exec_label = isolatedLabel; @@ -1287,7 +1301,7 @@ void msmRemoveRules(struct smack_accesses *smack_accesses, manifest_x *mfx, int if (!package) return; - if ((mfx->define) || (mfx->sw_sources)) { + if ((mfx->defines) || (mfx->sw_sources)) { /* remove smack rule file and rule set from kernel */ rpmlog(RPMLOG_DEBUG, "removing smack rules for %s\n", mfx->name); msmSetupSmackRules(smack_accesses, mfx->name, SMACK_UNINSTALL, SmackEnabled); -- cgit v1.2.3