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;
 

Reply via email to