Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: exim: dev

DCC header corruption

 

 

exim dev RSS feed   Index | Next | Previous | View Threaded


stu at metanate

May 17, 2012, 2:40 AM

Post #1 of 11 (825 views)
Permalink
DCC header corruption

I have recently been experiencing corruption and/or truncation of the DCC header being logged and added to my messages using the built-in exim dcc support in the data acl.

Upon investigation, I note that in dcc.c the global dcc_header is being pointed at a stack based string before the dcc code returns, despite the fact that there appears to be a global dcc_header_str which is presumably intended to have the header copied in to it and then dcc_header pointed at that.

Certainly replacing the following line (483 in src/dcc.c):

dcc_header = xhdr;

with

strncpy(dcc_header = dcc_header_str, xhdr, sizeof(dcc_header_str));

seems to cure the problem for me, although being unfamiliar with Exim's source base, it's not clear to me if this is the correct solution.

Regards

Stu


--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


stu at metanate

May 18, 2012, 3:53 AM

Post #2 of 11 (803 views)
Permalink
DCC header corruption [In reply to]

(Second attempt at submitting this as first message seems to have disappeared down a black hole.)

I have recently been experiencing corruption and/or truncation of the DCC header being logged and added to my messages using the built-in exim dcc support in the data acl.

Upon investigation, I note that in dcc.c the global dcc_header is being pointed at a stack based string before the dcc code returns, despite the fact that there appears to be a (currently unused) global char array dcc_header_str which is presumably intended to have the header copied in to it and then dcc_header pointed at that.

Replacing the following line (483 in my copy of src/dcc.c):

dcc_header = xhdr;

with

strncpy(dcc_header = dcc_header_str, xhdr, sizeof(dcc_header_str));

seems to cure the problem for me, although being unfamiliar with Exim's source base, it's not clear to me if this is the correct solution.

Regards

Stu


--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


wbreyha at gmx

May 18, 2012, 7:38 AM

Post #3 of 11 (796 views)
Permalink
Re: DCC header corruption [In reply to]

Northfield Stuart wrote, on 18.05.2012 12:53:
> seems to cure the problem for me, although being unfamiliar with Exim's
> source base, it's not clear to me if this is the correct solution.

I'll look into it. What type of corruption did you notice? And how do you use
dcc in your ACLs? If you don't want to disclose details of your config on the
mailing list feel free to send it directly.

Greetings, Wolfgang
--
Wolfgang Breyha <wbreyha [at] gmx> | http://www.blafasel.at/
Vienna University Computer Center | Austria


--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


wbreyha at gmx

May 18, 2012, 7:46 AM

Post #4 of 11 (792 views)
Permalink
Re: DCC header corruption [In reply to]

Hi!

Northfield Stuart wrote, on 18.05.2012 12:53:
> Upon investigation, I note that in dcc.c the global dcc_header is being
> pointed at a stack based string before the dcc code returns, despite the
> fact that there appears to be a (currently unused) global char array
> dcc_header_str which is presumably intended to have the header copied in to
> it and then dcc_header pointed at that.
>
> Replacing the following line (483 in my copy of src/dcc.c):
>
> dcc_header = xhdr;
>
> with
>
> strncpy(dcc_header = dcc_header_str, xhdr, sizeof(dcc_header_str));

After a first look ... you're absolutely right. I didn't recognized that,
because I use "direct_add_header";-)

I'll file a bug in the evening to hopefully get the fix in 4.80.

Greetings, Wolfgang
--
Wolfgang Breyha <wbreyha [at] gmx> | http://www.blafasel.at/
Vienna University Computer Center | Austria


--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


stu at metanate

May 18, 2012, 8:03 AM

Post #5 of 11 (797 views)
Permalink
Re: DCC header corruption [In reply to]

>> Replacing the following line (483 in my copy of src/dcc.c):
>>
>> dcc_header = xhdr;
>>
>> with
>>
>> strncpy(dcc_header = dcc_header_str, xhdr, sizeof(dcc_header_str));
>
> After a first look ... you're absolutely right. I didn't recognized that,
> because I use "direct_add_header";-)
>
> I'll file a bug in the evening to hopefully get the fix in 4.80.


Thanks Wolfgang.

For the record, I believe the issue surface when I changed my data acl use of dcc from this:

warn message = $dcc_header
log_message = DCC: $dcc_header
dcc = */defer_ok

to the following:

warn dcc = */defer_ok
add_header = $dcc_header
log_message = DCC: $dcc_header

The effect I observe is that either the string is truncated and appears thus:

X-Dcc-Metrics: mop.metanate.com 1290; Body=1 Fuz1=man
X-Dcc-Metrics: mop.metanate.com 1290; Body=1 Fuz1=1 F

or alternatively, garbage characters overwrite the end of the header thus:

