On Mon, 29 Apr 2013, Matthias Andree wrote:
Am 28.04.2013 09:07, schrieb Tomas Pospisek:
I have had a look at the patch and as an outsider to the fetchmail code
that is not a maintaner of fetchmail am personaly not comfortable with
it. The problem is that the patched code in question is allready twisted
enough, it's in very large part code to work around various buggy mail
software that f.ex. doesn't signal the content length correctly. So
dynamic content lengths and fixed buffers are passed around and written
into and added to. The provided patch now adds on top of all of this yet
another work around that adds a newline at the end of the buffer in
order to get the transformed mime encoded content right.
In short, in the time I was looking at the patch, I was not able to
determine, if it doesn't add a buffer overflow. To me as an ignorant
fetchmail code outsider the code in question does contain buffer
overflow code smell.
Would you care to share particular concerns with the code? If you can
share more concrete concerns, I may be able to answer them.
I am happy to answer questions or revise the patch if necessary.
From a point of view of code reading and understanding, i.e. code
analysis, tight coupling of code increases the difficulty to understand
the code significantly. Code that calls functions with side effects
can only be understodd if all called functions with sideeffects are
also understood.
To be specific:
1408: the 'readbody' function gets a socket, some more args and a length.
1416: char buf[MSGBUFSIZE+4];
fixed length 'buf'. This is the place where potentiall a buffer
overflow could occur.
1417: char *inbufp = buf
1430: while (protocol->delimited || len > 0)
we loop as long as len > 0.
1436: SockRead(sock, inbufp, sizeof(buf)-4-(inbufp-buf)))
This can not overflow.
1467: inbufp[len++] = '\r';
Can overflow if inbufp[len] outside of 'buf'. Can we prove that
inbufp[len] does not move out of 'buf'? inbufp is a pointer that
is being increased - possibly outside of buf.
1485: issoftline = UnMimeBodyline(&inbufp, protocol->delimited, issoftline);
This is the crux of the problem, i.e. the place where sideeffects
come into the picture. A referenc to inbufp is passed to
UnMimeBodyline. That function can in principle do whatever it
wants to the inbufp pointer. No 'remaining space in buf' argument
is passed to that function, thus the function can not
assure/enforce in bounds writing to 'buf'.
Can we prove that UnMimeBodyline allways stays within the bounds
of the remaining space in 'buf' and does not increase inbufp
beyond MSGBUFSIZE?
1529: inbufp[2] = '\0';
We're outside the loop. If we have proved at line 1485 that
UnMimeBodyline can never grow outside of MSGBUFSIZE then we are
safe. Otherwise we don't know.
There's one additional problem, which is not related to potential buffer
overflows:
1501: if (forward && (!issoftline))
{
...
1511: rb_send(ctl, buf, &n);
sends off the buffer
1524: if (forward && issoftline)
{
...
1533: return rb_send(ctl, buf, &n);
sends off the same buffer
Can this result in sending the same buffer twice? It all depends on
'issoftline' but that is a variable that is changing with line 1485.
This is what I've understood so far.
Greets,
*t
--
To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
