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