Skip to content

sftp: refuse dir REMOVE and replacing RENAME #1333

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.

Sign up for GitHub

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

Open
ejohnstown wants to merge 1 commit into
base: master
Choose a base branch
from
+255 −7
Open

sftp: refuse dir REMOVE and replacing RENAME #1333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ AC_LINK_IFELSE(
AC_MSG_RESULT([yes])],
[AC_MSG_RESULT([no])])

AC_MSG_CHECKING([for linkat])
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([[#include <fcntl.h>
#include <unistd.h>]],
[[(void)linkat(AT_FDCWD, "x", AT_FDCWD, "y", 0);]])],
[AC_DEFINE([HAVE_LINKAT], [1], [linkat() available and declared])
AC_MSG_RESULT([yes])],
[AC_MSG_RESULT([no])])

AC_CHECK_DECLS([[pread],[pwrite]],,[unistd.h])

# DEBUG
Expand Down
79 changes: 73 additions & 6 deletions src/wolfsftp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4981,6 +4981,9 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
char name[WOLFSSH_MAX_FILENAME];
word32 idx = 0;
int ret = WS_SUCCESS;
#ifndef USE_WINDOWS_API
WS_SFTP_FILEATRB atr;
#endif

byte type = WOLFSSH_FTP_OK;
int rc;
Expand All @@ -5006,7 +5009,20 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)

if (ret == WS_SUCCESS) {
#ifndef USE_WINDOWS_API
if (WREMOVE(ssh->fs, name) < 0)
/* REMOVE never takes a directory (draft-ietf-secsh-filexfer-02 6.5) */
WMEMSET(&atr, 0, sizeof(atr));
if (SFTP_GetAttributes(ssh->fs, name, &atr, 1, ssh->ctx->heap)
== WS_SUCCESS
&& (atr.per & FILEATRB_PER_MASK_TYPE) == FILEATRB_PER_DIR) {
WLOG(WS_LOG_SFTP, "Not removing a directory");
ret = WS_BAD_FILE_E;
}
#ifdef WUNLINK
/* unlink() refuses a directory swapped in after the check */
else if (WUNLINK(ssh->fs, name) < 0)
#else
else if (WREMOVE(ssh->fs, name) < 0)
#endif
#else /* USE_WINDOWS_API */
if (WS_DeleteFileA(name, ssh->ctx->heap) == 0)
#endif /* USE_WINDOWS_API */
Expand Down Expand Up @@ -5050,6 +5066,52 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
}


#ifndef USE_WINDOWS_API
/* Rename without replacing an existing name (draft-ietf-secsh-filexfer-02
* 6.5). linkat() refuses one atomically; other types, builds without linkat(),
* and filesystems without hard links check first and race a concurrent
* create.
*
* returns WS_SUCCESS on success
*/
static int SFTP_RenameNoReplace(WOLFSSH* ssh, char* old, char* name)
{
WS_SFTP_FILEATRB atr;

#ifdef WLINK
WMEMSET(&atr, 0, sizeof(atr));
if (SFTP_GetAttributes(ssh->fs, old, &atr, 1, ssh->ctx->heap)
== WS_SUCCESS
&& (atr.per & FILEATRB_PER_MASK_TYPE) == FILEATRB_PER_FILE) {
if (WLINK(ssh->fs, old, name) == 0) {
if (WUNLINK(ssh->fs, old) == 0) {
return WS_SUCCESS;
}
/* undo the link */
(void)WUNLINK(ssh->fs, name);
return WS_BAD_FILE_E;
}
if (errno == EEXIST) {
WLOG(WS_LOG_SFTP, "Rename target exists");
return WS_BAD_FILE_E;
}
}
#endif

WMEMSET(&atr, 0, sizeof(atr));
if (SFTP_GetAttributes(ssh->fs, name, &atr, 1, ssh->ctx->heap)
== WS_SUCCESS) {
WLOG(WS_LOG_SFTP, "Rename target exists");
return WS_BAD_FILE_E;
}
if (WRENAME(ssh->fs, old, name) < 0) {
return WS_BAD_FILE_E;
}
return WS_SUCCESS;
}
#endif /* USE_WINDOWS_API */


