Loading ...
Sorry, an error occurred while loading the content.

286294Re: Make smtpd/Postscreen compatible with load balancers

Expand Messages
  • Willy Tarreau
    Jun 8 12:27 AM
    • 0 Attachment
      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
    • Show all 17 messages in this topic