[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#701178: marked as done (preapproval unblock: dspam/dspam/3.10.1+dfsg-9)

Your message dated Fri, 1 Mar 2013 16:11:08 +0100
with message-id <20130301151108.GH5761@radis.cristau.org>
and subject line Re: Bug#701178: preapproval unblock: dspam/dspam/3.10.1+dfsg-9
has caused the Debian Bug report #701178,
regarding preapproval unblock: dspam/dspam/3.10.1+dfsg-9
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@bugs.debian.org
immediately.)


-- 
701178: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=701178
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems
--- Begin Message --- 
Package: release.debian.org
Severity: normal
User: release.debian.org@packages.debian.org
Usertags: unblock

Please unblock package dspam

[Sorry Jonathan for the duplicate]

Current dspam is affected by a corruption of message headers when
releasing from quarantine. This has for effect that mails are lost when
releasing from quarantine. A patch has been commited upstream but there
is some concern about buffer overflow. Hence, although the patch was
initially backported in sid, it was subsequently removed. See [1] for
the previous discussion.

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=698701#32

Today I've been working on a patch to solve the problem without security
concern. The patch is not pretty, I know it, but it should do its job.

The patch makes sure recipient points to an area of size at least 256
bytes. It wasn't always the case initially. The list of place where
recipient pointer is set is:

% egrep -RIn "recipient[^s]*=" src 
src/dspam.c:503:        ATX->recipient = CTX->username;
src/dspam.c:948:    ATX->recipient=args;
src/dspam.c:1675:      ATX->recipient = node_rcpt->ptr;
src/dspam.c:1683:      ATX->recipient = node_nt->ptr;
src/dspam.c:1694:      ATX->recipient = mailbox;

mailbox and args are of respective size 256 and 1024 bytes.
node_rcpt->ptr and node_nt->ptr on the other hand are exactly the size
of the string. They are allocated when calling nt_add (which call
nt_node_create).

Thus, the approach is to copy node_rcpt->ptr and node_nt->ptr into an
array of size 256 as well and this size can be used to limit the strlcpy
when copying CTX->username to ATX->recipient. I don't like to hardcode
the size but didn't find anything better for now. I'll forward upstream
and let him find a long term solution.

See attached debdiff for details.

Would you agree for an upload of this new package to tpu with sufficient
testing in unstable before?

unblock dspam/dspam/3.10.1+dfsg-9

-- System Information:
Debian Release: 7.0
  APT prefers unstable
  APT policy: (990, 'unstable'), (500, 'stable-updates'), (500, 'testing'), 
