commit 95448b627ab3c0c205b0677309e4487484658a12 from: Omar Polo date: Thu Sep 29 09:44:24 2022 UTC template: fix possible use-after-free and drop the `render' sugar if some of the tp_* function fails, the inner code could free the template too (that's what galileo does), so `tp->tp_ret' would access free'd memory! Instead, add a local variable `tp_ret' to keep track of the return code. While here, drop the `render' shortand that adds the argument call, the template_reset function and adapt the regress. commit - ee34715e9bf37e3f13a9d07b2ec1d817386c41fb commit + 95448b627ab3c0c205b0677309e4487484658a12 blob - 9f7c33896bdb131bad0479e007b5b5ab8fcb46e1 blob + d38f3dcbcaf3d10e3d4da1ad55b2a53a5d46a785 --- fcgi.c +++ fcgi.c @@ -802,10 +802,8 @@ clt_tp_puts(struct template *tp, const char *str) { struct client *clt = tp->tp_arg; - if (clt_puts(clt, str) == -1) { - tp->tp_ret = -1; + if (clt_puts(clt, str) == -1) return (-1); - } return (0); } @@ -815,10 +813,8 @@ clt_tp_putc(struct template *tp, int c) { struct client *clt = tp->tp_arg; - if (clt_putc(clt, c) == -1) { - tp->tp_ret = -1; + if (clt_putc(clt, c) == -1) return (-1); - } return (0); } blob - 552e6b568fd81e12154d647cee396d3190cd4ed4 blob + f3e96dbd4607647561536b0725e2f933bc49400b --- template/parse.y +++ template/parse.y @@ -136,9 +136,9 @@ verbatims : /* empty */ raw : STRING { dbg(); - printf("if (tp->tp_puts(tp, "); + printf("if ((tp_ret = tp->tp_puts(tp, "); printq($1); - printf(") == -1) goto err;\n"); + printf(")) == -1) goto err;\n"); free($1); } @@ -146,12 +146,12 @@ raw : STRING { block : define body end { printf("err:\n"); - puts("return tp->tp_ret;"); + puts("return tp_ret;"); puts("}"); in_define = 0; } | define body finally end { - puts("return tp->tp_ret;"); + puts("return tp_ret;"); puts("}"); in_define = 0; } @@ -162,6 +162,7 @@ define : '{' DEFINE string '}' { dbg(); printf("int\n%s\n{\n", $3); + puts("int tp_ret = 0;"); free($3); } @@ -175,11 +176,7 @@ body : /* empty */ special : '{' RENDER string '}' { dbg(); - if (strrchr($3, ')') != NULL) - printf("if (%s == -1) goto err;\n", $3); - else - printf("if (%s != NULL && %s(tp) == -1) " - "goto err;\n", $3, $3); + printf("if ((tp_ret = %s) == -1) goto err;\n", $3); free($3); } | printf @@ -187,20 +184,23 @@ special : '{' RENDER string '}' { | loop | '{' string '|' UNSAFE '}' { dbg(); - printf("if (tp->tp_puts(tp, %s) == -1) goto err;\n", + printf("if ((tp_ret = tp->tp_puts(tp, %s)) == -1)\n", $2); + puts("goto err;"); free($2); } | '{' string '|' URLESCAPE '}' { dbg(); - printf("if (tp_urlescape(tp, %s) == -1) goto err;\n", + printf("if ((tp_ret = tp_urlescape(tp, %s)) == -1)\n", $2); + puts("goto err;"); free($2); } | '{' string '}' { dbg(); - printf("if (tp->tp_escape(tp, %s) == -1) goto err;\n", + printf("if ((tp_ret = tp->tp_escape(tp, %s)) == -1)\n", $2); + puts("goto err;"); free($2); } ; @@ -210,7 +210,8 @@ printf : '{' PRINTF { printf("if (asprintf(&tp->tp_tmp, "); } printfargs '}' { printf(") == -1)\n goto err;\n"); - puts("if (tp->tp_escape(tp, tp->tp_tmp) == -1)"); + puts("if ((tp_ret = tp->tp_escape(tp, tp->tp_tmp)) " + "== -1)"); puts("goto err;"); puts("free(tp->tp_tmp);"); puts("tp->tp_tmp = NULL;"); blob - 72fc2491d73a975d4a090dbe73551f9a3bd990ee blob + f7aef5e2c673f80c7bfad865930f815b8c58859b --- template/regress/03-block.tmpl +++ template/regress/03-block.tmpl @@ -9,7 +9,6 @@

- {{ render tp->tp_body }} {{ finally }} blob - 87bad8ee4581a664cf2097201d2d371369bcf9b1 blob + 4b378ef895124815cb89173a7604d0c8630d3bed --- template/regress/04-flow.tmpl +++ template/regress/04-flow.tmpl @@ -22,7 +22,6 @@ {{ else }}