X-Dcc-Metrics: Mop.local 1290; Body=52 Fuz1=69 Fuz2=m€E¿ÿö

a combination which suggested to me that the string buffer was being overwritten - which would tie in with returning a pointer to a stack based string.

I guess the change in the order of things in my acl changed the memory usage sufficiently to cause the freed stack memory to be reused and corrupt the string.

Regards

Stu

--
Stuart Northfield
+44 (0) 1223 566759 (Direct), +44 (0) 1223 566727 (Fax)
Metanate Limited. Registered in England No 4046086 at:
Lincoln House, Station Court, Great Shelford, Cambridge CB22 5NE, UK
www.metanate.com (Consultancy) www.schemus.com (Data synchronisation)

This e-mail and all attachments it may contain is confidential and
intended solely for the use of the individual to whom it is addressed.
Any views or opinions presented are those of the author and do not
necessarily represent those of Metanate Ltd. If you are not the
intended recipient, be advised that you have received this e-mail in
error and that any use, dissemination, printing, forwarding or copying
of this e-mail is strictly prohibited. Please contact the sender if
you have received this e-mail in error.





--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


marcin at mejor

May 18, 2012, 9:32 AM

Post #6 of 11 (793 views)
Permalink
Re: DCC header corruption [In reply to]

W dniu 18.05.2012 12:53, Northfield Stuart pisze:
> (Second attempt at submitting this as first message seems to have disappeared down a black hole.)
>
> I have recently been experiencing corruption and/or truncation of the DCC header being logged and added to my messages using the built-in exim dcc support in the data acl.
>
> Upon investigation, I note that in dcc.c the global dcc_header is being pointed at a stack based string before the dcc code returns, despite the fact that there appears to be a (currently unused) global char array dcc_header_str which is presumably intended to have the header copied in to it and then dcc_header pointed at that.
>
> Replacing the following line (483 in my copy of src/dcc.c):
>
> dcc_header = xhdr;
>
> with
>
> strncpy(dcc_header = dcc_header_str, xhdr, sizeof(dcc_header_str));
>
> seems to cure the problem for me, although being unfamiliar with Exim's source base, it's not clear to me if this is the correct solution.

Hello!
Could it be problem founded by clang analyzer[1]? After applying your
patch those report disappear from scan result, this is why i suspect
clang analyzer found real problem. I appreciative feedback if clang
anaylze was correct or wrong.
Regards,
Marcin

[1] - http://mejor.pl/exim-4.77/report-QnPhZj.html#EndPath

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


pdp at exim

May 18, 2012, 12:33 PM

Post #7 of 11 (796 views)
Permalink
Re: DCC header corruption [In reply to]

On 2012-05-18 at 18:32 +0200, Marcin Mirosław wrote:
> Could it be problem founded by clang analyzer[1]? After applying your
> patch those report disappear from scan result, this is why i suspect
> clang analyzer found real problem. I appreciative feedback if clang
> anaylze was correct or wrong.

It was correct.

I put a bunch of work (March 2011) into 4.76 to improve Exim's clang
compatibility and to clean up a bunch of bits where the coding style had
led to many compiler warnings. So we're getting better here.

-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


marcin at mejor

May 18, 2012, 1:26 PM

Post #8 of 11 (798 views)
Permalink
Re: DCC header corruption [In reply to]

W dniu 2012-05-18 21:33, Phil Pennock pisze:
> On 2012-05-18 at 18:32 +0200, Marcin Miros³aw wrote:
>> Could it be problem founded by clang analyzer[1]? After applying your
>> patch those report disappear from scan result, this is why i suspect
>> clang analyzer found real problem. I appreciative feedback if clang
>> anaylze was correct or wrong.
>
> It was correct.
>
> I put a bunch of work (March 2011) into 4.76 to improve Exim's clang
> compatibility and to clean up a bunch of bits where the coding style had
> led to many compiler warnings. So we're getting better here.

Thanks for answer. May i ask for another review?
This reports are commented in the same way as report about dcc: "Stack
address stored into global variable"
http://mejor.pl/exim-4.77/report-IORFNM.html#EndPath
http://mejor.pl/exim-4.77/report-thpTB0.html#EndPath
http://mejor.pl/exim-4.77/report-kWMaKG.html#EndPath
I wonder if it could lead to any problem like it was in src/dcc.c ?
Thanks again,
Marcin

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


pdp at exim

May 18, 2012, 3:20 PM

Post #9 of 11 (792 views)
Permalink
Re: DCC header corruption [In reply to]

On 2012-05-18 at 22:26 +0200, Marcin Mirosław wrote:
> Thanks for answer. May i ask for another review?
> This reports are commented in the same way as report about dcc: "Stack
> address stored into global variable"
> http://mejor.pl/exim-4.77/report-IORFNM.html#EndPath