(500, 'stable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 3.2.0-4-amd64 (SMP w/4 CPU cores)
Locale: LANG=fr_FR.UTF-8, LC_CTYPE=fr_FR.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

diff -Nru dspam-3.10.2+dfsg/debian/changelog dspam-3.10.2+dfsg/debian/changelog
--- dspam-3.10.2+dfsg/debian/changelog	2013-02-11 14:55:20.000000000 +0100
+++ dspam-3.10.2+dfsg/debian/changelog	2013-02-22 11:54:57.000000000 +0100
@@ -1,3 +1,10 @@
+dspam (3.10.2+dfsg-7) unstable; urgency=low
+
+  * Add a new version of the patch fixing recipient corruption when releasing
+    a message from quarantine (Closes: #698136).
+
+ -- Thomas Preud'homme <robotux@debian.org>  Fri, 22 Feb 2013 11:28:17 +0100
+
 dspam (3.10.2+dfsg-6) unstable; urgency=low
 
   * Drop patch fixing recipient corruption when releasing a message from
diff -Nru dspam-3.10.2+dfsg/debian/patches/009_fix_recipient_corruption_when_releasing_message_from_quarantine.diff dspam-3.10.2+dfsg/debian/patches/009_fix_recipient_corruption_when_releasing_message_from_quarantine.diff
--- dspam-3.10.2+dfsg/debian/patches/009_fix_recipient_corruption_when_releasing_message_from_quarantine.diff	1970-01-01 01:00:00.000000000 +0100
+++ dspam-3.10.2+dfsg/debian/patches/009_fix_recipient_corruption_when_releasing_message_from_quarantine.diff	2013-02-22 11:54:57.000000000 +0100
@@ -0,0 +1,53 @@
+Description: Fix recipient corruption when releasing a message from quarantine
+
+When releasing mail from quarantine, dspam corrupts the FROM part in the
+SMTP/LMTP handshake.
+
+Author: Allan Ievers <aimail-dspam_users@rearden.com>
+Origin: vendor
+Bug-Debian: http://bugs.debian.org/698136
+Forwarded: no
+Last-Update: 2013-01-14
+
+--- a/src/dspam.c
++++ b/src/dspam.c
+@@ -499,8 +499,9 @@ process_message (
+                 ATX->train_pristine = 1;
+         }
+ 
+-        /* Change also the mail recipient */
+-        ATX->recipient = CTX->username;
++        /* Change also the mail recipient. ATX->recipient either points to
++	 * recipient[] or mailbox[] in process_users, hence the size of 256 */
++        strlcpy(ATX->recipient, CTX->username, 256);
+ 
+       }
+     }
+@@ -1634,6 +1635,7 @@ int process_users(AGENT_CTX *ATX, buffer *message) {
+     char filename[MAX_FILENAME_LENGTH];
+     int optin, optout;
+     char *username = NULL;
++    char recipient[256];
+ 
+     /* If ServerParameters specifies a --user, there will only be one
+      * instance on the stack, but possible multiple recipients. So we
+@@ -1672,7 +1674,7 @@ int process_users(AGENT_CTX *ATX, buffer *message) {
+ 	username = node_nt->ptr;
+ 
+     if (node_rcpt) {
+-      ATX->recipient = node_rcpt->ptr;
++      strlcpy(recipient, node_rcpt->ptr, sizeof(recipient));
+       node_rcpt = c_nt_next (ATX->recipients, &c_rcpt);
+     } else {
+ 
+@@ -1680,8 +1682,9 @@ int process_users(AGENT_CTX *ATX, buffer *message) {
+       if (have_rcpts)
+         break;
+ 
+-      ATX->recipient = node_nt->ptr;
++      strlcpy(recipient, node_nt->ptr, sizeof(recipient));
+     }
++    ATX->recipient = recipient;
+ 
+       /* If support for "+detail" is enabled, save full mailbox name for
+          delivery and strip detail for processing */
diff -Nru dspam-3.10.2+dfsg/debian/patches/series dspam-3.10.2+dfsg/debian/patches/series
--- dspam-3.10.2+dfsg/debian/patches/series	2013-02-11 14:55:20.000000000 +0100
+++ dspam-3.10.2+dfsg/debian/patches/series	2013-02-22 11:54:57.000000000 +0100
@@ -5,3 +5,4 @@
 006_default-daemon-port.diff
 007_process_quarantine_if_spanish.diff
 008_fix_exim_integration_doc.diff
+009_fix_recipient_corruption_when_releasing_message_from_quarantine.diff

--- End Message ---
--- Begin Message --- 
On Fri, Mar  1, 2013 at 16:06:21 +0100, Thomas Preud'homme wrote:

> Le vendredi 1 mars 2013 14:00:07, Julien Cristau a écrit :
> > Actually...
> > 
> 
> > 
> > Seems like ATX->recipient now points somewhere on the stack, and thus in
> > la-la-land at the end of the loop in process_users.  Is there any
> > guarantee it's not reused after that?  The scoping is kind of
> > non-obvious...
> 
> Indeed, it's not obvious. So ATX->recipient points to one of 3 buffers:
> 
> * args[1024] in deliver_message()
> * recipient[256] in the main while loop of process_users()
> * mailbox[256] in process_users()
> 
> I've checked all the place where recipient is used (grep -ERIn recipient | 
> grep ^src) and it's used only in 5 functions:
> 
Thanks for doing this.  Unblocked.

Cheers,
Julien

Attachment: signature.asc
Description: Digital signature


--- End Message ---
Reply to: