BUG in jail(8) variable substitution, and PATCH

[Dirk Engling]

The variable substitution of FreeBSD's jail tool yields unexpected
results when a parameter has more than one variable to substitute and
one of the later variables needs substitution as well.

Consider the simple test case:

$A = "A_${B}_C_${D}";
$B = "BBBBB";
$D = "DDDDD_${E}_FFFFF";
$E = "EEEEE";

bar {
        exec.poststart = "touch /tmp/$A";
}

EXPECTED OUTCOME for running "jail -c bar" would be a file with the name
/tmp/A_BBBBB_C_DDDDD_EEEEE_FFFFF to be touched (and, of course, the jail
bar being created).

OBSERVED OUTCOME is a file with the name
/tmp/A_BBBDDDDD_EEEEE_FFFFFBB_C_ being created.

The reason is the way jail(8) resolves recursive substitutions. In
head/usr.sbin/jail/config.c:193 a varoff variable is introduced that
handles a shifting offset for multiple variable substitutions per
parameter. This varoff is updated after each substitution in line 239 to
reflect a new offset into the parameter's string. This ensures that all
other variables are substituted at [their insertion point plus varoff]
which is the accumulated length of all previously substituted variables.

Now in our example, if $A is to be expanded, first ${B} is inserted at
offset 2 and varoff becomes 10. When substituting ${D}, the recursion
check at line 216 detects that variable $D also needs expansion. It
reorders the parameter list, so that the algorithm works on variable $D
now. Then it jumps to find_vars at line 191 and properly expands
DDDDD_${E}_FFFFF to DDDDD_EEEEE_FFFFF.

When the algorithm now returns to expanding $A by entering the loop body
again, it finds a re-set varoff variable leading to (the now expanded)
variable $D being inserted at the offset 5, where the parser initially
would find it (the internal format for $A is approx: { "A__C_", {2,
"B"}, {5, "D"}}) and not at the corrected offset 10.

PROPOSED SOLUTION: Get rid of the varoff and replace line 239 with:

[struct cfvar *]vv = v;
while ((vv = STAILQ_NEXT(vv, tq)))
    v->pos += vs->len;

to make the offset permanent. Find a patch attached.

Regards,

  erdgeist
-------------- next part --------------
Index: config.c
===================================================================
--- config.c    (revision 256751)
+++ config.c    (working copy)
@@ -129,9 +129,8 @@
    struct cfjail *j, *tj, *wj;
    struct cfparam *p, *vp, *tp;
    struct cfstring *s, *vs, *ns;
-   struct cfvar *v;
+   struct cfvar *v, *vv;
    char *ep;
-   size_t varoff;
    int did_self, jseq, pgen;
 
    if (!strcmp(cfname, "-")) {
@@ -190,7 +189,6 @@
            p->gen = ++pgen;
        find_vars:
            TAILQ_FOREACH(s, &p->val, tq) {
-           varoff = 0;
            while ((v = STAILQ_FIRST(&s->vars))) {
                TAILQ_FOREACH(vp, &j->params, tq)
                    if (!strcmp(vp->name, v->name))
@@ -232,11 +230,13 @@
                    goto bad_var;
                }
                s->s = erealloc(s->s, s->len + vs->len + 1);
-               memmove(s->s + v->pos + varoff + vs->len,
-                   s->s + v->pos + varoff,
-                   s->len - (v->pos + varoff) + 1);
-               memcpy(s->s + v->pos + varoff, vs->s, vs->len);
-               varoff += vs->len;
+               memmove(s->s + v->pos + vs->len,
+                   s->s + v->pos,
+                   s->len - v->pos + 1);
+               memcpy(s->s + v->pos, vs->s, vs->len);
+               vv = v;
+               while ((vv = STAILQ_NEXT(vv, tq)))
+                   vv->pos += vs->len;
                s->len += vs->len;
                while ((vs = TAILQ_NEXT(vs, tq))) {
                    ns = emalloc(sizeof(struct cfstring));