/* Handles packet to rename a file
*
* returns WS_SUCCESS on success
Expand Down Expand Up @@ -5098,8 +5160,9 @@ int wolfSSH_SFTP_RecvRename(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)

if (ret == WS_SUCCESS) {
#ifndef USE_WINDOWS_API
if (WRENAME(ssh->fs, old, name) < 0)
if (SFTP_RenameNoReplace(ssh, old, name) != WS_SUCCESS)
#else /* USE_WINDOWS_API */
/* MoveFile never replaces */
if (WS_MoveFileA(old, name, ssh->ctx->heap) == 0)
#endif /* USE_WINDOWS_API */
{
Expand Down Expand Up @@ -5212,8 +5275,10 @@ static int SFTP_GetAttributes(void* fs, const char* fileName,
return WS_SUCCESS;
}

/* handle case of "/" */
if (sz < 3 && fileName[0] == WS_DELIM && ret == NUF_NOFILE) {
/* handle case of "/" or "/.", not a root-level name such as "/a" */
if (fileName[0] == WS_DELIM
&& (sz == 1 || (sz == 2 && fileName[1] == '.'))
&& ret == NUF_NOFILE) {
atr->flags |= WOLFSSH_FILEATRB_PERM;
atr->per |= 040000;
return WS_SUCCESS;
Expand Down Expand Up @@ -5597,8 +5662,10 @@ static int SFTP_GetAttributes(void* fs, const char* fileName,
}

/* handle case of "/" */
/* Calling f_stat for "/" returns FR_INVALID_NAME. So we simulate the result. */
if (sz < 3 && fileName[0] == WS_DELIM) {
/* Calling f_stat for "/" returns FR_INVALID_NAME. So we simulate the result.
* Only "/" and "/." though; "/a" is a root-level name for f_stat. */
if (fileName[0] == WS_DELIM
&& (sz == 1 || (sz == 2 && fileName[1] == '.'))) {
atr->flags |= WOLFSSH_FILEATRB_PERM;
atr->per |= 040000;
return WS_SUCCESS;
Expand Down
163 changes: 163 additions & 0 deletions tests/regress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16021,6 +16021,167 @@ static void TestSftpAppendWritesAtEof(void)
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
}

/* FXP_RENAME path to newPath, or FXP_REMOVE path when newPath is NULL. */
static int SftpPathRequest(WOLFSSH* ssh, int reqId, const char* path,
const char* newPath)
{
byte pkt[2 * (WOLFSSH_MAX_FILENAME + UINT32_SZ)];
word32 idx = 0;
word32 sz = (word32)WSTRLEN(path);

SftpPutU32(sz, pkt + idx); idx += UINT32_SZ;
WMEMCPY(pkt + idx, path, sz); idx += sz;
if (newPath == NULL) {
return wolfSSH_SFTP_RecvRemove(ssh, reqId, pkt, idx);
}
sz = (word32)WSTRLEN(newPath);
SftpPutU32(sz, pkt + idx); idx += UINT32_SZ;
WMEMCPY(pkt + idx, newPath, sz); idx += sz;
return wolfSSH_SFTP_RecvRename(ssh, reqId, pkt, idx);
}

static void SftpWriteTestFile(const char* path, const char* content)
{
WFILE* file;

AssertIntEQ(WFOPEN(NULL, &file, path, "wb"), 0);
AssertTrue(file != WBADFILE);
AssertIntEQ((int)WFWRITE(NULL, content, 1, WSTRLEN(content), file),
(int)WSTRLEN(content));
WFCLOSE(NULL, file);
}

/* REMOVE refuses a directory, RENAME an existing newpath
* (draft-ietf-secsh-filexfer-02 6.5). */
static void TestSftpRemoveRenameSemantics(void)
{
WOLFSSH_CTX* ctx;
WOLFSSH* ssh;
int rid = 700;
WSTAT_T st;
char cwd[WOLFSSH_MAX_FILENAME];
char fileA[64], fileB[64], fileC[64];
char dirA[64], dirB[64], dirC[64];
#if defined(WOLFSSH_HAVE_SYMLINK) && !defined(USE_WINDOWS_API)
char lnk[64];
#endif
#ifndef USE_WINDOWS_API
char roDir[64], roFile[128], roDest[128];
#endif

ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
AssertNotNull(ctx);
ssh = wolfSSH_new(ctx);
AssertNotNull(ssh);
AssertIntEQ(wolfSSH_SFTP_TestRecvStateInit(ssh), WS_SUCCESS);

WSNPRINTF(fileA, sizeof(fileA), "wolfssh_rm_a_%lu.tmp", SFTP_TEST_PID());
WSNPRINTF(fileB, sizeof(fileB), "wolfssh_rm_b_%lu.tmp", SFTP_TEST_PID());
WSNPRINTF(fileC, sizeof(fileC), "wolfssh_rm_c_%lu.tmp", SFTP_TEST_PID());
WSNPRINTF(dirA, sizeof(dirA), "wolfssh_rm_da_%lu", SFTP_TEST_PID());
WSNPRINTF(dirB, sizeof(dirB), "wolfssh_rm_db_%lu", SFTP_TEST_PID());
WSNPRINTF(dirC, sizeof(dirC), "wolfssh_rm_dc_%lu", SFTP_TEST_PID());
WMEMSET(cwd, 0, sizeof(cwd));
AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1));
AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS);

SftpWriteTestFile(fileA, "aaaa");
SftpWriteTestFile(fileB, "bb");
(void)WREMOVE(ssh->fs, fileC);
AssertIntEQ(WMKDIR(ssh->fs, dirA, 0755), 0);
AssertIntEQ(WMKDIR(ssh->fs, dirB, 0755), 0);
(void)WRMDIR(ssh->fs, dirC);

/* REMOVE refuses an empty directory */
AssertIntEQ(SftpPathRequest(ssh, rid, dirA, NULL), WS_BAD_FILE_E);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
AssertIntEQ(WSTAT(ssh->fs, dirA, &st), 0);

/* RENAME refuses an existing file; neither changes */
AssertIntEQ(SftpPathRequest(ssh, rid, fileA, fileB), WS_BAD_FILE_E);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
AssertSftpFileContents(fileA, "aaaa", 4);
AssertSftpFileContents(fileB, "bb", 2);

/* RENAME refuses an existing empty directory */
AssertIntEQ(SftpPathRequest(ssh, rid, dirA, dirB), WS_BAD_FILE_E);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
AssertIntEQ(WSTAT(ssh->fs, dirA, &st), 0);
AssertIntEQ(WSTAT(ssh->fs, dirB, &st), 0);

#if defined(WOLFSSH_HAVE_SYMLINK) && !defined(USE_WINDOWS_API)
/* a dangling symlink is an existing newpath too */
WSNPRINTF(lnk, sizeof(lnk), "wolfssh_rm_ln_%lu", SFTP_TEST_PID());
(void)WREMOVE(ssh->fs, lnk);
AssertIntEQ(symlink(fileC, lnk), 0);
AssertIntEQ(SftpPathRequest(ssh, rid, fileA, lnk), WS_BAD_FILE_E);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
AssertIntEQ(WLSTAT(ssh->fs, lnk, &st), 0);
AssertTrue(S_ISLNK(st.st_mode));
AssertSftpFileContents(fileA, "aaaa", 4);
/* REMOVE takes the link, not its target */
AssertIntEQ(SftpPathRequest(ssh, rid, lnk, NULL), WS_SUCCESS);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
AssertTrue(WLSTAT(ssh->fs, lnk, &st) != 0);
/* a link to a directory is not a directory to REMOVE */
AssertIntEQ(symlink(dirB, lnk), 0);
AssertIntEQ(SftpPathRequest(ssh, rid, lnk, NULL), WS_SUCCESS);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
AssertTrue(WLSTAT(ssh->fs, lnk, &st) != 0);
AssertIntEQ(WSTAT(ssh->fs, dirB, &st), 0);
#endif

#ifndef USE_WINDOWS_API
/* a failed move leaves both names as they were: out of a read-only
* directory takes the link rollback, into one the link fallback */
if (geteuid() != 0) {
WSNPRINTF(roDir, sizeof(roDir), "wolfssh_rm_ro_%lu",
SFTP_TEST_PID());
WSNPRINTF(roFile, sizeof(roFile), "%s/%s", roDir, fileC);
WSNPRINTF(roDest, sizeof(roDest), "%s/%s", roDir, fileB);
AssertIntEQ(WMKDIR(ssh->fs, roDir, 0755), 0);
SftpWriteTestFile(roFile, "cc");
AssertIntEQ(chmod(roDir, 0555), 0);

AssertIntEQ(SftpPathRequest(ssh, rid, roFile, fileC), WS_BAD_FILE_E);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
AssertSftpFileContents(roFile, "cc", 2);
AssertTrue(WLSTAT(ssh->fs, fileC, &st) != 0);

AssertIntEQ(SftpPathRequest(ssh, rid, fileB, roDest), WS_BAD_FILE_E);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_FAILURE);
AssertSftpFileContents(fileB, "bb", 2);
AssertTrue(WLSTAT(ssh->fs, roDest, &st) != 0);

AssertIntEQ(chmod(roDir, 0755), 0);
AssertIntEQ(WREMOVE(ssh->fs, roFile), 0);
AssertIntEQ(WRMDIR(ssh->fs, roDir), 0);
}
#endif

/* RENAME to a free name moves a file and a directory */
AssertIntEQ(SftpPathRequest(ssh, rid, fileA, fileC), WS_SUCCESS);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
AssertTrue(WSTAT(ssh->fs, fileA, &st) != 0);
AssertSftpFileContents(fileC, "aaaa", 4);
AssertIntEQ(SftpPathRequest(ssh, rid, dirA, dirC), WS_SUCCESS);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
AssertTrue(WSTAT(ssh->fs, dirA, &st) != 0);
AssertIntEQ(WSTAT(ssh->fs, dirC, &st), 0);

/* REMOVE still removes a file */
AssertIntEQ(SftpPathRequest(ssh, rid, fileC, NULL), WS_SUCCESS);
AssertSftpStatusReply(ssh, rid++, WOLFSSH_FTP_OK);
AssertTrue(WSTAT(ssh->fs, fileC, &st) != 0);

(void)WREMOVE(ssh->fs, fileB);
(void)WRMDIR(ssh->fs, dirB);
(void)WRMDIR(ssh->fs, dirC);
wolfSSH_SFTP_TestRecvStateFree(ssh);
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
}
#endif /* !NO_WOLFSSH_SERVER && !NO_FILESYSTEM */

