fix(audit): security, correctness, refactors, docs (no wire change) #306
+39
-15
@@ -162,33 +162,57 @@ static bool is_modifier_char(char c) {
|
||||
return c == 's' || c == 'r' || c == 'p' || c == 'x' || c == '/' || c == '!' || c == 'C';
|
||||
}
|
||||
|
||||
/* Modifiers rsync defines but FastSync does not implement. They must still be
|
||||
* consumed as part of the modifier run so they are rejected explicitly instead
|
||||
* of leaking into the pattern (which produced misleading failures such as
|
||||
* "could not read merge file 'n file'"). */
|
||||
/* merge/dir-merge rules are the only rules rsync accepts the merge-file
|
||||
* modifiers on. */
|
||||
static bool is_merge_rule(RuleKind kind) {
|
||||
return kind == RULE_KIND_MERGE || kind == RULE_KIND_DIR_MERGE;
|
||||
}
|
||||
|
||||
/* Merge-file modifiers rsync defines but FastSync does not implement:
|
||||
* 'e' exclude the merge file itself, 'n' do not inherit the merge file, 'w'
|
||||
* word-split the merge file. They are recognized as part of a modifier run on
|
||||
* every rule (so a pure e/n/w token is rejected rather than folded into the
|
||||
* pattern), but are accepted (and ignored) only on merge/dir-merge rules. */
|
||||
static bool is_unsupported_modifier_char(char c) {
|
||||
return c == 'e' || c == 'n' || c == 'w';
|
||||
}
|
||||
|
||||
/* Characters that are part of a modifier run, whether supported or not. */
|
||||
static bool is_modifier_scan_char(char c) {
|
||||
return is_modifier_char(c) || is_unsupported_modifier_char(c);
|
||||
/* Merge-file modifiers rsync accepts on merge/dir-merge rules: 'e', 'n', 'w'
|
||||
* and '-' (do not transfer the merge file). */
|
||||
static bool is_merge_modifier_char(char c) {
|
||||
return c == 'e' || c == 'n' || c == 'w' || c == '-';
|
||||
}
|
||||
|
||||
/* Characters that count as part of a modifier run for `kind` when deciding
|
||||
* whether a token is a pure modifier run. e/n/w count on every rule so that a
|
||||
* pure e/n/w token is rejected on non-merge rules; '-' only on merge rules. */
|
||||
static bool is_modifier_scan_char(char c, RuleKind kind) {
|
||||
return is_modifier_char(c) || is_unsupported_modifier_char(c) ||
|
||||
(is_merge_rule(kind) && is_merge_modifier_char(c));
|
||||
}
|
||||
|
||||
/* Characters actually consumed as modifiers for `kind`. The merge-file
|
||||
* modifiers are consumed only on merge/dir-merge rules; elsewhere e/n/w fall
|
||||
* through to the pattern (so mixed tokens such as "H,!secret" keep their
|
||||
* historical "ecret" pattern). */
|
||||
static bool is_consumed_modifier_char(char c, RuleKind kind) {
|
||||
return is_modifier_char(c) || (is_merge_rule(kind) && is_merge_modifier_char(c));
|
||||
}
|
||||
|
||||
/* Inspect the token that follows a rule name (up to the first space/underscore
|
||||
* or the end). If the token is composed *solely* of modifier characters and
|
||||
* includes one FastSync does not implement, it is unambiguously a modifier run:
|
||||
* includes one that is invalid for `kind`, it is unambiguously a modifier run:
|
||||
* return that character so the caller can reject it. A token that contains any
|
||||
* non-modifier character is a pattern (e.g. "-newfile") and returns '\0', which
|
||||
* keeps the historical parsing of mixed tokens such as "H,!secret" intact. */
|
||||
static char unsupported_modifier_in_token(const char* tok) {
|
||||
static char unsupported_modifier_in_token(const char* tok, RuleKind kind) {
|
||||
if (*tok == '\0' || *tok == ' ' || *tok == '_')
|
||||
return '\0';
|
||||
char bad = '\0';
|
||||
for (const char* q = tok; *q != '\0' && *q != ' ' && *q != '_'; q++) {
|
||||
if (!is_modifier_scan_char(*q))
|
||||
if (!is_modifier_scan_char(*q, kind))
|
||||
return '\0';
|
||||
if (is_unsupported_modifier_char(*q))
|
||||
if (!is_merge_rule(kind) && is_unsupported_modifier_char(*q))
|
||||
bad = *q;
|
||||
}
|
||||
return bad;
|
||||
@@ -238,9 +262,9 @@ static bool parse_rule_syntax(const char* text, RuleKind* kind, unsigned* sides,
|
||||
Only commit a modifier run that terminates at a separator or the end, so a
|
||||
pattern such as "*.tmp" written as "-*.tmp" is not mistaken for modifiers. */
|
||||
if (*p == ',') {
|
||||
*bad_mod = unsupported_modifier_in_token(p + 1);
|
||||
*bad_mod = unsupported_modifier_in_token(p + 1, *kind);
|
||||
} else if (is_short) {
|
||||
*bad_mod = unsupported_modifier_in_token(p);
|
||||
*bad_mod = unsupported_modifier_in_token(p, *kind);
|
||||
}
|
||||
if (*bad_mod != '\0')
|
||||
return false;
|
||||
@@ -250,12 +274,12 @@ static bool parse_rule_syntax(const char* text, RuleKind* kind, unsigned* sides,
|
||||
if (*p == ',') {
|
||||
p++;
|
||||
mod_start = p;
|
||||
while (is_modifier_char(*p))
|
||||
while (is_consumed_modifier_char(*p, *kind))
|
||||
p++;
|
||||
mod_end = p;
|
||||
} else if (is_short) {
|
||||
const char* scan = p;
|
||||
while (is_modifier_char(*scan))
|
||||
while (is_consumed_modifier_char(*scan, *kind))
|
||||
scan++;
|
||||
if (*scan == '\0' || *scan == ' ' || *scan == '_') {
|
||||
mod_start = p;
|
||||
|
||||
+6
-2
@@ -24,8 +24,12 @@
|
||||
* clear/! clear the current rule list (takes no argument)
|
||||
* Modifiers: '/' absolute anchor, '!' negate match, 'C' inject CVS defaults,
|
||||
* 's' sender side, 'r' receiver side, 'p' perishable. The rsync 'x'
|
||||
* (xattr-name) modifier and the merge-only 'e'/'n'/'w' modifiers are not
|
||||
* implemented and are rejected explicitly.
|
||||
* (xattr-name) modifier is not implemented and is rejected explicitly
|
||||
* everywhere. The merge-file modifiers 'e' (exclude the merge file itself),
|
||||
* 'n' (do not inherit the merge file), 'w' (word-split the merge file) and '-'
|
||||
* (do not transfer the merge file) are accepted and consumed only on merge/
|
||||
* dir-merge rules (rejected on every other rule, matching rsync); their
|
||||
* semantics are not implemented and they are otherwise ignored.
|
||||
* A trailing '/' makes a pattern match directories only. A leading '/' anchors
|
||||
* the pattern to its owner directory.
|
||||
*/
|
||||
|
||||
+65
-1
@@ -40,11 +40,17 @@ static void test_filter_list_rejects_xattr_modifier() {
|
||||
}
|
||||
|
||||
static void test_filter_list_rejects_unsupported_modifiers() {
|
||||
/* The merge-file modifiers e/n/w/- are invalid on every non-merge rule; a
|
||||
token made up solely of modifier characters is a modifier run, so it must
|
||||
be rejected rather than folded into the pattern. */
|
||||
static const char* const rules[] = {
|
||||
"-e foo", /* e: merge-only in rsync */
|
||||
"-n foo", /* n: merge-only in rsync */
|
||||
"-w foo", /* w: merge-only in rsync */
|
||||
"merge,n /tmp/x", ".e /tmp/x", "dir-merge,e .rules", "exclude,w foo",
|
||||
"-new", /* pure modifier letters (n/e/w) */
|
||||
"-press", /* pure modifier letters (p/r/e/s) */
|
||||
"exclude,w foo", "exclude,e foo", "exclude,n foo",
|
||||
"hide,w foo", "protect,n foo", "risk,e foo",
|
||||
};
|
||||
for (size_t i = 0; i < sizeof(rules) / sizeof(rules[0]); i++) {
|
||||
FilterRuleList* list = filter_rule_list_create();
|
||||
@@ -57,6 +63,63 @@ static void test_filter_list_rejects_unsupported_modifiers() {
|
||||
}
|
||||
}
|
||||
|
||||
/* rsync accepts the merge-file modifiers e/n/w/- on merge and dir-merge rules.
|
||||
* They must be consumed so they never leak into the merge filename. */
|
||||
static void test_filter_list_accepts_merge_modifiers() {
|
||||
char tmpl[] = "/tmp/fastsync_filter_mmod_XXXXXX";
|
||||
EXPECT_TRUE(mkdtemp(tmpl) != NULL);
|
||||
char path[512];
|
||||
snprintf(path, sizeof(path), "%s/rules", tmpl);
|
||||
FILE* fp = fopen(path, "w");
|
||||
EXPECT_NOT_NULL(fp);
|
||||
fputs("- *.tmp\n", fp);
|
||||
fclose(fp);
|
||||
|
||||
/* merge with e/n/w/- consumes the modifiers and reads the right file. */
|
||||
static const char* const fmts[] = {
|
||||
"merge,e %s", "merge,n %s", "merge,w %s", "merge,- %s", ".e %s", ".- %s",
|
||||
};
|
||||
for (size_t i = 0; i < sizeof(fmts) / sizeof(fmts[0]); i++) {
|
||||
FilterRuleList* list = filter_rule_list_create();
|
||||
EXPECT_NOT_NULL(list);
|
||||
char rule[600];
|
||||
char err[256] = "";
|
||||
snprintf(rule, sizeof(rule), fmts[i], path);
|
||||
bool ok = filter_rule_list_parse_append(list, rule, NULL, NULL, err, sizeof(err));
|
||||
if (!ok)
|
||||
printf(" merge rule '%s' errored: %s\n", rule, err);
|
||||
EXPECT_TRUE(ok);
|
||||
EXPECT_EQ_INT(list->count, 1);
|
||||
EXPECT_EQ_STR(list->items[0]->pattern, "*.tmp");
|
||||
filter_rule_list_free(list);
|
||||
}
|
||||
|
||||
/* dir-merge with e/n/w/- registers the basename without the modifiers. */
|
||||
static const struct {
|
||||
const char* rule;
|
||||
const char* want;
|
||||
} drules[] = {
|
||||
{"dir-merge,e .rules", ".rules"}, {"dir-merge,n .rules", ".rules"},
|
||||
{"dir-merge,w .rules", ".rules"}, {"dir-merge,- .rules", ".rules"},
|
||||
{":e .rules", ".rules"}, {":- .rules", ".rules"},
|
||||
};
|
||||
for (size_t i = 0; i < sizeof(drules) / sizeof(drules[0]); i++) {
|
||||
FilterRuleList* list = filter_rule_list_create();
|
||||
EXPECT_NOT_NULL(list);
|
||||
char err[256] = "";
|
||||
bool ok = filter_rule_list_parse_append(list, drules[i].rule, NULL, NULL, err, sizeof(err));
|
||||
if (!ok)
|
||||
printf(" dir-merge rule '%s' errored: %s\n", drules[i].rule, err);
|
||||
EXPECT_TRUE(ok);
|
||||
EXPECT_EQ_INT(list->dir_merge_count, 1);
|
||||
EXPECT_EQ_STR(list->dir_merge_names[0], drules[i].want);
|
||||
filter_rule_list_free(list);
|
||||
}
|
||||
|
||||
unlink(path);
|
||||
rmdir(tmpl);
|
||||
}
|
||||
|
||||
static void test_filter_list_accepts_supported_rules_and_modifiers() {
|
||||
static const char* const rules[] = {
|
||||
"- *.tmp", "+ /a.txt", "-s foo", "-r foo", "-p foo",
|
||||
@@ -223,6 +286,7 @@ static void test_filter_rules_apply_supported_modifiers() {
|
||||
void test_filter() {
|
||||
test_filter_list_rejects_xattr_modifier();
|
||||
test_filter_list_rejects_unsupported_modifiers();
|
||||
test_filter_list_accepts_merge_modifiers();
|
||||
test_filter_list_accepts_supported_rules_and_modifiers();
|
||||
test_filter_list_merge_file_still_supported();
|
||||
test_filter_rule_parse_rejects_unsupported_and_keeps_supported();
|
||||
|
||||
Reference in New Issue
Block a user