[PATCH 2/7] Samba-VirusFilter: common headers and sources. version 3
Trever L. Adams
trever at middleearth.sapphiresunday.org
Tue Nov 1 21:51:56 UTC 2016
On 11/01/2016 01:21 PM, Jeremy Allison wrote:
> 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.
Thank you for your patience.
>
>> 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.
I will take care of this.
>
>> +#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.
Done.
>
>> +/* ====================================================================== */
>> +
>> +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.
I will take care of this as soon as I know things are working.
I have already taken care of the tabs/spaces.
>
> 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) ?
This is a good question. I am not even sure it works with utf-8, but it
might. I also have no answer on what is or isn't used. Part of the
problem will also be that Sophos, or another backend, may use a
different encoding. I am not sure where to go on this.
>
>> +/*
>> + * 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 ?
No. I am doing what I can to keep this stackable. Do you have any
suggestions on rewriting this or of a samba internal move that works
across file systems?
>
>> + 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 ?
I am in the process of trying to fix the writev code and this. Write now
I am struggling to figure out how to get wscript to build with
samba-socket as a declared public dependency.
I also realize I need to move my code out of sys_rw_data.[ch] as the
changes make it appropriate to be there. Would the Samba team prefer
this is abstracted out for others to use, or shall I merge it back into
vfs_virusfilter.utils.[ch]?
>
>> + 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 ?
This is what I believe was behind that design. That and allowing
flexibility in the scripts that are called with this code.
>
> Can we think more about this ?
Sure. You have given me much to work on.
>
>> 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.
Are you specifically meaning the str... macros or the header definition
macro? I believe I can also remove the
VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC stuff as that was for use during
the merge process.
>
>> + *
>> + * 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 :-).
Does this apply to the module name or just the alloc check?
>> + /* Size limit */
>> + ssize_t max_file_size;
>> + ssize_t min_file_size;
> Should the above be signed ?
I will look into that.
>
>> +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) ?
I do not know if it has been tested. I have never worked with streams.
If anyone has a test case of such a file that I can use in testing, I
will see what I can do to fix things up to handle streams. Of course,
changing the real virus for the test virus would be appreciated.
>
>> + 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
Thank you. I have done the conversion and will do the cleanups on the
messages as I work on your other change requests.
I believe I mostly have the tevent/tsocket code done, but until I can
figure out how to get wscript to not complain, I won't be able to test.
Thank you,
Trever
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 872 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161101/4cae3a6e/signature.sig>
More information about the samba-technical
mailing list