From 84e2041d3b6d9a240ddab3f39c7baed8e718bfab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Tue, 20 Nov 2018 12:00:48 +0100 Subject: [PATCH 1/4] Prevent duplicate variables in Ansible Playbooks The same XCCDF value can appear in multiple rules, which means there can be same variable in multiple Ansible task. But when we generate a Playbook we should put the variable only once. Ansible produces a warning if there are multiple definitions of the same variable. --- src/XCCDF_POLICY/xccdf_policy_remediate.c | 180 ++++++++++++---------- 1 file changed, 95 insertions(+), 85 deletions(-) diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c index 65544c7f74..6ac566225c 100644 --- a/src/XCCDF_POLICY/xccdf_policy_remediate.c +++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c @@ -134,11 +134,11 @@ static int _write_remediation_to_fd_and_free(int output_fd, const char* template free(text); return 1; } - } - if (_write_text_to_fd(output_fd, "\n") != 0) { - free(text); - return 1; + if (_write_text_to_fd(output_fd, "\n") != 0) { + free(text); + return 1; + } } if (next_delim != NULL) { @@ -610,7 +610,7 @@ static int _write_fix_missing_warning_to_fd(const char *sys, int output_fd, stru } } -static inline int _xccdf_policy_rule_generate_fix_ansible(const char *template, int output_fd, const char *fix_text, bool ansible_variable_mode) +static inline int _parse_ansible_fix(const char *fix_text, struct oscap_list *variables, struct oscap_list *tasks) { #if defined USE_REGEX_PCRE // TODO: Tolerate different indentation styles in this regex @@ -649,69 +649,53 @@ static inline int _xccdf_policy_rule_generate_fix_ansible(const char *template, return 1; } - if (ansible_variable_mode) { - // ovector[0] and [1] hold the start and end of the whole needle match - // ovector[2] and [3] hold the start and end of the first capture group - // ovector[4] and [5] hold the start and end of the second capture group - char *variable_name = malloc((ovector[3] - ovector[2] + 1) * sizeof(char)); - memcpy(variable_name, &fix_text[ovector[2]], ovector[3] - ovector[2]); - variable_name[ovector[3] - ovector[2]] = '\0'; + // ovector[0] and [1] hold the start and end of the whole needle match + // ovector[2] and [3] hold the start and end of the first capture group + // ovector[4] and [5] hold the start and end of the second capture group + char *variable_name = malloc((ovector[3] - ovector[2] + 1) * sizeof(char)); + memcpy(variable_name, &fix_text[ovector[2]], ovector[3] - ovector[2]); + variable_name[ovector[3] - ovector[2]] = '\0'; - char *variable_value = malloc((ovector[5] - ovector[4] + 1) * sizeof(char)); - memcpy(variable_value, &fix_text[ovector[4]], ovector[5] - ovector[4]); - variable_value[ovector[5] - ovector[4]] = '\0'; + char *variable_value = malloc((ovector[5] - ovector[4] + 1) * sizeof(char)); + memcpy(variable_value, &fix_text[ovector[4]], ovector[5] - ovector[4]); + variable_value[ovector[5] - ovector[4]] = '\0'; - char *var_line = oscap_sprintf(" %s: %s", variable_name, variable_value); + char *var_line = oscap_sprintf(" %s: %s\n", variable_name, variable_value); - free(variable_name); - free(variable_value); + free(variable_name); + free(variable_value); - if (_write_remediation_to_fd_and_free(output_fd, template, var_line) != 0) { - pcre_free(re); - return 1; - } - } - else { - // Remarks: ovector doesn't contain values relative to start_offset, it contains - // absolute indices of fix_text. - const int length_between_matches = ovector[0] - start_offset; - char *remediation_part = malloc((length_between_matches + 1) * sizeof(char)); - memcpy(remediation_part, &fix_text[start_offset], length_between_matches); - remediation_part[length_between_matches] = '\0'; - if (_write_remediation_to_fd_and_free(output_fd, template, remediation_part) != 0) { - pcre_free(re); - return 1; - } + if (!oscap_list_contains(variables, var_line, (oscap_cmp_func) oscap_streq)) { + oscap_list_add(variables, var_line); } + // Remarks: ovector doesn't contain values relative to start_offset, it contains + // absolute indices of fix_text. + const int length_between_matches = ovector[0] - start_offset; + char *remediation_part = malloc((length_between_matches + 1) * sizeof(char)); + memcpy(remediation_part, &fix_text[start_offset], length_between_matches); + remediation_part[length_between_matches] = '\0'; + oscap_list_add(tasks, remediation_part); + start_offset = ovector[1]; // next time start after the entire pattern } - if (!ansible_variable_mode && fix_text_len - start_offset > 0) { + if (fix_text_len - start_offset > 0) { char *remediation_part = malloc((fix_text_len - start_offset + 1) * sizeof(char)); memcpy(remediation_part, &fix_text[start_offset], fix_text_len - start_offset); remediation_part[fix_text_len - start_offset] = '\0'; - - if (_write_remediation_to_fd_and_free(output_fd, template, remediation_part) != 0) { - pcre_free(re); - return 1; - } + oscap_list_add(tasks, remediation_part); } pcre_free(re); return 0; #else // TODO: Implement the post-process for posix regex as well - if (ansible_variable_mode) { - // this is not implemented so we don't write anything out for variables - return 0; - } - else - return _write_remediation_to_fd_and_free(output_fd, template, fix_text); + oscap_list_add(tasks, strdup(fix_text)); #endif } -static inline int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total, bool ansible_variable_mode) +static int _xccdf_policy_rule_get_fix_text(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, char **fix_text) { // Ensure that given Rule is selected and applicable (CPE). const bool is_selected = xccdf_policy_is_item_selected(policy, xccdf_rule_get_id(rule)); @@ -722,15 +706,8 @@ static inline int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, s // Find the most suitable fix. const struct xccdf_fix *fix = _find_fix_for_template(policy, rule, template); if (fix == NULL) { - int ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); - if (ret != 0) - return ret; - ret = _write_fix_missing_warning_to_fd(template, output_fd, rule); - if (ret != 0) - return ret; - ret = _write_fix_footer_to_fd(template, output_fd, rule); dI("No fix element was found for Rule/@id=\"%s\"", xccdf_rule_get_id(rule)); - return ret; + return 0; } dI("Processing a fix for Rule/@id=\"%s\"", xccdf_rule_get_id(rule)); @@ -744,37 +721,55 @@ static inline int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, s return res == 1; // Value 2 indicates warning. } // Refine. Resolve XML comments, CDATA and remaining elements - char *fix_text = NULL; - if (_xccdf_fix_decode_xml(cfix, &fix_text) != 0) { + if (_xccdf_fix_decode_xml(cfix, fix_text) != 0) { oscap_seterr(OSCAP_EFAMILY_OSCAP, "A fix element for Rule/@id=\"%s\" contains unresolved child elements.", xccdf_rule_get_id(rule)); xccdf_fix_free(cfix); return 1; } xccdf_fix_free(cfix); + return 0; +} - int ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); +static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total, bool ansible_variable_mode) +{ + char *fix_text = NULL; + int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text); + if (fix_text == NULL || ret != 0) { + ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); + if (ret != 0) { + return ret; + } + ret = _write_fix_missing_warning_to_fd(template, output_fd, rule); + if (ret != 0) { + return ret; + } + ret = _write_fix_footer_to_fd(template, output_fd, rule); + return ret; + } + ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); if (ret != 0) { free(fix_text); return ret; } + ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text); + if (ret != 0) { + return ret; + } + ret = _write_fix_footer_to_fd(template, output_fd, rule); + return ret; +} - if (oscap_streq(template, "urn:xccdf:fix:script:ansible")) { - // Ansible is special because we have two output modes, variable and - // task mode. In both cases we have to do post processing. - // We can't recover the return value (it is rewritten by a later call) to _write_fix_footer_to_fd - _xccdf_policy_rule_generate_fix_ansible(template, output_fd, fix_text, ansible_variable_mode); - free(fix_text); - } - else { - ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text); - if (ret != 0) { - return ret; - } +static int _xccdf_policy_rule_generate_ansible_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, struct oscap_list *variables, struct oscap_list *tasks) +{ + char *fix_text = NULL; + int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text); + if (fix_text == NULL) { + return ret; } - - ret = _write_fix_footer_to_fd(template, output_fd, rule); + ret = _parse_ansible_fix(fix_text, variables, tasks); + free(fix_text); return ret; } @@ -988,30 +983,45 @@ int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result * // In Ansible we have to generate variables first and then tasks if (strcmp(sys, "urn:xccdf:fix:script:ansible") == 0) { + struct oscap_list *variables = oscap_list_new(); + struct oscap_list *tasks = oscap_list_new(); + struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); + while (oscap_iterator_has_more(rules_to_fix_it)) { + struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); + ret = _xccdf_policy_rule_generate_ansible_fix(policy, rule, sys, variables, tasks); + if (ret != 0) + break; + } + oscap_iterator_free(rules_to_fix_it); + _write_text_to_fd(output_fd, " vars:\n"); + struct oscap_iterator *variables_it = oscap_iterator_new(variables); + while(oscap_iterator_has_more(variables_it)) { + char *var_line = (char *) oscap_iterator_next(variables_it); + _write_text_to_fd(output_fd, var_line); + } + oscap_iterator_free(variables_it); + oscap_list_free(variables, free); + _write_text_to_fd(output_fd, " tasks:\n"); + struct oscap_iterator *tasks_it = oscap_iterator_new(tasks); + while(oscap_iterator_has_more(tasks_it)) { + char *var_line = strdup((char *) oscap_iterator_next(tasks_it)); + _write_remediation_to_fd_and_free(output_fd, sys, var_line); + } + oscap_iterator_free(tasks_it); + oscap_list_free(tasks, free); + } else { unsigned int current = 1; struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); while (oscap_iterator_has_more(rules_to_fix_it)) { struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); - ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, true); + ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, false); if (ret != 0) break; } oscap_iterator_free(rules_to_fix_it); - - _write_text_to_fd(output_fd, " tasks:\n"); - } - - unsigned int current = 1; - struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); - while (oscap_iterator_has_more(rules_to_fix_it)) { - struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); - ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, false); - if (ret != 0) - break; } - oscap_iterator_free(rules_to_fix_it); oscap_list_free(rules_to_fix, NULL); From 8badfbf30f423ca2f9452ab3ac6daf8bb8d850e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Thu, 22 Nov 2018 13:43:40 +0100 Subject: [PATCH 2/4] Remove unused parameter This parameter is not used in this function. The function is static and it is used only at one place, so we can safely omit this parameter. --- src/XCCDF_POLICY/xccdf_policy_remediate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c index 6ac566225c..520e9ff4de 100644 --- a/src/XCCDF_POLICY/xccdf_policy_remediate.c +++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c @@ -731,7 +731,7 @@ static int _xccdf_policy_rule_get_fix_text(struct xccdf_policy *policy, struct x return 0; } -static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total, bool ansible_variable_mode) +static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total) { char *fix_text = NULL; int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text); @@ -1016,7 +1016,7 @@ int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result * struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); while (oscap_iterator_has_more(rules_to_fix_it)) { struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); - ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, false); + ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total); if (ret != 0) break; } From 90e0354c3eef165d8b467e6d843e9dfe1d264416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Mon, 26 Nov 2018 18:18:47 +0100 Subject: [PATCH 3/4] Refactor: Extract functions To improve the readability of the code we can refactor the code that is specific to Ansible to a separate method. We can also make a similar change in the `else` branch. --- src/XCCDF_POLICY/xccdf_policy_remediate.c | 94 +++++++++++++---------- 1 file changed, 54 insertions(+), 40 deletions(-) diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c index 520e9ff4de..bf36523dfd 100644 --- a/src/XCCDF_POLICY/xccdf_policy_remediate.c +++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c @@ -929,6 +929,56 @@ static int _write_script_header_to_fd(struct xccdf_policy *policy, struct xccdf_ } } +static int _xccdf_policy_generate_fix_ansible(struct oscap_list *rules_to_fix, struct xccdf_policy *policy, const char *sys, int output_fd) +{ + int ret = 0; + struct oscap_list *variables = oscap_list_new(); + struct oscap_list *tasks = oscap_list_new(); + struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); + while (oscap_iterator_has_more(rules_to_fix_it)) { + struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); + ret = _xccdf_policy_rule_generate_ansible_fix(policy, rule, sys, variables, tasks); + if (ret != 0) + break; + } + oscap_iterator_free(rules_to_fix_it); + + _write_text_to_fd(output_fd, " vars:\n"); + struct oscap_iterator *variables_it = oscap_iterator_new(variables); + while(oscap_iterator_has_more(variables_it)) { + char *var_line = (char *) oscap_iterator_next(variables_it); + _write_text_to_fd(output_fd, var_line); + } + oscap_iterator_free(variables_it); + oscap_list_free(variables, free); + + _write_text_to_fd(output_fd, " tasks:\n"); + struct oscap_iterator *tasks_it = oscap_iterator_new(tasks); + while(oscap_iterator_has_more(tasks_it)) { + char *var_line = strdup((char *) oscap_iterator_next(tasks_it)); + _write_remediation_to_fd_and_free(output_fd, sys, var_line); + } + oscap_iterator_free(tasks_it); + oscap_list_free(tasks, free); + return ret; +} + +static int _xccdf_policy_generate_fix_other(struct oscap_list *rules_to_fix, struct xccdf_policy *policy, const char *sys, int output_fd) +{ + int ret = 0; + const unsigned int total = oscap_list_get_itemcount(rules_to_fix); + unsigned int current = 1; + struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); + while (oscap_iterator_has_more(rules_to_fix_it)) { + struct xccdf_rule *rule = (struct xccdf_rule *) oscap_iterator_next(rules_to_fix_it); + ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total); + if (ret != 0) + break; + } + oscap_iterator_free(rules_to_fix_it); + return ret; +} + int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result *result, const char *sys, int output_fd) { __attribute__nonnull__(policy); @@ -979,48 +1029,12 @@ int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result * xccdf_rule_result_iterator_free(rr_it); } - const unsigned int total = oscap_list_get_itemcount(rules_to_fix); - - // In Ansible we have to generate variables first and then tasks + // Ansible Playbooks are generated using a different function because + // in Ansible we have to generate variables first and then tasks if (strcmp(sys, "urn:xccdf:fix:script:ansible") == 0) { - struct oscap_list *variables = oscap_list_new(); - struct oscap_list *tasks = oscap_list_new(); - struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); - while (oscap_iterator_has_more(rules_to_fix_it)) { - struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); - ret = _xccdf_policy_rule_generate_ansible_fix(policy, rule, sys, variables, tasks); - if (ret != 0) - break; - } - oscap_iterator_free(rules_to_fix_it); - - _write_text_to_fd(output_fd, " vars:\n"); - struct oscap_iterator *variables_it = oscap_iterator_new(variables); - while(oscap_iterator_has_more(variables_it)) { - char *var_line = (char *) oscap_iterator_next(variables_it); - _write_text_to_fd(output_fd, var_line); - } - oscap_iterator_free(variables_it); - oscap_list_free(variables, free); - - _write_text_to_fd(output_fd, " tasks:\n"); - struct oscap_iterator *tasks_it = oscap_iterator_new(tasks); - while(oscap_iterator_has_more(tasks_it)) { - char *var_line = strdup((char *) oscap_iterator_next(tasks_it)); - _write_remediation_to_fd_and_free(output_fd, sys, var_line); - } - oscap_iterator_free(tasks_it); - oscap_list_free(tasks, free); + ret = _xccdf_policy_generate_fix_ansible(rules_to_fix, policy, sys, output_fd); } else { - unsigned int current = 1; - struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix); - while (oscap_iterator_has_more(rules_to_fix_it)) { - struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it); - ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total); - if (ret != 0) - break; - } - oscap_iterator_free(rules_to_fix_it); + ret = _xccdf_policy_generate_fix_other(rules_to_fix, policy, sys, output_fd); } oscap_list_free(rules_to_fix, NULL); From 2a11da0a6a1643669caadb72f1a217fafcfea6d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= Date: Wed, 28 Nov 2018 14:19:12 +0100 Subject: [PATCH 4/4] Refactor: Remove duplicate code --- src/XCCDF_POLICY/xccdf_policy_remediate.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c index bf36523dfd..15f9453938 100644 --- a/src/XCCDF_POLICY/xccdf_policy_remediate.c +++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c @@ -733,26 +733,17 @@ static int _xccdf_policy_rule_get_fix_text(struct xccdf_policy *policy, struct x static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total) { + int ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); + if (ret != 0) { + return ret; + } char *fix_text = NULL; - int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text); + ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text); if (fix_text == NULL || ret != 0) { - ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); - if (ret != 0) { - return ret; - } ret = _write_fix_missing_warning_to_fd(template, output_fd, rule); - if (ret != 0) { - return ret; - } - ret = _write_fix_footer_to_fd(template, output_fd, rule); - return ret; - } - ret = _write_fix_header_to_fd(template, output_fd, rule, current, total); - if (ret != 0) { - free(fix_text); - return ret; + } else { + ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text); } - ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text); if (ret != 0) { return ret; }