#if !defined(NO_WOLFSSH_SERVER) && !defined(USE_WINDOWS_API) && \
Expand Down Expand Up @@ -20771,6 +20932,8 @@ int main(int argc, char** argv)
#if !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM)
/* FXF_APPEND writes land at EOF regardless of the request offset */
TestSftpAppendWritesAtEof();
/* REMOVE refuses directories, RENAME refuses an existing newpath */
TestSftpRemoveRenameSemantics();
#endif
#if !defined(NO_WOLFSSH_SERVER) && defined(USE_WINDOWS_API) && \
!defined(NO_FILESYSTEM)
Expand Down
11 changes: 10 additions & 1 deletion wolfssh/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,7 @@ extern "C" {

#else

#include <unistd.h> /* used for rmdir */
#include <unistd.h> /* used for rmdir, linkat, and unlink */
#include <sys/stat.h> /* used for mkdir, stat, and lstat */
#include <stdio.h> /* used for remove and rename */

Expand All @@ -1622,6 +1622,15 @@ extern "C" {
#define WSTAT(fs,p,b) stat((p),(b))
#ifndef USE_OSE_API
#define WLSTAT(fs,p,b) lstat((p),(b))
#define WUNLINK(fs,p) unlink((p))
#ifdef HAVE_LINKAT
/* SFTP no-replace rename; it reads errno. linkat() without
* AT_SYMLINK_FOLLOW links a symlink itself, where link() may
* follow it. */
#include <errno.h>
#include <fcntl.h>
#define WLINK(fs,o,n) linkat(AT_FDCWD, (o), AT_FDCWD, (n), 0)
#endif
#else
#define WLSTAT(fs,p,b) stat((p),(b))
#endif
Expand Down
Loading