This is $auth1, which is *checked* where it occurs in server_condition.
That check is done while this function is in scope, in the OK branch.
So the var is left dangling in the event that auth fails, but nothing
will ever refer to it, so it's not a bug.

If there's another authentication attempt, the variables will be reset
first, so this can never be referred to in its current form.

It's still a little cleaner to not leave a global variable pointing to
invalid memory, even if it won't be referenced, so I've cleaned that up.

> http://mejor.pl/exim-4.77/report-thpTB0.html#EndPath

iplookup, only used at Cambridge, its use discouraged. Which would be
why problems have not arisen elsewhere; it's a Router, so something
referring to the expansion variables in a later Router is a mistake, so
people aren't writing such configs and we're not being bitten.

Looks like it's worse, as all the expansion variables will be pointing
to it.

So it requires a broken config to reference these, but it's still bad
for us to make it too easy for a broken config to cause a crash, so I've
fixed this too.

> http://mejor.pl/exim-4.77/report-kWMaKG.html#EndPath

Not a problem. This is a function only called by -bmalware, so Exim
exits after calling it, and the variable can never be referred to.

If these are the only complaints though, it's worth silencing the
message by resetting message_id to NULL afterwards.


So: one absolutely not a problem, one almost certainly not a problem,
and one which is a problem but requires a strongly-discouraged router
designed for one specific environment, *and* a broken config, to
trigger.

Will push in a moment.

Thanks for the reports,
-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


marcin at mejor

May 21, 2012, 2:15 AM

Post #10 of 11 (766 views)
Permalink
Re: DCC header corruption [In reply to]

W dniu 19.05.2012 00:20, Phil Pennock pisze:
> On 2012-05-18 at 22:26 +0200, Marcin Mirosław wrote:
Hello!

>> Thanks for answer. May i ask for another review?
>> This reports are commented in the same way as report about dcc: "Stack
>> address stored into global variable"
>> http://mejor.pl/exim-4.77/report-IORFNM.html#EndPath
>
> This is $auth1, which is *checked* where it occurs in server_condition.
> That check is done while this function is in scope, in the OK branch.
> So the var is left dangling in the event that auth fails, but nothing
> will ever refer to it, so it's not a bug.
>
> If there's another authentication attempt, the variables will be reset
> first, so this can never be referred to in its current form.
>
> It's still a little cleaner to not leave a global variable pointing to
> invalid memory, even if it won't be referenced, so I've cleaned that up.
>
>> http://mejor.pl/exim-4.77/report-thpTB0.html#EndPath
>
> iplookup, only used at Cambridge, its use discouraged. Which would be
> why problems have not arisen elsewhere; it's a Router, so something
> referring to the expansion variables in a later Router is a mistake, so
> people aren't writing such configs and we're not being bitten.
>
> Looks like it's worse, as all the expansion variables will be pointing
> to it.
>
> So it requires a broken config to reference these, but it's still bad
> for us to make it too easy for a broken config to cause a crash, so I've
> fixed this too.
>
>> http://mejor.pl/exim-4.77/report-kWMaKG.html#EndPath
>
> Not a problem. This is a function only called by -bmalware, so Exim
> exits after calling it, and the variable can never be referred to.
>
> If these are the only complaints though, it's worth silencing the
> message by resetting message_id to NULL afterwards.
>
>
> So: one absolutely not a problem, one almost certainly not a problem,
> and one which is a problem but requires a strongly-discouraged router
> designed for one specific environment, *and* a broken config, to
> trigger.
>
> Will push in a moment.

Thanks for all fixes and explanaitions. It's good news it was small
chance to hit problem.
I don't know if you do clang analyse from time to time so i'd like to
ask is it worth me to do clang analyse before/after each release or is
it too rarely usefull?
Marcin



--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##


pdp at exim

May 21, 2012, 2:41 AM

Post #11 of 11 (763 views)
Permalink
Re: DCC header corruption [In reply to]

On 2012-05-21 at 11:15 +0200, Marcin Mirosław wrote:
> I don't know if you do clang analyse from time to time so i'd like to
> ask is it worth me to do clang analyse before/after each release or is
> it too rarely usefull?

It's always worthwhile to know of potential problems, and I'm happy to
see people contribute to Exim in all sorts of ways. We've cleaned up
those issues, so anything new which appears will be a regression and
worth fixing.

I happen to have time now for some Exim work. I really hope I'll still
have time next week. In general, we never know who will have time, it's
all volunteer based, so anything you provide is a welcome distribution
of work. :)

Thank you for the analyses, they were useful.
-Phil

--
## List details at https://lists.exim.org/mailman/listinfo/exim-dev Exim details at http://www.exim.org/ ##

exim dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.