The current code for extracting the token name from %{name} can be
simplified by computing the token name length. The existing code
copies "name}" to token[] using memcpy(), then strchr() to find the
'}' and replace it with a NUL. Using strchr() here is fragile since
token[] is not yet NUL-terminated. This is currently not a problem
since there is an earlier check for '}' in the source string but
it could be dangerous is the code changes further.
I find it much simpler to compute the token name length, verify the
length, copy the bytes and then explicitly NUL-terminate token.
This results in less code and is more easily audited.
I've also removed the duplicate check for *(pbuf+1) != '{'.
OK?
- todd
Index: usr.sbin/smtpd/mda_variables.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/mda_variables.c,v
retrieving revision 1.8
diff -u -p -U10 -u -r1.8 mda_variables.c
--- usr.sbin/smtpd/mda_variables.c 19 Mar 2023 01:43:11 -0000 1.8
+++ usr.sbin/smtpd/mda_variables.c 19 Mar 2023 13:59:01 -0000
@@ -236,21 +236,21 @@ mda_expand_token(char *dest, size_t len,
ssize_t
mda_expand_format(char *buf, size_t len, const struct deliver *dlv,
const struct userinfo *ui, const char *mda_command)
{
char tmpbuf[EXPAND_BUFFER], *ptmp, *pbuf, *ebuf;
char exptok[EXPAND_BUFFER];
ssize_t exptoklen;
char token[MAXTOKENLEN];
- size_t ret, tmpret;
+ size_t ret, tmpret, toklen;
if (len < sizeof tmpbuf) {
log_warnx("mda_expand_format: tmp buffer < rule buffer");
return -1;
}
memset(tmpbuf, 0, sizeof tmpbuf);
pbuf = buf;
ptmp = tmpbuf;
ret = tmpret = 0;
@@ -261,48 +261,46 @@ mda_expand_format(char *buf, size_t len,
if (tmpret >= sizeof tmpbuf) {
log_warnx("warn: user directory for %s too large",
ui->directory);
return 0;
}
ret += tmpret;
ptmp += tmpret;
pbuf += 2;
}
-
/* expansion loop */
for (; *pbuf && ret < sizeof tmpbuf; ret += tmpret) {
if (*pbuf == '%' && *(pbuf + 1) == '%') {
*ptmp++ = *pbuf++;
pbuf += 1;
tmpret = 1;
continue;
}
if (*pbuf != '%' || *(pbuf + 1) != '{') {
*ptmp++ = *pbuf++;
tmpret = 1;
continue;
}
/* %{...} otherwise fail */
- if (*(pbuf+1) != '{' || (ebuf = strchr(pbuf+1, '}')) == NULL)
+ if ((ebuf = strchr(pbuf+2, '}')) == NULL)
return 0;
/* extract token from %{token} */
- if ((size_t)(ebuf - pbuf) - 1 >= sizeof token)
+ toklen = ebuf - (pbuf+2);
+ if (toklen >= sizeof token)
return 0;
- memcpy(token, pbuf+2, ebuf-pbuf-1);
- if (strchr(token, '}') == NULL)
- return 0;
- *strchr(token, '}') = '\0';
+ memcpy(token, pbuf+2, toklen);
+ token[toklen] = '\0';
exptoklen = mda_expand_token(exptok, sizeof exptok, token, dlv,
ui, mda_command);
if (exptoklen == -1)
return -1;
/* writing expanded token at ptmp will overflow tmpbuf */
if (sizeof (tmpbuf) - (ptmp - tmpbuf) <= (size_t)exptoklen)
return -1;
