[PATCH 2/7] Samba-VirusFilter: common headers and sources. version 3
Jeremy Allison
jra at samba.org
Tue Nov 1 19:21:24 UTC 2016
On Fri, Oct 21, 2016 at 11:58:05AM -0600, Trever L. Adams wrote:
OK - more requests inline (sorry).
We're getting there (slowly... :-).
Jeremy.
> From 9ae341612d59839d3833da4376d4015ac909606e Mon Sep 17 00:00:00 2001
> From: "Trever L. Adams" <trever.adams at gmail.com>
> Date: Tue, 18 Oct 2016 13:34:53 -0600
> Subject: [PATCH 2/7] Samba-VirusFilter: common headers and sources.
>
> +#define conn_session_info(conn) ((conn)->session_info)
> +#define conn_socket(conn) ((conn)->transport.sock)
> +#define conn_domain_name(conn) ((conn)->session_info->info->domain_name)
> +#define conn_client_name(conn) ((conn)->sconn->remote_hostname)
> +#define conn_client_addr(conn) \
> + tsocket_address_inet_addr_string((conn)->sconn->remote_address, \
> + talloc_tos())
> +
> +#define conn_server_addr(conn) \
> + tsocket_address_inet_addr_string((conn)->sconn->local_address, \
> + talloc_tos())
> +
Please remove the above macros. Just use the structure
elements directly. We don't really use macros like this
in new code anymore.
> +#ifndef HAVE_MEMMEM
> +void *memmem(const void *m1, size_t m1_len, const void *m2, size_t m2_len)
> +{
> + const char *m1_cur = (const char *)m1;
> + const char *m1_end = (const char *)m1 + m1_len - m2_len;
> + const char *m2_cur = (const char *)m2;
> +
> + if (m1_len == 0 || m2_len == 0 || m1_len < m2_len) {
> + return NULL;
> + }
> +
> + if (m2_len == 1) {
> + return memchr(m1_cur, *m2_cur, m1_len);
> + }
> +
> + while (m1_cur <= m1_end) {
> + if (*m1_cur == *m2_cur) {
> + if (memcmp(m1_cur+1, m2_cur+1, m2_len-1) == 0) {
> + return (void *)m1_cur;
> + }
> + }
> + m1_cur++;
> + }
> +
> + return NULL;
> +}
> +#endif
Please remove the above. We already have a rep_memmem() in lib/replace/replace.c,
so you can just use it directly.
> +/* ====================================================================== */
> +
> +char *virusfilter_string_sub(
> + TALLOC_CTX *mem_ctx,
> + connection_struct *conn,
> + const char *str)
> +{
> + return talloc_sub_advanced(mem_ctx,
> + lp_servicename(mem_ctx, SNUM(conn)),
> + conn_session_info(conn)->unix_info->unix_name,
> + conn->connectpath,
> + conn_session_info(conn)->unix_token->gid,
> + conn_session_info(conn)->unix_info->sanitized_username,
> + conn_domain_name(conn),
> + str);
> +}
> +
> +/* Python's urllib.quote(string[, safe]) clone */
> +int virusfilter_url_quote(const char *src, char *dst, int dst_size)
> +{
> + char *dst_c = dst;
> + static char hex[] = "0123456789ABCDEF";
> +
> + for (; *src != '\0'; src++) {
> + if ((*src < '0' && *src != '-' && *src != '.' && *src != '/') ||
> + (*src > '9' && *src < 'A') ||
> + (*src > 'Z' && *src < 'a' && *src != '_') ||
> + (*src > 'z'))
> + {
> + if (dst_size < 4) {
> + return -1;
> + }
> + *dst_c++ = '%';
> + *dst_c++ = hex[(*src >> 4) & 0x0F];
> + *dst_c++ = hex[*src & 0x0F];
> + dst_size -= 3;
> + } else {
> + if (dst_size < 2) {
> + return -1;
> + }
> + *dst_c++ = *src;
> + dst_size--;
> + }
> + }
> +
> + *dst_c = '\0';
> +
> + return (dst_c - dst);
> +}
virusfilter_url_quote() seems to be used only in the sophos
module. I think it should be moved there and out of the common
code. Also please fix the spacing/tabs inside there.
Something to think about - the above is passed pathname
strings. This will only work if the backend is utf8 or ascii
compatible - yes ? What if the backend is one of the asian
sjis or others ? (Do we even use those anymore) ?
> +/*
> + * For rename across filesystems initial Patch from Warren Birnbaum
> + * <warrenb at hpcvscdp.cv.hp.com>
> + */
> +
> +static int virusfilter_copy_reg(const char *source, const char *dest)
> +{
> + SMB_STRUCT_STAT source_stats;
> + int saved_errno;
> + int ifd = -1;
> + int ofd = -1;
> +
> + if (sys_lstat(source, &source_stats, false) == -1) {
> + return -1;
> + }
> +
> + if (!S_ISREG(source_stats.st_ex_mode)) {
> + return -1;
> + }
> +
> + ifd = open(source, O_RDONLY, 0);
> + if (ifd < 0) {
> + return -1;
> + }
> +
> + if (unlink(dest) && errno != ENOENT) {
> + return -1;
> + }
> +
> +#ifdef O_NOFOLLOW
> + ofd = open(dest, O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW, 0600);
> +
> +#else
> + ofd = open(dest, O_WRONLY | O_CREAT | O_TRUNC , 0600);
> +#endif
> + if (ofd < 0) {
> + goto err;
> + }
> +
> + if (transfer_file(ifd, ofd, (size_t)-1) == -1) {
> + goto err;
> + }
> +
> + /*
> + * Try to preserve ownership. For non-root it might fail, but that's ok.
> + * But root probably wants to know, e.g. if NFS disallows it.
> + */
> +
> +#ifdef HAVE_FCHOWN
> + if ((fchown(ofd, source_stats.st_ex_uid, source_stats.st_ex_gid) == -1)
> + && (errno != EPERM))
> + {
> +#else
> + if ((chown(dest, source_stats.st_ex_uid, source_stats.st_ex_gid) == -1)
> + && (errno != EPERM))
> + {
> +#endif
> + goto err;
> + }
> +
> + /*
> + * fchown turns off set[ug]id bits for non-root,
> + * so do the chmod last.
> + */
> +
> +#if defined(HAVE_FCHMOD)
> + if (fchmod(ofd, source_stats.st_ex_mode & 07777)) {
> +#else
> + if (chmod(dest, source_stats.st_ex_mode & 07777)) {
> +#endif
> + goto err;
> + }
> +
> + if (close(ifd) == -1) {
> + goto err;
> + }
> +
> + if (close(ofd) == -1) {
> + return -1;
> + }
> +
> + /* Try to copy the old file's modtime and access time. */
> +#if defined(HAVE_UTIMENSAT)
> + {
> + struct timespec ts[2];
> +
> + ts[0] = source_stats.st_ex_atime;
> + ts[1] = source_stats.st_ex_mtime;
> + utimensat(AT_FDCWD, dest, ts, AT_SYMLINK_NOFOLLOW);
> + }
> +#elif defined(HAVE_UTIMES)
> + {
> + struct timeval tv[2];
> +
> + tv[0] = convert_timespec_to_timeval(source_stats.st_ex_atime);
> + tv[1] = convert_timespec_to_timeval(source_stats.st_ex_mtime);
> +#ifdef HAVE_LUTIMES
> + lutimes(dest, tv);
> +#else
> + utimes(dest, tv);
> +#endif
> + }
> +#elif defined(HAVE_UTIME)
> + {
> + struct utimbuf tv;
> +
> + tv.actime = convert_timespec_to_time_t(
> + source_stats.st_ex_atime);
> + tv.modtime = convert_timespec_to_time_t(
> + source_stats.st_ex_mtime);
> + utime(dest, &tv);
> + }
> +#endif
> +
> + if (unlink(source) == -1) {
> + return -1;
> + }
> +
> + return 0;
> +
> + err:
> +
> + saved_errno = errno;
> + if (ifd != -1) {
> + close(ifd);
> + }
> + if (ofd != -1) {
> + close(ofd);
> + }
> + errno = saved_errno;
> + return -1;
> +}
All of the functions above are direct calls into POSIX.
This makes the module explicitly not-stackable. Is this
what you intended ?
> + iov_p->iov_len = va_arg(ap, int);
> + }
> + va_end(ap);
> +
> + iov_p->iov_base = io_h->r_eol;
> + iov_p->iov_len = io_h->r_eol_size;
> + iov_n++;
> +
> + switch (write_data_iov_timeout(io_h->socket, iov, iov_n,
> + io_h->io_timeout, 1))
> + {
> + case -1:
> + return VIRUSFILTER_RESULT_ERROR;
> + default:
> + return VIRUSFILTER_RESULT_OK;
> + }
> +}
> +
> +virusfilter_result virusfilter_io_readl(virusfilter_io_handle *io_h)
> +{
> + char *buffer;
> + ssize_t buffer_size = VIRUSFILTER_IO_BUFFER_SIZE;
> + struct pollfd pollfd;
> + ssize_t read_size;
> + char *eol;
> +
> + if (io_h->r_rest_buffer == NULL) {
> + DEBUG(11, ("Rest data not found in read buffer\n"));
> + buffer = io_h->r_buffer = io_h->r_buffer_real;
> + buffer_size = VIRUSFILTER_IO_BUFFER_SIZE;
> + } else {
> + DEBUG(11, ("Rest data found in read buffer: %s, size=%ld\n",
> + io_h->r_rest_buffer, (long)io_h->r_rest_size));
> + eol = memmem(io_h->r_rest_buffer, io_h->r_rest_size,
> + io_h->r_eol, io_h->r_eol_size);
> + if (eol) {
> + *eol = '\0';
> + io_h->r_buffer = io_h->r_rest_buffer;
> + io_h->r_size = eol - io_h->r_rest_buffer;
> + DEBUG(11, ("Read line data from read buffer: %s\n",
> + io_h->r_buffer));
> +
> + io_h->r_rest_size -= io_h->r_size + io_h->r_eol_size;
> + io_h->r_rest_buffer = (io_h->r_rest_size > 0) ?
> + (eol + io_h->r_eol_size) : NULL;
> +
> + return VIRUSFILTER_RESULT_OK;
> + }
> +
> + io_h->r_buffer = io_h->r_buffer_real;
> + memmove(io_h->r_buffer, io_h->r_rest_buffer,
> + io_h->r_rest_size);
> +
> + buffer = io_h->r_buffer + io_h->r_size;
> + buffer_size = VIRUSFILTER_IO_BUFFER_SIZE - io_h->r_rest_size;
> + }
> +
> + io_h->r_rest_buffer = NULL;
> + io_h->r_rest_size = 0;
> +
> + pollfd.fd = io_h->socket;
> + pollfd.events = POLLIN;
> +
> + while (buffer_size > 0) {
> + switch (poll(&pollfd, 1, io_h->io_timeout)) {
> + case -1:
> + if (errno == EINTR) {
> + errno = 0;
> + continue;
> + }
> + return VIRUSFILTER_RESULT_ERROR;
> + case 0:
> + errno = ETIMEDOUT;
> + return VIRUSFILTER_RESULT_ERROR;
> + }
> +
> + read_size = read(io_h->socket, buffer, buffer_size);
> + if (read_size == -1) {
> + if (errno == EINTR) {
> + errno = 0;
> + continue;
> + }
> + return VIRUSFILTER_RESULT_ERROR;
> + }
This suffers from the same problems as the writev code
I discussed in previous emails.
If we're changing that to be a tstream can we fix this
up as well to use a tevent context with callback functions ?
> + buffer[read_size] = '\0';
> +
> + if (read_size == 0) { /* EOF */
> + return VIRUSFILTER_RESULT_OK;
> + }
> +
> + io_h->r_size += read_size;
> +
> + eol = memmem(io_h->r_buffer, read_size, io_h->r_eol,
> + io_h->r_eol_size);
> + if (eol) {
> + *eol = '\0';
> + DEBUG(11, ("Read line data from socket: %s\n",
> + io_h->r_buffer));
> + io_h->r_size = eol - io_h->r_buffer;
> + io_h->r_rest_size = read_size - (eol - buffer +
> + io_h->r_eol_size);
> + if (io_h->r_rest_size > 0) {
> + io_h->r_rest_buffer = eol + io_h->r_eol_size;
> + DEBUG(11, ("Rest data in read buffer: %s, "
> + "size=%ld\n", io_h->r_rest_buffer,
> + (long)io_h->r_rest_size));
> + }
> + return VIRUSFILTER_RESULT_OK;
> + }
> +
> + buffer += read_size;
> + buffer_size -= read_size;
> + }
> +
> + errno = E2BIG;
> +
> + return VIRUSFILTER_RESULT_ERROR;
> +}
> + if (strncmp("::ffff:", server_addr_p, 7) == 0) {
> + server_addr_p += 7;
> + }
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_SERVER_IP",
> + server_addr_p);
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_SERVER_NAME",
> + myhostname());
> + virusfilter_env_set(mem_ctx, env_list,
> + "VIRUSFILTER_SERVER_NETBIOS_NAME",
> + local_machine_name);
> + slprintf(pidstr,sizeof(pidstr)-1, "%ld", (long)getpid());
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_SERVER_PID",
> + pidstr);
> +
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_SERVICE_NAME",
> + lp_const_servicename(snum));
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_SERVICE_PATH",
> + conn->connectpath);
> +
> + client_addr_p = conn_client_addr(conn);
> + if (strncmp("::ffff:", client_addr_p, 7) == 0) {
> + client_addr_p += 7;
> + }
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_CLIENT_IP",
> + client_addr_p);
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_CLIENT_NAME",
> + conn_client_name(conn));
> + virusfilter_env_set(mem_ctx, env_list,
> + "VIRUSFILTER_CLIENT_NETBIOS_NAME",
> + get_remote_machine_name());
> +
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_USER_NAME",
> + get_current_username());
> + virusfilter_env_set(mem_ctx, env_list, "VIRUSFILTER_USER_DOMAIN",
> + current_user_info.domain);
> +
> + return 0;
> +}
> +
> +/* Wrapper to Samba's smbrun() in smbrun.c */
> +int virusfilter_shell_run(
> + TALLOC_CTX *mem_ctx,
> + const char *cmd,
> + char **env_list,
> + connection_struct *conn,
> + bool sanitize)
> +{
> + if (conn != NULL && virusfilter_shell_set_conn_env(mem_ctx, env_list,
> + conn) == -1) {
> + return -1;
> + }
> +
> + if (sanitize) {
> + return smbrun(cmd, NULL, strv_to_env(talloc_tos(), *env_list));
> + } else {
> + return smbrun_no_sanitize(cmd, NULL, strv_to_env(talloc_tos(),
> + *env_list));
> + }
> +}
Urgggh. I *hate* passing these parameters as environment variables.
I guess it's more dangerous to pass them as command line parameters
though, due to shell expansions ?
Can we think more about this ?
> diff --git a/source3/modules/vfs_virusfilter_utils.h b/source3/modules/vfs_virusfilter_utils.h
> new file mode 100644
> index 0000000..22bf983
> --- /dev/null
> +++ b/source3/modules/vfs_virusfilter_utils.h
> @@ -0,0 +1,168 @@
> +/*
> + Samba-VirusFilter VFS modules
> + Copyright (C) 2010-2016 SATOH Fumiyasu @ OSS Technology Corp., Japan
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifndef _VIRUSFILTER_UTILS_H
> +#define _VIRUSFILTER_UTILS_H
> +
> +#include "modules/vfs_virusfilter_common.h"
> +#include "../lib/util/memcache.h"
> +#include "../lib/util/strv.h"
> +
> +#ifndef VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC
> +#define VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC SINGLETON_CACHE_TALLOC
> +#endif
> +
> +#define str_eq(s1, s2) \
> + ((strcmp((s1), (s2)) == 0) ? true : false)
> +#define strn_eq(s1, s2, n) \
> + ((strncmp((s1), (s2), (n)) == 0) ? true : false)
Please don't use macro's like the above. No one else
has a clue what they mean (although these are more obvious than
most :-). Please use the standard functions - or create functions
for this.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "modules/vfs_virusfilter_common.h"
> +#include "modules/vfs_virusfilter_utils.h"
> +
> +#define VIRUSFILTER_MODULE_NAME "virusfilter_" VIRUSFILTER_MODULE_ENGINE
> +#define ALLOC_CHECK(ptr, label) do { if ((ptr) == NULL) { DEBUG(0, \
> + ("virusfilter-vfs: out of memory!\n")); \
> + errno = ENOMEM; goto label; } } while(0)
No macros like the above please - we're trying to get rid of things
like this (yes I know it's copied from vfs_recycle - it's ugly there
too :-).
> + /* Size limit */
> + ssize_t max_file_size;
> + ssize_t min_file_size;
Should the above be signed ?
> +static int virusfilter_vfs_unlink(
> + vfs_handle_struct *vfs_h,
> + const struct smb_filename *smb_fname)
> +{
> + int ret = SMB_VFS_NEXT_UNLINK(vfs_h, smb_fname);
> + virusfilter_handle *virusfilter_h;
> + char *fname;
> +
> + if (ret != 0 && errno != ENOENT) {
> + return ret;
> + }
> +
> + SMB_VFS_HANDLE_GET_DATA(vfs_h, virusfilter_h, virusfilter_handle,
> + return -1);
> +
> + if (virusfilter_h->cache_h) {
> + fname = smb_fname->base_name;
> + DEBUG(10, ("Removing cache entry (if existent): fname: %s\n",
> + fname));
> + virusfilter_cache_remove(virusfilter_h->cache_h, fname);
> + }
> +
> + return ret;
> +}
How does the above handle streams ? Has this been tested
with viruses being written into streams ? (This is a very
popular way to hide virus code on Windows NTFS filesystems) ?
> + virusfilter_debug_level = debug_add_class(VIRUSFILTER_MODULE_NAME);
> + if (virusfilter_debug_level == -1) {
> + virusfilter_debug_level = DBGC_VFS;
> + DEBUG(0, ("Couldn't register custom debugging class!\n"));
> + } else {
> + DEBUG(10, ("Debug class number of '%s': %d\n",
> + VIRUSFILTER_MODULE_NAME, virusfilter_debug_level));
> + }
> +
> + DEBUG(5, ("%s registered\n", VIRUSFILTER_MODULE_NAME));
> +
> + return ret;
> +}
> --
> 2.9.3
>
Can we change all the DEBUG() calls to the new standard of
DBG_XXX() please ?
From README.Coding:
DEBUG statements
----------------
Use these following macros instead of DEBUG:
DBG_ERR log level 0 error conditions
DBG_WARNING log level 1 warning conditions
DBG_NOTICE log level 3 normal, but significant, condition
DBG_INFO log level 5 informational message
DBG_DEBUG log level 10 debug-level message
Example usage:
DBG_ERR("Memory allocation failed\n");
DBG_DEBUG("Received %d bytes\n", count);
The messages from these macros are automatically prefixed with the
function name.
More information about the samba-technical
mailing list