On Fri, Mar 19, 2021 at 11:18:27AM -0400, Jaroslav Skarvada wrote:
> 14. postfix-3.5.8/src/util/dict_inline.c:124: uninit_use_in_call: Using
> uninitialized value "value" when calling "dict_file_to_b64".
> 17. postfix-3.5.8/src/util/dict_inline.c:125: overwrite_var: Overwriting
> "err" in "err = free_me = dict_file_get_error(dict)" leaks the storage that
> "err" points to.
> # 123|
> # 124|-> if ((base64_buf = dict_file_to_b64(dict, value)) == 0) {
> # 125|-> err = free_me = dict_file_get_error(dict);
> # 126| break;
> # 127| }
>
> I think it could call dict_file_to_b64 with uninitialized value.
Yes, when inline tables in the main.cf file are malformed in a
particular way, this may not be handled correctly. Patch below.
> 61. postfix-3.5.8/src/util/dict_inline.c:131:2: note: 2nd function call
> argument is an uninitialized value
> # dict->update(dict, vname, value);
> # ^ ~~~~~
> # 129| }
> # 130| /* No duplicate checks. See comments in dict_thash.c. */
> # 131|-> dict->update(dict, vname, value);
> # 132| count += 1;
> # 133| }
Same dict_inline issue as above...
> 7. postfix-3.5.8/src/global/haproxy_srvr.c:456: dereference: Dereferencing a
> pointer that might be "NULL" "mystrtok(&cp, " \r")" when calling
> "haproxy_srvr_parse_proto".
>
> # 454| else if (haproxy_srvr_parse_lit(NEXT_TOKEN, "PROXY", (char *)
> 0) < 0)
> # 455| err = "unexpected protocol header";
> # 456|-> else if (haproxy_srvr_parse_proto(NEXT_TOKEN, &addr_family) < 0)
> # 457| err = "unsupported protocol type";
> # 458| else if (haproxy_srvr_parse_addr(NEXT_TOKEN, smtp_client_addr,
>
> I think for malformed input it could fail by calling strncasecmp with NULL.
Yes, parse_proto is missing a NULL check. Patch below.
> 1. postfix-3.5.8/src/tlsproxy/tlsproxy.c:1881:49: warning[-Wmissing-braces]:
> missing braces around initializer
> # static const CONFIG_STR_TABLE str_table[] = {
> # ^
> # 1879| 0,
> # 1880| };
> # 1881|-> static const CONFIG_STR_TABLE str_table[] = {
> # 1882| VAR_TLSP_TLS_CHAIN_FILES, DEF_TLSP_TLS_CHAIN_FILES,
> &var_tlsp_tls_chain_files, 0, 0,
> # 1883| VAR_TLSP_TLS_CERT_FILE, DEF_TLSP_TLS_CERT_FILE,
> &var_tlsp_tls_cert_file, 0, 0,
>
> I think it should have braces around table lines. There are multiple tables
> with the same warning.
The table content is correct as written, and I don't expect we'll be
changing it. But if Wietse is inclined to rototill all the tables...
> 2. postfix-3.5.8/src/smtp/smtp_session.c:200: var_compare_op: Comparing
> "session->stream" to null implies that "session->stream" might be null.
> 5. postfix-3.5.8/src/smtp/smtp_session.c:208: var_deref_model: Passing null
> pointer "session->stream" to "tls_session_stop", which dereferences it.
> # 206| tls_proxy_context_free(session->tls_context);
> # 207| else
> # 208|-> tls_client_stop(smtp_tls_ctx, session->stream,
> # 209| var_smtp_starttls_tmout, 0,
> session->tls_context);
> # 210| }
>
> It's suspicious that there is the NULL check, but later it could fail on NULL
> dereference.
In that particular branch the stream is sure to be non-null.
> 7. postfix-3.5.8/src/smtpd/smtpd.c:1712: var_compare_op: Comparing
> "state->milters" to null implies that "state->milters" might be null.
> 9. postfix-3.5.8/src/smtpd/smtpd.c:1727: var_deref_model: Passing "state" to
> "mail_reset", which dereferences null "state->milters".
> # 1725| helo_reset(state);
> # 1726| chat_reset(state, var_smtpd_hist_thrsh);
> # 1727|-> mail_reset(state);
> # 1728| rcpt_reset(state);
> # 1729| state->helo_name = mystrdup(printable(argv[1].strval, '?'));
>
> It's suspicious that there is the NULL check, but later it could fail on NULL
> dereference.
The flag that guards the milter dereference is only set when the milter
pointer is not null.
> 31. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:335:9: note: Access to
> field 'next' results in a dereference of a null pointer (loaded from variable
> 'tp')
> # if (tp->next)
> # ^~
> # 333| static void tls_proxy_client_tlsa_free(TLS_TLSA *tp)
> # 334| {
> # 335|-> if (tp->next)
> # 336| tls_proxy_client_tlsa_free(tp->next);
> # 337| myfree(tp->mdalg);
> 37. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:324:9: note: Access to
> field 'next' results in a dereference of a null pointer (loaded from variable
> 'tp')
> # if (tp->next)
> # ^~
> # 322| static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
> # 323| {
> # 324|-> if (tp->next)
> # 325| tls_proxy_client_pkeys_free(tp->next);
> # 326| if (tp->pkey)
> 37. postfix-3.5.8/src/tls/tls_proxy_client_scan.c:313:9: note: Access to
> field 'next' results in a dereference of a null pointer (loaded from variable
> 'tp')
> # if (tp->next)
> # ^~
> # 311| static void tls_proxy_client_certs_free(TLS_CERTS *tp)
> # 312| {
> # 313|-> if (tp->next)
> # 314| tls_proxy_client_certs_free(tp->next);
> # 315| if (tp->cert)
This code has been replaced in Postfix 3.6, patch for earlier versions
below my signature. The NPE can only arise when internal communication
protocols are interrupted midstream.
> 88. postfix-3.5.8/src/global/maillog_client.c:256:10: note: Null pointer
> passed to 2nd parameter expecting 'nonnull'
> # if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
> # ^ ~~~~~~~~~~~~
> # 254| msg_info("export %s=%s", POSTLOG_SERVICE_ENV, service_path);
> # 255| #endif
> # 256|-> if (setenv(POSTLOG_SERVICE_ENV, service_path, 1) < 0)
> # 257| msg_fatal("setenv: %m");
> # 258| }
This looks plausible, but I'm not sure what the right fix is.
> 19. postfix-3.5.8/include/vstring.h:70:36: note: expanded from macro
> 'vstring_str'
> # #define vstring_str(vp) ((char *) (vp)->vbuf.data)
> # ^~~~~~~~~~~~~~~
> # 72| vstring_sprintf(canon_name, "%s/%s", tag, argv0);
> # 73| }
> # 74|-> return (vstring_str(canon_name));
> # 75| }
This is a non-problem, the first call always sets a non-null value, but
I'm adding a harmless fallback, just in case argv[0] is NULL.
----------
Patch for Postfix 3.5 and 3.6 (perhaps also earlier versions?)
---------
--- a/src/global/haproxy_srvr.c
+++ b/src/global/haproxy_srvr.c
@@ -201,6 +201,8 @@ static int haproxy_srvr_parse_proto(const char *str, int
*addr_family)
if (msg_verbose)
msg_info("haproxy_srvr_parse: proto=%s", STR_OR_NULL(str));
+ if (str == 0)
+ return (-1);
#ifdef AF_INET6
if (strcasecmp(str, "TCP6") == 0) {
if (strchr((char *) proto_info->sa_family_list, AF_INET6) != 0) {
--- a/src/global/mail_task.c
+++ b/src/global/mail_task.c
@@ -71,5 +71,7 @@ const char *mail_task(const char *argv0)
mail_conf_eval(DEF_SYSLOG_NAME);
vstring_sprintf(canon_name, "%s/%s", tag, argv0);
}
- return (vstring_str(canon_name));
+ if (canon_name)
+ return (vstring_str(canon_name));
+ return ("unknown");
}
--- a/src/util/dict_inline.c
+++ b/src/util/dict_inline.c
@@ -113,9 +113,11 @@ DICT *dict_inline_open(const char *name, int open_flags,
int dict_flags)
dict = dict_open3(DICT_TYPE_HT, name, open_flags, dict_flags);
dict_type_override(dict, DICT_TYPE_INLINE);
while ((nameval = mystrtokq(&cp, CHARS_COMMA_SP, CHARS_BRACE)) != 0) {
- if ((nameval[0] != CHARS_BRACE[0]
- || (err = free_me = extpar(&nameval, CHARS_BRACE,
EXTPAR_FLAG_STRIP)) == 0)
- && (err = split_qnameval(nameval, &vname, &value)) != 0)
+ if (nameval[0] != CHARS_BRACE[0])
+ err = free_me = extpar(&nameval, CHARS_BRACE, EXTPAR_FLAG_STRIP);
+ if (!err)
+ err = split_qnameval(nameval, &vname, &value);
+ if (err)
break;
if ((dict->flags & DICT_FLAG_SRC_RHS_IS_FILE) != 0) {
--
Viktor.
----------
Below patch for Postfix 3.5 only:
----------
--- a/postfix/src/tls/tls_proxy_client_scan.c
+++ b/postfix/src/tls/tls_proxy_client_scan.c
@@ -310,6 +310,8 @@ int tls_proxy_client_init_scan(ATTR_SCAN_MASTER_FN
scan_fn, VSTREAM *fp,
static void tls_proxy_client_certs_free(TLS_CERTS *tp)
{
+ if (tp == 0)
+ return;
if (tp->next)
tls_proxy_client_certs_free(tp->next);
if (tp->cert)
@@ -321,6 +323,8 @@ static void tls_proxy_client_certs_free(TLS_CERTS *tp)
static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
{
+ if (tp == 0)
+ return;
if (tp->next)
tls_proxy_client_pkeys_free(tp->next);
if (tp->pkey)
@@ -332,6 +336,8 @@ static void tls_proxy_client_pkeys_free(TLS_PKEYS *tp)
static void tls_proxy_client_tlsa_free(TLS_TLSA *tp)
{
+ if (tp == 0)
+ return;
if (tp->next)
tls_proxy_client_tlsa_free(tp->next);
myfree(tp->mdalg);
