From a7dcb7901c8b4bedf42560557fe7e583298f081b Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Wed, 2 Sep 2026 14:40:46 -0500 Subject: [PATCH 1/3] Added a predicate for the secret variable tag Variable expansion has to decide what to do about a secret-tagged variable before the plaintext exists in a buffer, which EvalContextVariableGet() with get_secret=false cannot do: it produces a value either way. This asks the question on its own, straight through VariableResolve(). Ticket: CFE-3298 Changelog: None --- libpromises/eval_context.c | 12 ++++++++++++ libpromises/eval_context.h | 1 + tests/unit/eval_context_test.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index feb5766b012..18aabc9eb51 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -2757,6 +2757,18 @@ StringSet *EvalContextVariableTags(const EvalContext *ctx, const VarRef *ref) return var_tags; } +/** + * Whether the variable #ref resolves to is tagged secret. + * + * Unlike EvalContextVariableGet() with get_secret=false, this answers without + * producing a value, so a caller can decide before the plaintext exists. + */ +bool EvalContextVariableIsTaggedSecret(const EvalContext *ctx, const VarRef *ref) +{ + Variable *var = VariableResolve(ctx, ref); + return (var != NULL) && VariableIsSecret(var); +} + bool EvalContextVariableClearMatch(EvalContext *ctx) { return VariableTableClear(ctx->match_variables, NULL, NULL, NULL); diff --git a/libpromises/eval_context.h b/libpromises/eval_context.h index 2dd0f1df05d..5754c454277 100644 --- a/libpromises/eval_context.h +++ b/libpromises/eval_context.h @@ -232,6 +232,7 @@ const Promise *EvalContextVariablePromiseGet(const EvalContext *ctx, const VarRe bool EvalContextVariableRemoveSpecial(const EvalContext *ctx, SpecialScope scope, const char *lval); bool EvalContextVariableRemove(const EvalContext *ctx, const VarRef *ref); StringSet *EvalContextVariableTags(const EvalContext *ctx, const VarRef *ref); +bool EvalContextVariableIsTaggedSecret(const EvalContext *ctx, const VarRef *ref); bool EvalContextVariableClearMatch(EvalContext *ctx); VariableTableIterator *EvalContextVariableTableIteratorNew(const EvalContext *ctx, const char *ns, const char *scope, const char *lval); VariableTableIterator *EvalContextVariableTableFromRefIteratorNew(const EvalContext *ctx, const VarRef *ref); diff --git a/tests/unit/eval_context_test.c b/tests/unit/eval_context_test.c index 88a192bbaf9..c425d76414b 100644 --- a/tests/unit/eval_context_test.c +++ b/tests/unit/eval_context_test.c @@ -197,6 +197,33 @@ static void test_persistent_class_timer_policy(void) EvalContextDestroy(ctx); } +/* The predicate has to answer for a variable that exists, one that is not + * tagged, and one that does not exist at all -- code that expands variables + * asks about every reference it meets, resolvable or not. */ +static void test_variable_is_tagged_secret(void) +{ + EvalContext *ctx = EvalContextNew(); + + VarRef *secret = VarRefParse("default:bundle.password"); + assert_true(EvalContextVariablePut(ctx, secret, "hunter2", + CF_DATA_TYPE_STRING, "secret")); + + VarRef *plain = VarRefParse("default:bundle.user"); + assert_true(EvalContextVariablePut(ctx, plain, "alice", + CF_DATA_TYPE_STRING, "source=promise")); + + VarRef *missing = VarRefParse("default:bundle.nosuchvariable"); + + assert_true(EvalContextVariableIsTaggedSecret(ctx, secret)); + assert_false(EvalContextVariableIsTaggedSecret(ctx, plain)); + assert_false(EvalContextVariableIsTaggedSecret(ctx, missing)); + + VarRefDestroy(secret); + VarRefDestroy(plain); + VarRefDestroy(missing); + EvalContextDestroy(ctx); +} + /* A secret-tagged container must redact for an INDEXED read too, not just for * the whole variable. Before the type was derived from the redacted value, this * took the container branch of EvalContextVariableGet() and handed the scalar @@ -255,6 +282,7 @@ int main() unit_test(test_persistent_class_timer_policy), unit_test(test_changes_chroot), unit_test(test_eval_with_token_from_list), + unit_test(test_variable_is_tagged_secret), unit_test(test_secret_container_redacts_indexed_read), }; From 92fce29adeedf7568e35477cc0391033d8a4c4fc Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Wed, 2 Sep 2026 14:40:58 -0500 Subject: [PATCH 2/3] Added scalar expansion modes that keep and resolve secret references ExpandScalar() substitutes the value of every variable it meets, so by the time a promise is evaluated the plaintext of a secret is already in the promiser and in the constraints, and every log line, lock name and report downstream can leak it. ExpandScalarKeepSecrets() leaves "$(name)" in place instead, taking the same path an unresolvable variable takes, and ExpandScalarSecretsOnly() substitutes those references and nothing else. Between the two, the value only ever exists in the string the point of use builds for itself. ExpandScalar() is unchanged, so nothing defers until a caller asks for it. Nested references are left alone when resolving, because the outer name is not known until the inner one is substituted; a secret used as part of a variable name therefore stays unresolved rather than being expanded early. Ticket: CFE-3298 Changelog: None --- libpromises/expand.c | 81 ++++++++++++++++++++++++-- libpromises/expand.h | 4 ++ tests/unit/expand_test.c | 121 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+), 4 deletions(-) diff --git a/libpromises/expand.c b/libpromises/expand.c index 27e2ca36701..e7a5ae8709b 100644 --- a/libpromises/expand.c +++ b/libpromises/expand.c @@ -514,6 +514,26 @@ Rval ExpandBundleReference(EvalContext *ctx, return RvalNew(NULL, RVAL_TYPE_NOPROMISEE); } +/** + * What to do with a reference to a secret-tagged variable while expanding. + * + * DEFER keeps "$(password)" in the string, so nothing between expansion and the + * use of the value can leak it; the point of use calls ONLY to substitute it. + */ +typedef enum +{ + /** Substitute the value, like for any other variable. */ + EXPAND_SECRETS_INLINE, + /** Leave "$(name)" in place, for the point of use to resolve. */ + EXPAND_SECRETS_DEFER, + /** Substitute secrets and only secrets; leave every other reference. */ + EXPAND_SECRETS_ONLY, +} ExpandSecretsMode; + +static char *ExpandScalarInternal(const EvalContext *ctx, const char *ns, + const char *scope, const char *string, + Buffer *out, ExpandSecretsMode secrets); + /** * Expand a #string into Buffer #out, returning the pointer to the string * itself, inside the Buffer #out. If #out is NULL then the buffer will be @@ -523,6 +543,43 @@ Rval ExpandBundleReference(EvalContext *ctx, */ char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope, const char *string, Buffer *out) +{ + return ExpandScalarInternal(ctx, ns, scope, string, out, + EXPAND_SECRETS_INLINE); +} + +/** + * As ExpandScalar(), but leaves a secret-tagged variable's reference verbatim. + * + * The result is not final: the consumer has to call ExpandScalarSecretsOnly() + * to get the value in. Anything that only logs the string uses it as it is. + */ +char *ExpandScalarKeepSecrets(const EvalContext *ctx, const char *ns, + const char *scope, const char *string, + Buffer *out) +{ + return ExpandScalarInternal(ctx, ns, scope, string, out, + EXPAND_SECRETS_DEFER); +} + +/** + * Substitute the references ExpandScalarKeepSecrets() left behind, and nothing + * else -- a non-secret reference is copied through whether or not it resolves. + * + * Call it as late as possible, keep the result out of any log, and free it as + * soon as the value has been handed over. + */ +char *ExpandScalarSecretsOnly(const EvalContext *ctx, const char *ns, + const char *scope, const char *string, + Buffer *out) +{ + return ExpandScalarInternal(ctx, ns, scope, string, out, + EXPAND_SECRETS_ONLY); +} + +static char *ExpandScalarInternal(const EvalContext *ctx, const char *ns, + const char *scope, const char *string, + Buffer *out, ExpandSecretsMode secrets) { bool out_belongs_to_us = false; @@ -553,11 +610,16 @@ char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope, ExtractScalarReference(current_item, sp, strlen(sp), true); sp += BufferSize(current_item) + 2; - if (IsCf3VarString(BufferData(current_item))) + /* ONLY mode re-emits the reference unless it names a secret, and + * expanding an inner one first would re-emit a rewritten name. So a + * secret used as part of a variable *name* stays unresolved. */ + if (IsCf3VarString(BufferData(current_item)) && + secrets != EXPAND_SECRETS_ONLY) { Buffer *temp = BufferCopy(current_item); BufferClear(current_item); - ExpandScalar(ctx, ns, scope, BufferData(temp), current_item); + ExpandScalarInternal(ctx, ns, scope, BufferData(temp), current_item, + secrets); BufferDestroy(temp); } @@ -566,8 +628,19 @@ char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope, VarRef *ref = VarRefParseFromNamespaceAndScope( BufferData(current_item), ns, scope, CF_NS, '.'); - DataType value_type; - const void *value = EvalContextVariableGetPlaintext(ctx, ref, &value_type); + const bool is_secret = (secrets != EXPAND_SECRETS_INLINE) && + EvalContextVariableIsTaggedSecret(ctx, ref); + const bool substitute = (secrets == EXPAND_SECRETS_ONLY) + ? is_secret + : !is_secret; + + /* Not substituting leaves the type NONE, which falls through to + * the re-emit below -- the path an unresolvable variable takes. The + * value is never fetched, not even to be discarded. */ + DataType value_type = CF_DATA_TYPE_NONE; + const void *value = substitute + ? EvalContextVariableGetPlaintext(ctx, ref, &value_type) + : NULL; VarRefDestroy(ref); switch (DataTypeToRvalType(value_type)) diff --git a/libpromises/expand.h b/libpromises/expand.h index 6a35c26edeb..906b998358b 100644 --- a/libpromises/expand.h +++ b/libpromises/expand.h @@ -41,6 +41,10 @@ bool IsExpandable(const char *str); char *ExpandScalar(const EvalContext *ctx, const char *ns, const char *scope, const char *string, Buffer *out); +char *ExpandScalarKeepSecrets(const EvalContext *ctx, const char *ns, const char *scope, + const char *string, Buffer *out); +char *ExpandScalarSecretsOnly(const EvalContext *ctx, const char *ns, const char *scope, + const char *string, Buffer *out); Rval ExpandBundleReference(EvalContext *ctx, const char *ns, const char *scope, Rval rval); Rval ExpandPrivateRval(const EvalContext *ctx, const char *ns, const char *scope, const void *rval_item, RvalType rval_type); Rlist *ExpandList(const EvalContext *ctx, const char *ns, const char *scope, const Rlist *list, int expandnaked); diff --git a/tests/unit/expand_test.c b/tests/unit/expand_test.c index 219f07571d0..5b2870b644c 100644 --- a/tests/unit/expand_test.c +++ b/tests/unit/expand_test.c @@ -559,6 +559,122 @@ static void test_expand_promise_array_with_slist_arg(void **state) PolicyDestroy(policy); } +/* Puts one secret-tagged and one ordinary variable in the same bundle, and + * asserts the tag really landed -- without that control every expectation + * below would also hold for a build where nothing is ever tagged secret. */ +static void PutSecretAndPlain(EvalContext *ctx) +{ + VarRef *secret = VarRefParse("default:bundle.password"); + assert_true(EvalContextVariablePut(ctx, secret, "hunter2", + CF_DATA_TYPE_STRING, "secret")); + assert_true(EvalContextVariableIsTaggedSecret(ctx, secret)); + VarRefDestroy(secret); + + VarRef *plain = VarRefParse("default:bundle.user"); + assert_true(EvalContextVariablePut(ctx, plain, "alice", + CF_DATA_TYPE_STRING, NULL)); + assert_false(EvalContextVariableIsTaggedSecret(ctx, plain)); + VarRefDestroy(plain); +} + +/* ExpandScalar() itself is unchanged: a secret expands like anything else. */ +static void test_expand_scalar_secret_inlined_by_default(void **state) +{ + EvalContext *ctx = *state; + PutSecretAndPlain(ctx); + + Buffer *res = BufferNew(); + ExpandScalar(ctx, "default", "bundle", "$(user):$(password)", res); + + assert_string_equal("alice:hunter2", BufferData(res)); + BufferDestroy(res); +} + +static void test_expand_scalar_keep_secrets(void **state) +{ + EvalContext *ctx = *state; + PutSecretAndPlain(ctx); + + Buffer *res = BufferNew(); + ExpandScalarKeepSecrets(ctx, "default", "bundle", "$(user):$(password)", res); + + /* Only the secret is held back; the ordinary variable still expands. */ + assert_string_equal("alice:$(password)", BufferData(res)); + BufferDestroy(res); +} + +static void test_expand_scalar_secrets_only(void **state) +{ + EvalContext *ctx = *state; + PutSecretAndPlain(ctx); + + Buffer *res = BufferNew(); + ExpandScalarSecretsOnly(ctx, "default", "bundle", "$(user):$(password)", res); + + /* The mirror image: a resolvable non-secret reference is left alone. */ + assert_string_equal("$(user):hunter2", BufferData(res)); + BufferDestroy(res); +} + +/* Deferring and then resolving has to end up where expanding in one go would, + * or a commands promise would run something other than what it promised. */ +static void test_expand_scalar_secret_round_trip(void **state) +{ + EvalContext *ctx = *state; + PutSecretAndPlain(ctx); + + const char *const cases[] = { + "$(user):$(password)", + "${user}:${password}", /* the brace form is preserved too */ + "$(password)$(password)", + "no references at all", + "a$(undefined)b", /* unresolvable, untouched by both */ + }; + + for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); i++) + { + char *const direct = ExpandScalar(ctx, "default", "bundle", cases[i], NULL); + char *const deferred = ExpandScalarKeepSecrets(ctx, "default", "bundle", + cases[i], NULL); + char *const resolved = ExpandScalarSecretsOnly(ctx, "default", "bundle", + deferred, NULL); + assert_string_equal(direct, resolved); + free(direct); + free(deferred); + free(resolved); + } +} + +/* A secret used as part of a variable *name* cannot be resolved late: the + * outer name is not known until the inner one is substituted. Both modes leave + * the whole reference alone rather than leaking the inner value. */ +static void test_expand_scalar_secret_nested_reference(void **state) +{ + EvalContext *ctx = *state; + PutSecretAndPlain(ctx); + { + VarRef *lval = VarRefParse("default:bundle.foo[hunter2]"); + EvalContextVariablePut(ctx, lval, "bar", CF_DATA_TYPE_STRING, NULL); + VarRefDestroy(lval); + } + + /* Control: expanded in one go, this resolves all the way through. */ + char *const direct = ExpandScalar(ctx, "default", "bundle", + "a$(foo[$(password)])b", NULL); + assert_string_equal("abarb", direct); + free(direct); + + char *const deferred = ExpandScalarKeepSecrets(ctx, "default", "bundle", + "a$(foo[$(password)])b", NULL); + assert_string_equal("a$(foo[$(password)])b", deferred); + + char *const resolved = ExpandScalarSecretsOnly(ctx, "default", "bundle", + deferred, NULL); + assert_string_equal("a$(foo[$(password)])b", resolved); + free(deferred); + free(resolved); +} + static void test_setup(void **state) { *state = EvalContextNew(); @@ -589,6 +705,11 @@ int main() unit_test_setup_teardown(test_expand_scalar_array_concat, test_setup, test_teardown), unit_test_setup_teardown(test_expand_scalar_array_with_scalar_arg, test_setup, test_teardown), unit_test_setup_teardown(test_expand_scalar_undefined, test_setup, test_teardown), + unit_test_setup_teardown(test_expand_scalar_secret_inlined_by_default, test_setup, test_teardown), + unit_test_setup_teardown(test_expand_scalar_keep_secrets, test_setup, test_teardown), + unit_test_setup_teardown(test_expand_scalar_secrets_only, test_setup, test_teardown), + unit_test_setup_teardown(test_expand_scalar_secret_round_trip, test_setup, test_teardown), + unit_test_setup_teardown(test_expand_scalar_secret_nested_reference, test_setup, test_teardown), unit_test_setup_teardown(test_expand_scalar_nested_inner_undefined, test_setup, test_teardown), unit_test_setup_teardown(test_expand_list_nested, test_setup, test_teardown), unit_test_setup_teardown(test_expand_promise_array_with_scalar_arg, test_setup, test_teardown), From ed8574457f0469f5eca92de41ef41c8419f2c416 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Wed, 2 Sep 2026 14:41:12 -0500 Subject: [PATCH 3/3] Made commands promises keep a secret's reference until they run the command The promiser of a commands promise is now expanded with the references to secret-tagged variables left in place, and RepairExec() resolves them only in the string it hands to cf_popen(). The lock name, $(this.promiser), the promise banner, "Executing ...", "Would execute script ..." and the module output all keep the reference, so a secret used in a command no longer reaches the log at inform level, nor the lock database, nor a --dry-run report. The executable check needs the value, since "$(x)" is never a path, so it resolves the promiser too and frees it immediately; its own diagnostics still print the reference. Ordinary variables are unaffected, and a commands promise with no references left in its promiser skips the extra pass entirely. 'args' and 'arglist' are ordinary constraints and still expand eagerly. So does the vars promise's own debug logging, which prints the value at -d; that is a separate call site, covered by CFE-3294. Ticket: CFE-3298 Changelog: Title --- cf-agent/verify_exec.c | 72 ++++++++-- libpromises/promises.c | 27 +++- tests/acceptance/31_tickets/CFE-3298/test.cf | 126 ++++++++++++++++++ .../31_tickets/CFE-3298/test.cf.sub | 80 +++++++++++ 4 files changed, 295 insertions(+), 10 deletions(-) create mode 100644 tests/acceptance/31_tickets/CFE-3298/test.cf create mode 100644 tests/acceptance/31_tickets/CFE-3298/test.cf.sub diff --git a/cf-agent/verify_exec.c b/cf-agent/verify_exec.c index 15c74d5fee9..6d163ed9749 100644 --- a/cf-agent/verify_exec.c +++ b/cf-agent/verify_exec.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include @@ -202,6 +203,33 @@ static char *GetLockNameExec(const Attributes *a, const Promise *pp) /*****************************************************************************/ +/** + * Resolve the secret references the promiser of a commands promise carries + * unexpanded (PromiseTypeResolvesSecretsAtUse() in promises.c). + * + * Every result goes straight to the process being started; it must not reach a + * log message, a lock name or a report. + * + * @return A new string, always; the caller owns it. + */ +static char *ResolveSecrets(const EvalContext *ctx, const char *ns, + const char *string) +{ + assert(string != NULL); + if (string == NULL) + { + return NULL; + } + + /* Nothing was deferred if no reference is left, the usual case. */ + if (!IsCf3VarString(string)) + { + return xstrdup(string); + } + + return ExpandScalarSecretsOnly(ctx, ns, "this", string, NULL); +} + static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, const Promise *pp, PromiseResult *result) { @@ -221,9 +249,22 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, module_context[0] = '\0'; - if (IsAbsoluteFileName(CommandArg0(pp->promiser)) || a->contain.shelltype == SHELL_TYPE_NONE) + const char *const ns = PromiseGetNamespace(pp); + + /* "$(x)" never names an executable, so this check needs the values; the + * diagnostics below keep pp->promiser, which does not have them. Freed at + * once, and fetched again as late as possible at cf_popen(). CommandArg0() + * returns a static buffer, hence the copy. */ + char *real_arg0; + { + char *const real_promiser = ResolveSecrets(ctx, ns, pp->promiser); + real_arg0 = xstrdup(CommandArg0(real_promiser)); + free(real_promiser); + } + + if (IsAbsoluteFileName(real_arg0) || a->contain.shelltype == SHELL_TYPE_NONE) { - if (!IsExecutable(CommandArg0(pp->promiser))) + if (!IsExecutable(real_arg0)) { cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, a, "'%s' promises to be executable but isn't", pp->promiser); *result = PromiseResultUpdate(*result, PROMISE_RESULT_FAIL); @@ -233,6 +274,7 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, Log(LOG_LEVEL_VERBOSE, "Paths with spaces must be inside escaped quoutes (e.g. \\\"%s\\\")", pp->promiser); } + free(real_arg0); return ACTION_RESULT_FAILED; } else @@ -240,6 +282,7 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, Log(LOG_LEVEL_VERBOSE, "Promiser string contains a valid executable '%s' - ok", CommandArg0(pp->promiser)); } } + free(real_arg0); char timeout_str[CF_BUFSIZE]; if (a->contain.timeout == CF_NOINT) @@ -315,13 +358,18 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, } #endif /* !__MINGW32__ */ + /* Point of use: nothing below cf_popen*() needs the values, so the + * strings holding them are built and freed here. 'cmdline' keeps the + * references and is what every log message in this function prints. */ const char *open_mode = a->module ? "rt" : "r"; if (a->contain.shelltype == SHELL_TYPE_POWERSHELL) { #ifdef __MINGW32__ + char *const real_cmdline = ResolveSecrets(ctx, ns, cmdline); pfp = - cf_popen_powershell_setuid(cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, + cf_popen_powershell_setuid(real_cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, a->transaction.background); + free(real_cmdline); #else // !__MINGW32__ Log(LOG_LEVEL_ERR, "Powershell is only supported on Windows"); return ACTION_RESULT_FAILED; @@ -329,9 +377,11 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, } else if (a->contain.shelltype == SHELL_TYPE_USE) { + char *const real_cmdline = ResolveSecrets(ctx, ns, cmdline); pfp = - cf_popen_shsetuid(cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, + cf_popen_shsetuid(real_cmdline, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, a->transaction.background); + free(real_cmdline); } else { @@ -339,15 +389,21 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, ? xstrdup(pp->promiser) : StringFormat("%s %s", pp->promiser, a->args); + char *const real_command = ResolveSecrets(ctx, ns, command); + free(command); + + /* arglist entries are ordinary constraints, already expanded, but + * one can hold $(this.promiser), which here is unexpanded. */ Seq *arglist = NULL; if (a->arglist != NULL) { - arglist = SeqNew(8, NULL); + arglist = SeqNew(RlistLen(a->arglist), free); for (const Rlist *rp = a->arglist; rp != NULL; rp = rp->next) { if (rp->val.type == RVAL_TYPE_SCALAR) { - SeqAppend(arglist, RlistScalarValue(rp)); + SeqAppend(arglist, + ResolveSecrets(ctx, ns, RlistScalarValue(rp))); } else { @@ -357,11 +413,11 @@ static ActionResult RepairExec(EvalContext *ctx, const Attributes *a, } } } - pfp = cf_popensetuid(command, arglist, open_mode, + pfp = cf_popensetuid(real_command, arglist, open_mode, a->contain.owner, a->contain.group, a->contain.chdir, a->contain.chroot, a->transaction.background); - free(command); + free(real_command); SeqDestroy(arglist); } diff --git a/libpromises/promises.c b/libpromises/promises.c index ec8b3d903c8..9e1770cb8b3 100644 --- a/libpromises/promises.c +++ b/libpromises/promises.c @@ -658,6 +658,19 @@ static void DereferenceAndPutComment(Promise* pp, const char *comment) } } +/** + * Whether this promise type fetches secret values itself, at the point of use. + * + * Its promiser keeps the references, so the lock name, $(this.promiser) and the + * actuator's own log lines cannot leak the value. In exchange the actuator has + * to resolve them before use; see RepairExec(). + */ +static bool PromiseTypeResolvesSecretsAtUse(const Promise *pp) +{ + assert(pp != NULL); + return StringEqual(PromiseGetPromiseType(pp), "commands"); +} + Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) { assert(pp != NULL); @@ -668,8 +681,18 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) *excluded = false; - Rval returnval = ExpandPrivateRval(ctx, PromiseGetNamespace(pp), - "this", pp->promiser, RVAL_TYPE_SCALAR); + Rval returnval; + if (PromiseTypeResolvesSecretsAtUse(pp)) + { + returnval = (Rval) { ExpandScalarKeepSecrets(ctx, PromiseGetNamespace(pp), + "this", pp->promiser, NULL), + RVAL_TYPE_SCALAR }; + } + else + { + returnval = ExpandPrivateRval(ctx, PromiseGetNamespace(pp), + "this", pp->promiser, RVAL_TYPE_SCALAR); + } if (returnval.item == NULL) { assert(returnval.type == RVAL_TYPE_LIST || diff --git a/tests/acceptance/31_tickets/CFE-3298/test.cf b/tests/acceptance/31_tickets/CFE-3298/test.cf new file mode 100644 index 00000000000..78e6a1ff7a7 --- /dev/null +++ b/tests/acceptance/31_tickets/CFE-3298/test.cf @@ -0,0 +1,126 @@ +####################################################### +# +# A commands promise must work with a secret variable's reference, not its +# value, until it runs the command (CFE-3298). So every log line carries +# "$(value)" while the output files hold the real value. +# +# Each absent value is paired with a present control -- the reference that +# replaced it, an ordinary variable that must still expand, and every promise +# outcome -- so an agent that expanded nothing also fails. +# +# The sub-policy runs from a commands promise, not execresult(), so that it +# runs exactly once instead of on every evaluation pass. +# +####################################################### +body file control +{ + inputs => { "../../default.sub.cf" }; +} + +bundle agent __main__ +{ + methods: + "bundlesequence" usebundle => default("$(this.promise_filename)"); +} + +####################################################### +bundle agent init +{ + files: + "$(test.output)" delete => tidy; +} + +####################################################### +bundle agent test +{ + meta: + "description" + string => "A commands promise keeps the reference to a secret variable unexpanded until it runs the command", + meta => { "CFE-3298" }; + + vars: + "output" string => "$(G.testdir)$(const.dirsep)CFE-3298.out"; + + commands: + "$(sys.cf_agent) -KI -f $(this.promise_filename).sub > $(output) 2>&1" + contain => in_shell; +} + +####################################################### +bundle agent check +{ + vars: + "dir" string => "$(G.testdir)$(const.dirsep)CFE-3298"; + + # Must not have reached any log line. $(G.touch) is the resolved 'exe'. + "must_be_absent" slist => { "S3CR3T-CFE3298-VALUE", "$(G.touch)" }; + + # The controls: the references that replaced them, an ordinary variable + # that must still expand, and the promise outcomes. + "must_be_present" + slist => { + # A bare "$" would anchor the regex, hence the character class. + "[$(const.dollar)]\(value\)", + "[$(const.dollar)]\(exe\)", + "PLAIN-CFE3298-VALUE", + "CHECKPOINT_shell_repaired", + "CHECKPOINT_noshell_repaired", + "CHECKPOINT_plain_repaired", + "CHECKPOINT_DONE", + }; + + "shell_got" string => readfile("$(dir)$(const.dirsep)shell.out", 128); + "plain_got" string => readfile("$(dir)$(const.dirsep)plain.out", 128); + + classes: + "leaked" + expression => isgreaterthan( + countlinesmatching(".*$(must_be_absent).*", "$(test.output)"), "0" + ); + + "missing" + expression => islessthan( + countlinesmatching(".*$(must_be_present).*", "$(test.output)"), "1" + ); + + # Keeping the reference must not stop the command getting the value. + "shell_delivered" + expression => strcmp("$(shell_got)", "S3CR3T-CFE3298-VALUE"); + + "plain_delivered" + expression => strcmp("$(plain_got)", "PLAIN-CFE3298-VALUE"); + + "noshell_delivered" + expression => fileexists("$(dir)$(const.dirsep)noshell.out"); + + "delivered" + and => { "shell_delivered", "plain_delivered", "noshell_delivered" }; + + "ok" and => { "!leaked", "!missing", "delivered" }; + + reports: + DEBUG:: + "a secret value matching '$(must_be_absent)' reached the agent output" + if => isgreaterthan( + countlinesmatching(".*$(must_be_absent).*", "$(test.output)"), "0" + ); + + "expected output matching '$(must_be_present)' is missing" + if => islessthan( + countlinesmatching(".*$(must_be_present).*", "$(test.output)"), "1" + ); + + "Output of $(this.promise_filename).sub is in '$(test.output)'"; + + DEBUG.!delivered:: + "a command did not receive the value (shell '$(shell_got)', plain '$(plain_got)')"; + + DEBUG.!noshell_delivered:: + "the noshell command did not run: '$(dir)$(const.dirsep)noshell.out' was not created"; + + ok:: + "$(this.promise_filename) Pass"; + + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/31_tickets/CFE-3298/test.cf.sub b/tests/acceptance/31_tickets/CFE-3298/test.cf.sub new file mode 100644 index 00000000000..f1d66ec6048 --- /dev/null +++ b/tests/acceptance/31_tickets/CFE-3298/test.cf.sub @@ -0,0 +1,80 @@ +####################################################### +# +# Sub-policy for CFE-3298 test.cf. +# +# Everything this prints is what the parent inspects, so what matters is +# whether each log line carries the reference or the value. CHECKPOINT_DONE +# separates "the value was not printed" from "the policy never got there". +# +####################################################### +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { "prep", "run", "done" }; +} + +####################################################### +body contain no_shell +# @brief Take the argv path through cf_popensetuid() rather than a shell. +{ + useshell => "noshell"; +} + +####################################################### +bundle agent prep +{ + vars: + "dir" string => "$(G.testdir)$(const.dirsep)CFE-3298"; + + files: + "$(dir)$(const.dirsep)." create => "true"; + + "$(dir)$(const.dirsep)shell.out" delete => tidy; + "$(dir)$(const.dirsep)noshell.out" delete => tidy; + "$(dir)$(const.dirsep)plain.out" delete => tidy; +} + +####################################################### +bundle agent run +{ + vars: + "value" string => "S3CR3T-CFE3298-VALUE", meta => { "secret" }; + + # The command itself is secret, so the reference has to be resolved + # before the promiser can be checked for executability. + "exe" string => "$(G.touch)", meta => { "secret" }; + + # Control: an ordinary variable must still expand, or a build that + # stopped expanding promisers entirely would pass. + "plain" string => "PLAIN-CFE3298-VALUE"; + + commands: + "$(G.printf) %s $(value) > $(prep.dir)$(const.dirsep)shell.out" + contain => in_shell, + classes => results("namespace", "shell"); + + "$(exe) $(prep.dir)$(const.dirsep)noshell.out" + contain => no_shell, + classes => results("namespace", "noshell"); + + "$(G.printf) %s $(plain) > $(prep.dir)$(const.dirsep)plain.out" + contain => in_shell, + classes => results("namespace", "plain"); +} + +####################################################### +bundle agent done +{ + reports: + shell_repaired:: + "CHECKPOINT_shell_repaired"; + + noshell_repaired:: + "CHECKPOINT_noshell_repaired"; + + plain_repaired:: + "CHECKPOINT_plain_repaired"; + + any:: + "CHECKPOINT_DONE"; +}