hi tech@, In lka.c:lka_expand(), there is a bug which causes the function to not compute correctly the remaining space in its expansion buffer. All strlcpy and strlcat truncation tests will use the bogus value making them useless. The consequence is that IF you hit that bug you will crash at RCPT time [1][2].
oga@ spotted the bug and rewrote a correct and simpler version of lka_expand() which fixes the crash and other known shortcomings. the caller now knows if an expansion has failed allowing us to reject recipient at RCPT time, rather than assuming the admin knows how to write a proper format :) please test, especially if you use rules with formats: accept [...] deliver to mda "/path/to/bin %u" [1] you are unlikely to hit the bug unless you have an insanely long format or many many many specifiers. [2] lka_expand() only processes sanitized inputs. lka_expand() rewrite by oga@, lka_queue_append() change by me Index: lka.c =================================================================== RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v retrieving revision 1.116 diff -u -p -r1.116 lka.c --- lka.c 8 Sep 2010 13:46:18 -0000 1.116 +++ lka.c 8 Sep 2010 23:51:10 -0000 @@ -62,7 +62,7 @@ void lka_rcpt_action(struct smtpd *, ch void lka_session_destroy(struct smtpd *, struct lkasession *); void lka_expansion_done(struct smtpd *, struct lkasession *); void lka_session_fail(struct smtpd *, struct lkasession *); -void lka_queue_append(struct smtpd *, struct lkasession *, int); +int lka_queue_append(struct smtpd *, struct lkasession *, int); u_int32_t lka_id; @@ -376,24 +376,24 @@ lka_expand(char *buf, size_t len, struct { char *p, *pbuf; struct rule r; - size_t ret; + size_t ret, lret = 0; struct passwd *pw; bzero(r.r_value.path, MAXPATHLEN); pbuf = r.r_value.path; ret = 0; - for (p = path->rule.r_value.path; *p != '\0'; ++p) { + for (p = path->rule.r_value.path; *p != '\0'; + ++p, len -= lret, pbuf += lret, ret += lret) { if (p == path->rule.r_value.path && *p == '~') { if (*(p + 1) == '/' || *(p + 1) == '\0') { pw = getpwnam(path->pw_name); if (pw == NULL) - continue; + return 0; - ret += strlcat(pbuf, pw->pw_dir, len); - if (ret >= len) - return ret; - pbuf += strlen(pw->pw_dir); + lret = strlcat(pbuf, pw->pw_dir, len); + if (lret >= len) + return 0; continue; } @@ -401,105 +401,81 @@ lka_expand(char *buf, size_t len, struct char username[MAXLOGNAME]; char *delim; - ret = strlcpy(username, p + 1, + lret = strlcpy(username, p + 1, sizeof(username)); - delim = strchr(username, '/'); - if (delim == NULL && ret >= sizeof(username)) { - continue; - } + if (lret >= sizeof(username)) + return 0; - if (delim != NULL) { - *delim = '\0'; - } + delim = strchr(username, '/'); + if (delim == NULL) + goto copy; + *delim = '\0'; pw = getpwnam(username); if (pw == NULL) - continue; + return 0; - ret += strlcat(pbuf, pw->pw_dir, len); - if (ret >= len) - return ret; - pbuf += strlen(pw->pw_dir); + lret = strlcat(pbuf, pw->pw_dir, len); + if (lret >= len) + return 0; p += strlen(username); continue; } } - if (strncmp(p, "%U", 2) == 0) { - ret += strlcat(pbuf, sender->user, len); - if (ret >= len) - return ret; - pbuf += strlen (sender->user); - ++p; - continue; - } - if (strncmp(p,"%D",2) == 0) { - ret += strlcat(pbuf, sender->domain, len); - if (ret >= len) - return ret; - pbuf += strlen(sender->domain); - ++p; - continue; - } - if (strncmp(p, "%a", 2) == 0) { - ret += strlcat(pbuf, path->user, len); - if (ret >= len) - return ret; - pbuf += strlen(path->user); - ++p; - continue; - } - if (strncmp(p, "%u", 2) == 0) { - ret += strlcat(pbuf, path->pw_name, len); - if (ret >= len) - return ret; - pbuf += strlen(path->pw_name); - ++p; - continue; - } - if (strncmp(p, "%d", 2) == 0) { - ret += strlcat(pbuf, path->domain, len); - if (ret >= len) - return ret; - pbuf += strlen(path->domain); - ++p; - continue; - } - if (*p == '%' && isdigit((int)*(p+1)) && *(p+2) == 'a') { - size_t idx; + if (*p == '%') { + char *string, *tmp = p + 1; + int digit = 0; + + if (isdigit((int)*tmp)) { + digit = 1; + tmp++; + } + switch (*tmp) { + case 'U': + string = sender->user; + break; + case 'D': + string = sender->domain; + break; + case 'a': + string = path->user; + break; + case 'u': + string = path->pw_name; + break; + case 'd': + string = path->domain; + break; + default: + goto copy; + } - idx = *(p+1) - '0'; - if (idx < strlen(path->user)) - *pbuf++ = path->user[idx]; - p+=2; - ++ret; - continue; - } - if (*p == '%' && isdigit((int)*(p+1)) && *(p+2) == 'u') { - size_t idx; + if (digit == 1) { + size_t idx = *(tmp - 1) - '0'; - idx = *(p+1) - '0'; - if (idx < strlen(path->pw_name)) - *pbuf++ = path->pw_name[idx]; - p+=2; - ++ret; - continue; - } - if (*p == '%' && isdigit((int)*(p+1)) && *(p+2) == 'd') { - size_t idx; + lret = 1; + if (idx < strlen(string)) + *pbuf++ = string[idx]; + else { /* fail */ + return 0; + } - idx = *(p+1) - '0'; - if (idx < strlen(path->domain)) - *pbuf++ = path->domain[idx]; - p+=2; - ++ret; + p += 2; /* loop only does ++ */ + continue; + } + lret = strlcat(pbuf, string, len); + if (lret >= len) + return 0; + p++; continue; } - - *pbuf++ = *p; - ++ret; +copy: + lret = 1; + *pbuf = *p; } - memcpy(path->rule.r_value.path, r.r_value.path, ret); + /* + 1 to include the NUL byte. */ + memcpy(path->rule.r_value.path, r.r_value.path, ret + 1); return ret; } @@ -665,17 +641,21 @@ lka_expansion_done(struct smtpd *env, st log_debug("lka_expansion_done: list empty"); else log_debug("lka_expansion_done: session error"); - status = S_MESSAGE_PERMFAILURE; - imsg_compose_event(env->sc_ievs[PROC_MFA], IMSG_LKA_RCPT, - s->message.id, 0, -1, &status, sizeof status); - lka_clear_expandtree(&s->expandtree); - lka_clear_deliverylist(&s->deliverylist); - lka_session_destroy(env, s); - } else - lka_queue_append(env, s, 0); + goto error; + } else if (! lka_queue_append(env, s, 0)) + goto error; + return; + +error: + status = S_MESSAGE_PERMFAILURE; + imsg_compose_event(env->sc_ievs[PROC_MFA], IMSG_LKA_RCPT, + s->message.id, 0, -1, &status, sizeof status); + lka_clear_expandtree(&s->expandtree); + lka_clear_deliverylist(&s->deliverylist); + lka_session_destroy(env, s); } -void +int lka_queue_append(struct smtpd *env, struct lkasession *s, int status) { struct path *path; @@ -684,21 +664,28 @@ lka_queue_append(struct smtpd *env, stru const char *errstr; char *sep; uid_t uid; + int ret; path = TAILQ_FIRST(&s->deliverylist); - if (path == NULL || status) { imsg_compose_event(env->sc_ievs[PROC_MFA], IMSG_LKA_RCPT, s->message.id, 0, -1, &status, sizeof status); lka_clear_expandtree(&s->expandtree); lka_clear_deliverylist(&s->deliverylist); lka_session_destroy(env, s); - return; + return 0; } /* send next item to queue */ message = s->message; - lka_expand(path->rule.r_value.path, sizeof(path->rule.r_value.path), path, &message.sender); + log_debug("lka_expand: before: [%s]", path->rule.r_value.path); + ret = lka_expand(path->rule.r_value.path, sizeof(path->rule.r_value.path), path, &message.sender); + log_debug("lka_expand: after: [%s]", path->rule.r_value.path); + if (! ret) { + log_debug("lka_expand: returned failure."); + return 0; + } + message.recipient = *path; sep = strchr(message.session_hostname, '@'); if (sep) { @@ -715,6 +702,7 @@ lka_queue_append(struct smtpd *env, stru s->id, 0, -1, &message, sizeof message); TAILQ_REMOVE(&s->deliverylist, path, entry); free(path); + return 1; } int -- Gilles Chehade freelance developer/sysadmin/consultant http://www.poolp.org