"{{ title }}" doesn't have a '*' in it

{{ end }} - {{ render tp->tp_body }} {{ finally }} blob - b2dcc4306bba2066a2b6162240a848836820a343 blob + 50bf6c0ec4dfcaec1fd63cba01c46d176957b614 --- template/regress/06-escape.tmpl +++ template/regress/06-escape.tmpl @@ -8,7 +8,6 @@

- {{ render tp->tp_body }} {{ end }} blob - 079c1cdcd7646cc4367a33ddbc44e91dbe465c30 blob + 587f2ed68d2caf23a4828887385dae846d090cfe --- template/regress/runbase.c +++ template/regress/runbase.c @@ -27,10 +27,8 @@ my_putc(struct template *tp, int c) { FILE *fp = tp->tp_arg; - if (putc(c, fp) < 0) { - tp->tp_ret = -1; + if (putc(c, fp) < 0) return (-1); - } return (0); } @@ -40,10 +38,8 @@ my_puts(struct template *tp, const char *s) { FILE *fp = tp->tp_arg; - if (fputs(s, fp) < 0) { - tp->tp_ret = -1; + if (fputs(s, fp) < 0) return (-1); - } return (0); } @@ -56,12 +52,12 @@ main(int argc, char **argv) if ((tp = template(stdout, my_puts, my_putc)) == NULL) err(1, "template"); - base(tp, " *hello* "); + if (base(tp, " *hello* ") == -1) + return (1); puts(""); - template_reset(tp); - - base(tp, ""); + if (base(tp, "") == -1) + return (1); puts(""); free(tp); blob - 73afb09894b94e6728dbf1905ec8dae81608924f blob + e5a35a7dfefa2010c52a71d596f2aa798602ab39 --- template/regress/runlist.c +++ template/regress/runlist.c @@ -28,10 +28,8 @@ my_putc(struct template *tp, int c) { FILE *fp = tp->tp_arg; - if (putc(c, fp) < 0) { - tp->tp_ret = -1; + if (putc(c, fp) < 0) return (-1); - } return (0); } @@ -41,10 +39,8 @@ my_puts(struct template *tp, const char *s) { FILE *fp = tp->tp_arg; - if (fputs(s, fp) < 0) { - tp->tp_ret = -1; + if (fputs(s, fp) < 0) return (-1); - } return (0); } @@ -69,7 +65,8 @@ main(int argc, char **argv) TAILQ_INSERT_TAIL(&head, np, entries); } - base(tp, &head); + if (base(tp, &head) == -1) + return (1); puts(""); while ((np = TAILQ_FIRST(&head))) { @@ -78,9 +75,8 @@ main(int argc, char **argv) free(np); } - template_reset(tp); - - base(tp, &head); + if (base(tp, &head) == -1) + return (1); puts(""); free(tp); blob - c09d3b9a06921d1759263225b112bbeb5741f4b2 blob + 618f25c398ce5cfd534311076b139b2128826cf5 --- template/tmpl.c +++ template/tmpl.c @@ -31,10 +31,8 @@ tp_urlescape(struct template *tp, const char *str) isspace((unsigned char)*str) || *str == '\'' || *str == '"' || *str == '\\') { r = snprintf(tmp, sizeof(tmp), "%%%2X", *str); - if (r < 0 || (size_t)r >= sizeof(tmp)) { - tp->tp_ret = -1; - return (-1); - } + if (r < 0 || (size_t)r >= sizeof(tmp)) + return (0); if (tp->tp_puts(tp, tmp) == -1) return (-1); } else { @@ -73,10 +71,8 @@ tp_htmlescape(struct template *tp, const char *str) break; } - if (r == -1) { - tp->tp_ret = -1; + if (r == -1) return (-1); - } } return (0); @@ -99,12 +95,6 @@ template(void *arg, tmpl_puts putsfn, tmpl_putc putcfn } void -template_reset(struct template *tp) -{ - tp->tp_ret = 0; -} - -void template_free(struct template *tp) { free(tp->tp_tmp); blob - 0de4f24796d823d76a6882bca883da23eb005c74 blob + a1ac0a10b029ac7e14728d1a670c61bb9cd195c1 --- template/tmpl.h +++ template/tmpl.h @@ -29,8 +29,6 @@ struct template { tmpl_puts tp_puts; tmpl_putc tp_putc; - int tp_ret; - int (*tp_body)(struct template *); }; @@ -38,7 +36,6 @@ int tp_urlescape(struct template *, const char *); int tp_htmlescape(struct template *, const char *); struct template *template(void *, tmpl_puts, tmpl_putc); -void template_reset(struct template *); void template_free(struct template *); #endif

{{ title }}

{{ title | unsafe }}