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
