Am 29.04.2013 09:28, schrieb Tomas Pospisek:
> To be specific:
> 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.
Well, we specifically have made sure not to read more data than would
fit buf, on 1436 above. Very bad things happen if inbufp is really
invalid because the length calculation (sizeof...) would be cast to
unsigned and overflow.
However, this part only applies when truncating what we have got (to
strip off the closing parenthesis from IMAP's ({123}...123 octets...)
part, to mention the use case.
I think us safe, because we only get here if (a) the protocol is
undelimited (thus can handle unterminated lines), (b) the length of data
we want to process is shorter than what we got. Because of line 1436,
we still know that we have not overrun the buffer here.
> 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?
UnMimeBodyline can, however, promise to never extend data, it is
supposed to only shorten it, or at the maximum pass data verbatim.
Base64 decoding causes a shortening of 25% (discounting EOL sequences);
quoted-printable of 0 up to 2/3rds depending on the amount of encoded
data. So inbufp cannot grow.
We rely on UnMimeBodyLine() not growing the buffer.
> 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.
In between, either we read new stuff from the network, or we fall
through from 1511 + epilog-of-block into 1524, where forward and
issoftline haven't changed, so these blocks are mutually exclusive.
Lines 1524-1533 are actually crucial to the fix and send out the
unterminated last line.
Auditing the unmime.c and transact.c code, however, reveals that base64
decoding is not taking place for bodies, and not for other multipart/*
than multipart/mixed, limiting the usefulness of the unmime feature.
I have received a certain percentage of messages with base64-encoded
text/plain bodies over the past years -- and the fact that people have
not complained about that speaks for itself.
I am inclined to kill the MIME decoding feature with the next major
fetchmail release, but we still need to plug the leaking bucket.
If I have overlooked anything important, do speak up.
Thank you.
Best regards
Matthias
--
To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
