count single \r or \n as \r\n while checking line length against MAXHEADERSIZE
Florian Schlichting
fschlich at CIS.FU-Berlin.DE
Fri Oct 22 15:12:18 UTC 2010
Hi Julien,
> >I'm having second thoughts on this issue. Today I noticed that 20-30
> >articles get rejected every day due to References: headers longer than
> >998 bytes (but almost always shorter than 1024 bytes)
>
> I agree we should do something for it.
> I will have to see where the headers are used elsewhere in the code.
> I do not know whether innd copes with an arbitrary length for a header
> line. (Well, I believe it does because a field can be wrapped on an
> arbitrary number of lines.) We just have to be sure that it works well
> at the time of treating the initial buffer containing the message.
> >Unfortunately, nnrpd currently relies on innd to check header line
> >length. So I've tried to implement the check in nnrpd, but I think the
> >error handling looks ugly...
>
> The check should indeed be in nnrpd.
> I will have a look at your patch. Thanks.
please find below the complete patch. I have been running with it on our
feeder for a month now without issues.
Also, I looked into adding a test for nnrpd similar to artparse.t for
innd, but it's not so simple; ARTpost() in nnrpd/post.c probably needs
to be rewritten / refactored for it to be feasible. Have you ever
thought about that?
Florian
Subject: [PATCH] innd/art.c, nnrpd/post.c: check MAXARTLINELENGTH in nnrpd, not innd
this patch means reverting my previous "CRwithoutLF" patch and also
ripping out all length checking and both unsigned long length as well as
data->LastCRLF
---
innd/art.c | 22 ++--------------------
innd/innd.h | 3 ---
innd/nc.c | 1 -
nnrpd/post.c | 14 +++++++++++---
tests/innd/artparse-t.c | 4 ++--
5 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/innd/art.c b/innd/art.c
index b23d0cb..a157b4d 100644
--- a/innd/art.c
+++ b/innd/art.c
@@ -730,7 +730,7 @@ ARTprepare(CHANNEL *cp)
}
data->Lines = data->HeaderLines = data->CRwithoutLF = data->LFwithoutCR = 0;
data->DotStuffedLines = 0;
- data->CurHeader = data->LastCRLF = data->Body = cp->Start;
+ data->CurHeader = data->Body = cp->Start;
data->BytesHeader = NULL;
data->Feedsite = "?";
data->FeedsiteLength = strlen(data->Feedsite);
@@ -807,18 +807,12 @@ ARTparseheader(CHANNEL *cp)
struct buffer *bp = &cp->In;
ARTDATA *data = &cp->Data;
size_t i;
- unsigned long length;
for (i = cp->Next; i < bp->used; i++) {
if (bp->data[i] == '\0')
ARTerror(cp, "Nul character in header");
- if (bp->data[i] == '\n') {
+ if (bp->data[i] == '\n')
data->LFwithoutCR++;
- /* Behave as though we really saw a new line (otherwise,
- * the length of the header line may be exceeded in
- * non-compliant messages). */
- data->LastCRLF = i;
- }
if (bp->data[i] != '\r')
continue;
@@ -853,13 +847,6 @@ ARTparseheader(CHANNEL *cp)
cp->Next = i + 5;
return;
} else if (bp->data[i + 1] == '\n') {
- length = i - data->LastCRLF - 1;
- if (data->LastCRLF == cp->Start)
- length++;
- /* length includes final CRLF. */
- if (length > MAXARTLINELENGTH)
- ARTerror(cp, "Header line too long (%lu bytes)", length);
-
/* Be a little tricky here. Normally, the headers end at the
first occurrence of \r\n\r\n, so since we've seen \r\n, we want
to advance i and then look to see if we have another one. The
@@ -883,13 +870,8 @@ ARTparseheader(CHANNEL *cp)
ARTparsebody(cp);
return;
}
- data->LastCRLF = i - 1;
} else {
data->CRwithoutLF++;
- /* Behave as though we really saw a new line (otherwise,
- * the length of the header line may be exceeded in
- * non-compliant messages). */
- data->LastCRLF = i;
}
}
cp->Next = i;
diff --git a/innd/innd.h b/innd/innd.h
index e1f8a32..c1bc003 100644
--- a/innd/innd.h
+++ b/innd/innd.h
@@ -262,9 +262,6 @@ typedef struct _ARTDATA {
size_t CurHeader; /* where current header starts.
this is used for folded header
it indicates offset from bp->Data */
- size_t LastCRLF; /* where last CRLF exists.
- indicates where last LF exists
- it indicates offset from bp->Data */
HDRCONTENT HdrContent[MAX_ARTHEADER];
/* includes system headers info */
bool AddAlias; /* Whether Pathalias should be added
diff --git a/innd/nc.c b/innd/nc.c
index a5a3f3a..0fad0b3 100644
--- a/innd/nc.c
+++ b/innd/nc.c
@@ -1581,7 +1581,6 @@ NCproc(CHANNEL *cp)
cp->State == CSeatarticle) {
/* adjust offset only in CSgetheader, CSgetbody or CSeatarticle */
data->CurHeader -= cp->Start;
- data->LastCRLF -= cp->Start;
data->Body -= cp->Start;
if (data->BytesHeader != NULL)
data->BytesHeader -= cp->Start;
diff --git a/nnrpd/post.c b/nnrpd/post.c
index 6b01290..580ec43 100644
--- a/nnrpd/post.c
+++ b/nnrpd/post.c
@@ -164,12 +164,21 @@ TrimSpaces(char *p)
static char *
NextHeader(char *p)
{
- for ( ; (p = strchr(p, '\n')) != NULL; p++) {
+ char *q;
+
+ for (q = p; (p = strchr(p, '\n')) != NULL; p++) {
+ if (p - q > MAXARTLINELENGTH) {
+ strlcpy(Error, "Header line too long",
+ sizeof(Error));
+ return NULL;
+ }
if (ISWHITE(p[1]))
continue;
*p = '\0';
return p + 1;
}
+ strlcpy(Error, "Article has no body -- just headers",
+ sizeof(Error));
return NULL;
}
@@ -226,8 +235,7 @@ StripOffHeaders(char *article)
/* Get start of next header; if it's a blank line, we hit the end. */
if ((p = NextHeader(p)) == NULL) {
- strlcpy(Error, "Article has no body -- just headers",
- sizeof(Error));
+ /* Error set in NextHeader() */
return NULL;
}
if (*p == '\n')
diff --git a/tests/innd/artparse-t.c b/tests/innd/artparse-t.c
index e3055f0..560c225 100644
--- a/tests/innd/artparse-t.c
+++ b/tests/innd/artparse-t.c
@@ -37,9 +37,9 @@ const struct {
{ "../data/articles/bad-hdr-trunc",
"437 No colon-space in \"Test:\" header" },
{ "../data/articles/bad-long-cont",
- "437 Header line too long (1001 bytes)" },
+ "" },
{ "../data/articles/bad-long-hdr",
- "437 Header line too long (1001 bytes)" },
+ "" },
{ "../data/articles/bad-no-body",
"437 No body" },
{ "../data/articles/bad-no-header",
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5557 bytes
Desc: not available
URL: <https://lists.isc.org/pipermail/inn-workers/attachments/20101022/791fc32d/attachment.bin>
More information about the inn-workers
mailing list