Re: [exim-dev] New malware scanner type "sock"

Página Inicial
Delete this message
Reply to this message
Autor: Phil Pennock
Data:  
Para: Jeremy Harris
CC: exim-dev
Assunto: Re: [exim-dev] New malware scanner type "sock"
On 2014-02-02 at 20:44 +0000, Jeremy Harris wrote:
> I've attached it as a patch. Does anyone feel strongly that it should
> not go into the exim sourcebase?


No, but some fixes would be good before it goes in.

It looks sockline_scanner might cause Badness if specified with
arbitrary %-expandos; Exim should be robust to such things, since with
FIXED_NEVER_USERS containing root, a compromise of CONFIGURE_OWNER
should not trivially lead to execution of arbitrary code as root.

> +        in = *(struct in_addr *) he->h_addr_list[0];
> +
> +        /* Open the Socket */
> +        if ( (sock = ip_socket(SOCK_STREAM, AF_INET)) < 0) {
> +          log_write(0, LOG_MAIN|LOG_PANIC,
> +                    "malware acl condition: sock: unable to acquire socket (%s)",
> +                    strerror(errno));
> +          return DEFER;
> +        }


You haven't checked he->h_addrtype and have just assumed AF_INET, so
this will be problematic given AF_INET6 results.

Similarly for later assumptions that inet_ntoa() is correct.

(There are functions in ip.c which might be useful in your refactoring
of malware.c).

> +        server.sun_family = AF_UNIX;
> +        Ustrcpy(server.sun_path, sock_options);


Buffer overflow. There are length limits on .sun_path (and quite short
ones too). 104 bytes, don't know of a portable constant #define name
for that.

> +      /*
> +        We're done sending, close socket for writing.
> +      */
> +
> +      /* shutdown(sock, SHUT_WR); */


?

> +      /* Read the result */
> +      memset(av_buffer, 0, sizeof(av_buffer));
> +      bread = read(sock, av_buffer, sizeof(av_buffer));
> +      (void)close(sock);
> +
> +      if (!(bread  > 0)) {
> +        log_write(0, LOG_MAIN|LOG_PANIC,
> +                  "malware acl condition: sock: unable to read from socket (%s)",
> +                  strerror(errno));
> +        return DEFER;
> +      }


An EOF caused by remote shutdown would not be cleanly reported here,
since errno will have been stomped on by close().

-Phil