Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLAM-1531: Fix clamdscan directory scan on Windows #230

Merged

Conversation

kang-grace
Copy link
Contributor

Cause: _wopen on Windows doesn't work on directories and gives a Permission Denied error.
Old approach used _wopen to get a file descriptor and gets the
realpath from that. New approach redefines realpath() to _fullpath
on Windows, removing the use of _wopen in clamdscan on directories.

_fullpath: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fullpath-wfullpath?view=msvc-160

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the error, but unfortunately _fullpath() only converts relative paths to absolute paths and it doesn't evaluate symlinks.

I spent some time on this today trying to figure out a different solution and found that we could instead replace safe_open() with a call to CreateFileA(), and then convert the handle to a file descriptor before passing it to cli_get_filepath_from_filedesc(). Like this:

    hFile = CreateFileA(file_name,                  // file to open
                        GENERIC_READ,               // open for reading
                        FILE_SHARE_READ,            // share for reading
                        NULL,                       // default security
                        OPEN_EXISTING,              // existing file only
                        FILE_FLAG_BACKUP_SEMANTICS, // may be a directory
                        NULL);                      // no attr. template
    if (hFile == INVALID_HANDLE_VALUE) {
        cli_warnmsg("Can't open file %s: %d\n", file_name, GetLastError());
        status = CL_EOPEN;
        goto done;
    }

    if (-1 == (desc = _open_osfhandle((intptr_t)hFile, O_RDONLY))) {
        cli_warnmsg("Can't convert handle to file descriptor.\n");
        status = CL_EOPEN;
        goto done;
    }
    hFile = INVALID_HANDLE_VALUE; // _open_osfhandle() gives up ownership of the handle to desc.

    status = cli_get_filepath_from_filedesc(desc, &real_file_path);

This works pretty well, though it will still print errors on some devices like RAM disks (see the commit message 3a7a20c for details). That's not so bad though. It's a pretty uncommon situation.

But it's a little bit silly because cli_get_filepath_from_filedesc() will simply convert the file descriptor back to a handle.

I think ideally we could refactor cli_get_filepath_from_filedesc() into two functions, adding one called cli_get_filepath_from_handle(). We could call that one directly from cli_realpath() and then not have to do the switch to an fd & then back to a handle.

@kang-grace would you up for giving that a try?

by adding cli_get_filepath_from_handle
@kang-grace kang-grace force-pushed the CLAM-1531-win32-clamdscan-directory branch from e5a9a5f to 9c9d426 Compare August 5, 2021 15:05
@@ -1458,13 +1483,21 @@ cl_error_t cli_realpath(const char *file_name, char **real_filename)

#else

if ((desc = safe_open(file_name, O_RDONLY | O_BINARY)) == -1) {
cli_warnmsg("Can't open file %s: %s\n", file_name, strerror(errno));
HANDLE hFile;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please declare this at the top of the function, replacing desc and initialize it to INVALID_HANDLE_VALUE. Then we'll need to clean it up with CloseHandle() at the end after done: instead of desc, checking first if (INVALID_HANDLE_VALUE != hFile) {...}.

@@ -1290,6 +1290,81 @@ int cli_regcomp(regex_t *preg, const char *pattern, int cflags)
return cli_regcomp_real(preg, pattern, cflags);
}

#ifdef _WIN32
cl_error_t cli_get_filepath_from_handle(HANDLE hFile, char **filepath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a prototype to others.h so it can be used elsewhere in libclamav.

fix declaration and handle cleanup as well as prototype in others.h
@micahsnyder
Copy link
Contributor

Perfect! Also, it's working really nicely.

@micahsnyder micahsnyder merged commit e68b14e into Cisco-Talos:main Aug 9, 2021
22 of 24 checks passed
@micahsnyder
Copy link
Contributor

I also cherry-picked the fix into the rel/0.104 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants