Hi Wietse,
[ just subscribed to the list, I realized that our past conversation
was dropped since I was not subscribed, never mind ]
On Thu, Jun 07, 2012 at 08:16:53PM -0400, Wietse Venema wrote:
> Willy Tarreau:
> > > >Regardless of command format details, if the proxy prepends a command
> > > >to the client's SMTP stream, then postscreen must use unbuffered
> > > >I/O to read that command. If buffering were turned on, the buffering
> > > >layer could read past the proxy's<CR><LF> and eat up part of the
> > > >client input kind-of like CVE-2011-0411.
> >
> > Precisely on this point there is an easier way, it consists in using
> > recv(MSG_PEEK). The big advantage is that you don't need to store the
> > temporary bytes you've read since they remain in the kernel's buffers.
> > So it more or less looks like this :
> >
> > len = recv(fd, trash, sizeof(trash), MSG_PEEK);
> > if (len == -1 && errno == EAGAIN)
> > return;
> >
> > lf = memchr(trash, '\n', len);
> > if (lf == NULL) {
> > if (len < trash) /* Huh?? */
> > return;
> > /* else abort the connection */
> > }
>
> In an event-driven program such as postscreen, this code breaks
> when the proxy line arrives as multiple fragments.
>
> If the program does not drain proxy line fragments from the kernel
> buffer, then the socket remains readable and the program will go
> into a read-notification loop until the entire line is received.
Indeed this is totally true. I would say that given the short size
of the message the risk of this occurring is barely 0 explaining
whit this has probably never hit anybody, but it would be sufficient
that someone implements the protocol using a series of
write(fd, buf++, 1) and you'd be spinning (even worse if it dies in
the middle).
I've seen implementations doing recv(MSG_PEEK) and reject connections
from incomplete messages (again, almost zero risk but YMMV).
In haproxy, I'm using the input buffer associated to the fd, so I
don't have this problem. I'm basically doing a recv() on the buffer.
I think it is equivalent to the VSTREAM you're using.
In postscreen you don't want to do this since you don't want to
consume any possible incoming data (btw you probably drop the
connection if you get any data at this point). That said, if you
have a buffer associated to the connection, then you can perform
the first MSG_PEEK to check the pending data size and then a real
recv() to only consume up to the end of line.
But then doing so indeed invalidates the following suggestion.
> This implies that the following suggestion is not valid for an
> event-driven program such as postscreen:
>
> > On the one hand, if it is as trivial to make smtpd parse the PROXY
> > line as it was for postscreen, it can solve the problem by having
> > postscreen not consume the first line, which makes sense in that
> > postscreen remains the first layer analyser which doesn't mangle
> > data on the connection.
> Either you need to update the protocol spec (require non-fragmented
> proxy lines)
I have mixed opinions on this. On the one hand, we can't really impose
lower layers segmentation behaviour, so from a layering perspective, it
is not correct. On the other hand, the use cases for the protocol are
very specific. We're the very first segment over the connection so we
are always allowed to send at least one MSS. Nobody should sanely use
this proxy line on connections with an MSS lower than the 116 bytes a
max line may be for long IPv6 addresses and ports.
So indeed, I'm tempted to follow your suggestion, it will ease processing
for everyone and ensure that nobody tries sending fragmented lines. We'd
rely on a sane lower layer and declare other cases out of scope.
> or provide a code example that doesn't go into a
> read-notification loop when the proxy line arrives as multiple
> fragments.
With a buffer this problem does not happen, but it's the first case I'm
facing this need with fd passing, which makes me scratch my head a lot.
I really like the way you're plugging postscreen in front of smtpd, and
I'd like to ensure we don't make it complex to keep this nice model.
That's why I think that adding a sane requirement in the spec should be
the most adequate solution. If in the mean time you get a smarter idea,
do not hesitate to share it :-)
I'll keep thinking about it a bit before updating it. I think I will also
propose some generic code in the spec for both sides.
Best regards,
Willy
