From c3f8d0d94888ec3d82aab2c88f102bd9e986eaeb Mon Sep 17 00:00:00 2001 From: Paul Arthur Date: Sat, 2 Nov 2024 07:51:12 +0000 Subject: [PATCH] libopenarc: simplify and tweak header wrapping --- CHANGELOG.md | 1 + libopenarc/arc.c | 187 +++++++++++++------------------------------- libopenarc/arc.h | 2 +- test/test_milter.py | 4 +- util/arc-dstring.c | 36 +++++++-- util/arc-dstring.h | 5 +- 6 files changed, 93 insertions(+), 142 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f26a9d..b9758fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ All notable changes to this project will be documented in this file. - libopenarc - `arc_eom()` propagates internal errors like memory allocation failure instead of marking the chain as failed. - libopenarc - Signature fields are wrapped at the configured margin. +- libopenarc - Header margin wrapping is more accurate and precise. - milter - Use after free. - milter - Unlikely division by zero. - milter - Small memory leak during config loading. diff --git a/libopenarc/arc.c b/libopenarc/arc.c index 2fc4d58..251d7ee 100644 --- a/libopenarc/arc.c +++ b/libopenarc/arc.c @@ -370,11 +370,8 @@ arc_genamshdr(ARC_MESSAGE *msg, bool seal) { bool firsthdr; - int n; int status; - int delimlen; size_t hashlen; - char *format; unsigned char *hash; struct arc_hdrfield *hdr; unsigned char b64hash[ARC_MAXHEADER + 1]; @@ -383,45 +380,40 @@ arc_genamshdr(ARC_MESSAGE *msg, assert(dstr != NULL); assert(delim != NULL); - delimlen = strlen(delim); - /* - ** We need to generate an ARC-Message-Signature: header field template + ** We need to generate a signature header field template ** and include it in the canonicalization. */ - /* basic required stuff */ - if (sizeof(msg->arc_timestamp) == sizeof(unsigned long long)) - { - format = "i=%u;%sa=%s;%sd=%s;%ss=%s;%st=%llu"; - } - else if (sizeof(msg->arc_timestamp) == sizeof(unsigned long)) - { - format = "i=%u;%sa=%s;%sd=%s;%ss=%s;%st=%lu"; - } - else - { - format = "i=%u;%sa=%s;%sd=%s;%ss=%s;%st=%u"; - } + /* i= */ + arc_dstring_printf(dstr, "i=%u", msg->arc_nsets + 1); + + /* d= */ + arc_dstring_printf(dstr, ";%sd=%s", delim, msg->arc_domain); - (void) arc_dstring_printf(dstr, format, msg->arc_nsets + 1, delim, - arc_code_to_name(algorithms, msg->arc_signalg), - delim, msg->arc_domain, delim, msg->arc_selector, - delim, msg->arc_timestamp); + /* s= */ + arc_dstring_printf(dstr, ";%ss=%s", delim, msg->arc_selector); + + /* a= */ + arc_dstring_printf(dstr, ";%sa=%s", delim, + arc_code_to_name(algorithms, msg->arc_signalg)); if (seal) { + /* cv= */ arc_dstring_printf(dstr, ";%scv=%s", delim, arc_code_to_name(chainstatus, msg->arc_cstate)); } else { + /* c= */ arc_dstring_printf( dstr, ";%sc=%s/%s", delim, arc_code_to_name(canonicalizations, msg->arc_canonhdr), arc_code_to_name(canonicalizations, msg->arc_canonbody)); } + /* q= */ if (msg->arc_querymethods != NULL) { bool firstq = true; @@ -447,25 +439,17 @@ arc_genamshdr(ARC_MESSAGE *msg, } } + /* t= */ + arc_dstring_printf(dstr, ";%st=%ju", delim, (uintmax_t) msg->arc_timestamp); + + /* x= */ if (msg->arc_sigttl != 0) { - uint64_t expire; - - expire = msg->arc_timestamp + msg->arc_sigttl; - if (sizeof(expire) == sizeof(unsigned long long)) - { - arc_dstring_printf(dstr, ";%sx=%llu", delim, expire); - } - else if (sizeof(expire) == sizeof(unsigned long)) - { - arc_dstring_printf(dstr, ";%sx=%lu", delim, expire); - } - else - { - arc_dstring_printf(dstr, ";%sx=%u", delim, expire); - } + arc_dstring_printf(dstr, ";%sx=%ju", delim, + (uintmax_t) (msg->arc_timestamp + msg->arc_sigttl)); } + /* extra tags */ if (msg->arc_xtags != NULL) { struct arc_xtag *x; @@ -493,9 +477,6 @@ arc_genamshdr(ARC_MESSAGE *msg, return (size_t) -1; } - arc_base64_encode(hash, hashlen, b64hash, sizeof b64hash); - arc_dstring_printf(dstr, ";%sbh=%s", delim, b64hash); - /* l= */ if (msg->arc_partial) { @@ -513,61 +494,49 @@ arc_genamshdr(ARC_MESSAGE *msg, continue; } - if (!firsthdr) + if (firsthdr) { - arc_dstring_cat1(dstr, ':'); + arc_dstring_printf(dstr, ";%sh=", delim); + firsthdr = false; } else { - arc_dstring_cat1(dstr, ';'); - arc_dstring_catn(dstr, delim, delimlen); - arc_dstring_catn(dstr, "h=", 2); + arc_dstring_cat1(dstr, ':'); } - firsthdr = false; - arc_dstring_catn(dstr, hdr->hdr_text, hdr->hdr_namelen); } - if (msg->arc_library->arcl_oversignhdrs != NULL && - msg->arc_library->arcl_oversignhdrs[0] != NULL) + if (msg->arc_library->arcl_oversignhdrs != NULL) { - bool wrote = false; - - if (firsthdr) - { - arc_dstring_cat1(dstr, ';'); - arc_dstring_catn(dstr, delim, delimlen); - arc_dstring_catn(dstr, "h=", 2); - } - else - { - arc_dstring_cat1(dstr, ':'); - } - - for (n = 0; msg->arc_library->arcl_oversignhdrs[n] != NULL; n++) + for (int i = 0; msg->arc_library->arcl_oversignhdrs[i] != NULL; i++) { - if (msg->arc_library->arcl_oversignhdrs[n][0] == '\0') + if (msg->arc_library->arcl_oversignhdrs[i][0] == '\0') { continue; } - if (wrote) + if (firsthdr) + { + arc_dstring_printf(dstr, ";%sh=", delim); + firsthdr = false; + } + else { arc_dstring_cat1(dstr, ':'); } - arc_dstring_cat(dstr, msg->arc_library->arcl_oversignhdrs[n]); - - wrote = true; + arc_dstring_cat(dstr, msg->arc_library->arcl_oversignhdrs[i]); } } + + /* bh= */ + arc_base64_encode(hash, hashlen, b64hash, sizeof b64hash); + arc_dstring_printf(dstr, ";%sbh=%s", delim, b64hash); } /* and finally, an empty b= */ - arc_dstring_cat1(dstr, ';'); - arc_dstring_catn(dstr, delim, delimlen); - arc_dstring_catn(dstr, "b=", 2); + arc_dstring_printf(dstr, ";%sb=", delim); return arc_dstring_len(dstr); } @@ -662,9 +631,7 @@ arc_getamshdr_d( else { bool first = true; - bool forcewrap; int pvlen; - int whichlen; char *p; char *q; char *end; @@ -682,23 +649,6 @@ arc_getamshdr_d( *(q + 1) = '\0'; } - whichlen = strlen(which); - - /* force wrapping of "b=" ? */ - - forcewrap = false; - if (msg->arc_keytype == ARC_KEYTYPE_RSA) - { - unsigned int siglen; - - siglen = BASE64SIZE(msg->arc_keybits / 8); - if (strcmp(which, "b") == 0 && - len + whichlen + siglen + 1 >= msg->arc_margin) - { - forcewrap = true; - } - } - pvlen = strlen(pv); if (len == 0 || first) @@ -707,13 +657,16 @@ arc_getamshdr_d( len += pvlen; first = false; } - else if (forcewrap || len + pvlen > msg->arc_margin) + else if (len + pvlen + 1 > msg->arc_margin || + strcmp(which, "b") == 0 || strcmp(which, "bh") == 0 || + strcmp(which, "h") == 0) { arc_dstring_cat(msg->arc_hdrbuf, "\r\n\t"); len = 8; if (strcmp(which, "h") == 0) - { /* break at colons */ + { + /* break at colons */ bool ifirst = true; int tmplen; char *tmp; @@ -749,51 +702,23 @@ arc_getamshdr_d( } else if (strcmp(which, "b") == 0 || strcmp(which, "bh") == 0 || strcmp(which, "z") == 0) - { /* break at margins */ - int offset; - int n; - char *x; - char *y; - - offset = whichlen + 1; - - arc_dstring_catn(msg->arc_hdrbuf, which, whichlen); - arc_dstring_cat1(msg->arc_hdrbuf, '='); - - len += offset; - - x = pv + offset; - y = pv + pvlen; - - while (x < y) - { - if (msg->arc_margin - len == 0) - { - arc_dstring_cat(msg->arc_hdrbuf, "\r\n\t "); - len = 9; - } - - n = MIN(msg->arc_margin - len, y - x); - arc_dstring_catn(msg->arc_hdrbuf, x, n); - x += n; - len += n; - } + { + /* break at margins */ + arc_dstring_cat_wrap(msg->arc_hdrbuf, pv, msg->arc_margin, + &len); } else - { /* break at delimiter */ + { + /* don't break this field */ arc_dstring_catn(msg->arc_hdrbuf, pv, pvlen); len += pvlen; } } else { - if (!first) - { - arc_dstring_cat1(msg->arc_hdrbuf, ' '); - len += 1; - } + arc_dstring_cat1(msg->arc_hdrbuf, ' '); + len += 1; - first = false; arc_dstring_catn(msg->arc_hdrbuf, pv, pvlen); len += pvlen; } @@ -3584,7 +3509,7 @@ arc_getseal(ARC_MESSAGE *msg, } /* append it to the stub */ - arc_dstring_cat_wrap(dstr, (char *) b64sig, msg->arc_margin); + arc_dstring_cat_wrap(dstr, (char *) b64sig, msg->arc_margin, NULL); /* add it to the seal */ h = ARC_MALLOC(sizeof hdr); @@ -3683,7 +3608,7 @@ arc_getseal(ARC_MESSAGE *msg, } /* append it to the stub */ - arc_dstring_cat_wrap(dstr, (char *) b64sig, msg->arc_margin); + arc_dstring_cat_wrap(dstr, (char *) b64sig, msg->arc_margin, NULL); /* add it to the seal */ h = ARC_MALLOC(sizeof hdr); diff --git a/libopenarc/arc.h b/libopenarc/arc.h index f59abac..60e29f8 100644 --- a/libopenarc/arc.h +++ b/libopenarc/arc.h @@ -31,7 +31,7 @@ extern "C" { #define OPENARC_LIB_VERSION 0x00010000 /* definitions */ -#define ARC_HDRMARGIN 75 /* "standard" header margin */ +#define ARC_HDRMARGIN 78 /* standard email line length */ #define ARC_MAXHEADER 4096 /* buffer for caching one header */ #define ARC_MAXHOSTNAMELEN 256 /* max. FQDN we support */ #define ARC_MAXLINELEN 1000 /* physical line limit (RFC5321) */ diff --git a/test/test_milter.py b/test/test_milter.py index 8f54308..5cfe066 100644 --- a/test/test_milter.py +++ b/test/test_milter.py @@ -52,7 +52,7 @@ def _run_miltertest( assert '\r' not in msg[1]['value'] # Check for proper wrapping if msg[1]['name'] in ['ARC-Message-Signature', 'ARC-Seal']: - assert not any(len(x) > 80 for x in msg[1]['value'].splitlines()) + assert not any(len(x) > 78 for x in msg[1]['value'].splitlines()) ins_headers.insert(msg[1]['index'], [msg[1]['name'], msg[1]['value']]) elif msg[0] in miltertest.DISPOSITION_REPLIES: assert msg[0] == miltertest.SMFIR_ACCEPT @@ -520,7 +520,7 @@ def test_milter_idna(run_miltertest): ] ) - assert res['headers'][1][1].startswith(' i=1; a=rsa-sha256; d=시험.example.com; s=예;') + assert res['headers'][1][1].startswith(' i=1; d=시험.example.com; s=예;') assert res['headers'][2][1] == ' i=1; 시험.example.com; spf=pass smtp.mailfrom=привіт@시험.example.com;\n\tarc=none smtp.remote-ip=127.0.0.1' res = run_miltertest(res['headers']) diff --git a/util/arc-dstring.c b/util/arc-dstring.c index fa0c629..84748bd 100644 --- a/util/arc-dstring.c +++ b/util/arc-dstring.c @@ -385,31 +385,47 @@ arc_dstring_catn(struct arc_dstring *dstr, const char *str, size_t nbytes) * dstr: dstring to modify * str: string to append * margin: maximum line length, not including CRLF + * newlen: pointer to store the length of the last line in (optional) * * Returns: * Whether the operation succeeded. */ bool -arc_dstring_cat_wrap(struct arc_dstring *dstr, const char *str, size_t margin) +arc_dstring_cat_wrap(struct arc_dstring *dstr, + const char *str, + size_t margin, + size_t *newlen) { size_t len; size_t slen; - if (margin < 3) + if (margin == 0) { - /* margin is either disabled or nonsensical */ + /* margin is disabled */ return arc_dstring_cat(dstr, str); } + if (margin < 10) + { + margin = 10; + } /* find the current line length */ len = 0; for (char *p = dstr->ds_buf; *p; p++) { - len++; if (*p == '\n') { len = 0; } + else if (*p == '\t') + { + /* we're actually measuring visual indentation */ + len += 8; + } + else + { + len++; + } } if (len >= margin) @@ -435,7 +451,7 @@ arc_dstring_cat_wrap(struct arc_dstring *dstr, const char *str, size_t margin) } } - for (const char *p = str + len; p < str + slen; p += margin - 2) + for (const char *p = str + len; p < str + slen; p += margin - 9) { if (!arc_dstring_cat(dstr, "\r\n\t ")) { @@ -443,9 +459,9 @@ arc_dstring_cat_wrap(struct arc_dstring *dstr, const char *str, size_t margin) } len = strlen(p); - if (len > margin - 2) + if (len > margin - 9) { - len = margin - 2; + len = margin - 9; } if (!arc_dstring_catn(dstr, p, len)) @@ -453,6 +469,12 @@ arc_dstring_cat_wrap(struct arc_dstring *dstr, const char *str, size_t margin) return false; } } + + if (newlen) + { + *newlen = len + 9; + } + return true; } diff --git a/util/arc-dstring.h b/util/arc-dstring.h index dec7fcf..d11ddac 100644 --- a/util/arc-dstring.h +++ b/util/arc-dstring.h @@ -27,7 +27,10 @@ extern void arc_dstring_blank(struct arc_dstring *); extern bool arc_dstring_cat(struct arc_dstring *, const char *); extern bool arc_dstring_cat1(struct arc_dstring *, int); extern bool arc_dstring_catn(struct arc_dstring *, const char *, size_t); -extern bool arc_dstring_cat_wrap(struct arc_dstring *, const char *, size_t); +extern bool arc_dstring_cat_wrap(struct arc_dstring *, + const char *, + size_t, + size_t *); extern bool arc_dstring_copy(struct arc_dstring *, const char *); extern void arc_dstring_strip(struct arc_dstring *, const char *); extern void arc_dstring_free(struct arc_dstring *);