Skip to content

Commit fe2d686

Browse files
ejohnstownejohnstown
committed
SFTP List
More Updates from peer review comments.
1 parent b816ac5 commit fe2d686

File tree

4 files changed

+171
-86
lines changed

4 files changed

+171
-86
lines changed

‎src/ssh.c‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2317,13 +2317,17 @@ int wolfSSH_RealPath(const char* defaultPath, char* in,
23172317
WMEMSET(out, 0, outSz);
23182318
inSz = (word32)WSTRLEN(in);
23192319
out[0] = '/';
2320+
curSz = 1;
23202321
if (inSz == 0 || (!IS_DELIM(in[0]) && !IS_WINPATH(inSz, in))) {
23212322
if (defaultPath != NULL) {
2323+
curSz = (word32)WSTRLEN(defaultPath);
2324+
if (curSz >= outSz) {
2325+
return WS_INVALID_PATH_E;
2326+
}
23222327
WSTRNCPY(out, defaultPath, outSz);
23232328
}
23242329
}
2325-
out[outSz - 1] = 0;
2326-
curSz = (word32)WSTRLEN(out);
2330+
out[curSz] = 0;
23272331

23282332
for (seg = WSTRTOK(in, DELIM, &tail);
23292333
seg;
@@ -2352,6 +2356,10 @@ int wolfSSH_RealPath(const char* defaultPath, char* in,
23522356
}
23532357
/* Everything else is copied */
23542358
else {
2359+
if (curSz >= outSz - segSz) {
2360+
return WS_INVALID_PATH_E;
2361+
}
2362+
23552363
if (curSz != 1) {
23562364
WSTRNCAT(out, "/", outSz - curSz);
23572365
curSz++;

‎src/wolfsftp.c‎

Lines changed: 113 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -1179,42 +1179,59 @@ static int wolfSSH_SFTP_RecvRealPath(WOLFSSH* ssh, int reqId, byte* data,
11791179
r[rSz] = '\0';
11801180
WLOG(WS_LOG_SFTP, "Real Path Request = %s", r);
11811181

1182-
if (ssh->sftpDefaultPath != NULL) {
1183-
ret = wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sizeof s);
1184-
}
1185-
else {
1182+
/* If the default path isn't set, try to get it. */
1183+
if (ssh->sftpDefaultPath == NULL) {
11861184
char wd[WOLFSSH_MAX_FILENAME];
11871185
WMEMSET(wd, 0, WOLFSSH_MAX_FILENAME);
1186+
ret = WS_SUCCESS;
1187+
11881188
#ifndef USE_WINDOWS_API
1189-
if (WGETCWD(ssh->fs, wd, WOLFSSH_MAX_FILENAME) == NULL) {
1190-
WLOG(WS_LOG_SFTP, "Unable to get current working directory");
1191-
if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId,
1192-
"Directory error", "English", NULL, &outSz)
1193-
!= WS_SIZE_ONLY) {
1194-
return WS_FATAL_ERROR;
1195-
}
1196-
out = (byte*) WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER);
1197-
if (out == NULL) {
1198-
return WS_MEMORY_E;
1199-
}
1200-
if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId,
1201-
"Directory error", "English", out, &outSz)
1202-
!= WS_SUCCESS) {
1203-
WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER);
1204-
return WS_FATAL_ERROR;
1205-
}
1206-
/* take over control of buffer */
1207-
wolfSSH_SFTP_RecvSetSend(ssh, out, outSz);
1208-
return WS_BAD_FILE_E;
1209-
}
1189+
if (WGETCWD(ssh->fs, wd, sizeof(wd)-1) == NULL) {
1190+
ret = WS_INVALID_PATH_E;
1191+
}
1192+
#else
1193+
if (GetCurrentDirectoryA(sizeof(wd)-1, wd) == 0) {
1194+
ret = WS_INVALID_PATH_E;
1195+
}
12101196
#endif
1211-
ret = wolfSSH_RealPath(wd, r, s, sizeof s);
1197+
1198+
if (ret == WS_SUCCESS) {
1199+
wd[sizeof(wd) - 1] = 0;
1200+
ret = wolfSSH_RealPath(NULL, wd, s, sizeof s);
1201+
}
1202+
if (ret == WS_SUCCESS) {
1203+
ret = wolfSSH_SFTP_SetDefaultPath(ssh, s);
1204+
}
1205+
}
1206+
1207+
/* If the default path still isn't set, send error to peer. */
1208+
if (ssh->sftpDefaultPath == NULL) {
1209+
WLOG(WS_LOG_SFTP, "Unable to get current working directory");
1210+
if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId,
1211+
"Directory error", "English", NULL, &outSz)
1212+
!= WS_SIZE_ONLY) {
1213+
return WS_FATAL_ERROR;
1214+
}
1215+
out = (byte*) WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER);
1216+
if (out == NULL) {
1217+
return WS_MEMORY_E;
1218+
}
1219+
if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId,
1220+
"Directory error", "English", out, &outSz)
1221+
!= WS_SUCCESS) {
1222+
WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER);
1223+
return WS_FATAL_ERROR;
1224+
}
1225+
/* take over control of buffer */
1226+
wolfSSH_SFTP_RecvSetSend(ssh, out, outSz);
1227+
return ret;
12121228
}
1213-
sSz = (word32)WSTRLEN(s);
12141229

1230+
ret = wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sizeof s);
12151231
if (ret != WS_SUCCESS) {
1216-
return WS_ERROR;
1232+
return ret;
12171233
}
1234+
sSz = (word32)WSTRLEN(s);
12181235

12191236
WLOG(WS_LOG_SFTP, "Real Path Directory = %s", s);
12201237

@@ -1570,7 +1587,8 @@ static int GetAndCleanPath(const char* defaultPath,
15701587
{
15711588
char r[WOLFSSH_MAX_FILENAME];
15721589

1573-
if (sz > sizeof r) sz = sizeof r;
1590+
if (sz >= sizeof r)
1591+
return WS_BUFFER_E;
15741592
WMEMCPY(r, data, sz);
15751593
r[sz] = '\0';
15761594

@@ -1881,9 +1899,10 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
18811899
return WS_BUFFER_E;
18821900
}
18831901

1884-
if (GetAndCleanPath(ssh->sftpDefaultPath,
1885-
data + idx, sz, dir, sizeof(dir)) < 0) {
1886-
return WS_BUFFER_E;
1902+
ret = GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz,
1903+
dir, sizeof(dir));
1904+
if (ret < 0) {
1905+
return ret;
18871906
}
18881907
idx += sz;
18891908

@@ -2156,7 +2175,7 @@ int wolfSSH_SFTP_RecvOpenDir(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
21562175
}
21572176

21582177
if (WOPENDIR(ssh->fs, ssh->ctx->heap, &ctx, dir) != 0) {
2159-
WLOG(WS_LOG_SFTP, "Error with opening directory");
2178+
WLOG(WS_LOG_SFTP, "Error with opening directory: %s", dir);
21602179
if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_NOFILE, reqId,
21612180
"Unable To Open Directory", "English", NULL, &outSz)
21622181
!= WS_SIZE_ONLY) {
@@ -2304,7 +2323,7 @@ int wolfSSH_SFTP_RecvOpenDir(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
23042323
realName, sizeof(realName), &isDir, ssh->ctx->heap);
23052324
if (findHandle == INVALID_HANDLE_VALUE || !isDir) {
23062325

2307-
WLOG(WS_LOG_SFTP, "Error with opening directory");
2326+
WLOG(WS_LOG_SFTP, "Error with opening directory: %s", name);
23082327
WFREE(dirName, ssh->ctx->heap, DYNTYPE_BUFFER);
23092328

23102329
if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_NOFILE, reqId,
@@ -2545,16 +2564,26 @@ static int wolfSSH_SFTPNAME_readdir(WOLFSSH* ssh, WDIR* dir, WS_SFTPNAME* out,
25452564
char r[WOLFSSH_MAX_FILENAME];
25462565
char s[WOLFSSH_MAX_FILENAME];
25472566

2548-
r[0] = '\0';
25492567
if (!special) { /* do not add dir name in special case */
2550-
WSTRNCAT(r, dirName, sizeof(r));
2551-
WSTRNCAT(r, "/", sizeof(r));
2568+
if (WSTRLEN(dirName) + out->fSz + 2 > (sizeof r)) {
2569+
WLOG(WS_LOG_SFTP, "Path length too large");
2570+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
2571+
return WS_FATAL_ERROR;
2572+
}
2573+
WSNPRINTF(r, sizeof(r), "%s/%s", dirName, out->fName);
2574+
}
2575+
else {
2576+
if (out->fSz + 1 > (sizeof r)) {
2577+
WLOG(WS_LOG_SFTP, "Path length too large");
2578+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
2579+
return WS_FATAL_ERROR;
2580+
}
2581+
WSTRNCPY(r, out->fName, sizeof(r));
25522582
}
2553-
WSTRNCAT(r, out->fName, sizeof(r));
2554-
r[sizeof(r) - 1] = 0;
25552583

25562584
if (wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sizeof(s)) < 0) {
25572585
WLOG(WS_LOG_SFTP, "Error cleaning path to get attributes");
2586+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
25582587
return WS_FATAL_ERROR;
25592588
}
25602589

@@ -2626,13 +2655,16 @@ static int wolfSSH_SFTPNAME_readdir(WOLFSSH* ssh, WDIR* dir, WS_SFTPNAME* out,
26262655
char r[WOLFSSH_MAX_FILENAME];
26272656
char s[WOLFSSH_MAX_FILENAME];
26282657

2629-
r[0] = '\0';
2630-
WSTRNCAT(r, dirName, sizeof(r));
2631-
WSTRNCAT(r, "/", sizeof(r));
2632-
WSTRNCAT(r, out->fName, sizeof(r));
2633-
r[sizeof(r) - 1] = 0;
2658+
if ((WSTRLEN(dirName) + WSTRLEN(out->fName) + 2) > sizeof(r)) {
2659+
WLOG(WS_LOG_SFTP, "Path length too large");
2660+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
2661+
return WS_FATAL_ERROR;
2662+
}
2663+
WSNPRINTF(r, sizeof(r), "%s/%s", dirName, out->fName);
26342664

26352665
if (wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sizeof(s)) < 0) {
2666+
WLOG(WS_LOG_SFTP, "Error cleaning path to get attributes");
2667+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
26362668
return WS_FATAL_ERROR;
26372669
}
26382670

@@ -2722,26 +2754,18 @@ static int wolfSSH_SFTPNAME_readdir(WOLFSSH* ssh, WDIR* dir, WS_SFTPNAME* out,
27222754
int bufSz;
27232755
int tmpSz;
27242756

2725-
bufSz = out->fSz + (int)WSTRLEN(dirName) + sizeof(WS_DELIM);
2726-
buf = (char*)WMALLOC(bufSz + 1, out->heap, DYNTYPE_SFTP);
2757+
bufSz = out->fSz + (int)WSTRLEN(dirName) + 2; /* /+nul */
2758+
buf = (char*)WMALLOC(bufSz, out->heap, DYNTYPE_SFTP);
27272759
if (buf == NULL) {
2760+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
27282761
return WS_MEMORY_E;
27292762
}
2730-
buf[0] = '\0';
2731-
if (!special) {
2732-
WSTRNCAT(buf, dirName, bufSz + 1);
2733-
tmpSz = (int)WSTRLEN(buf);
27342763

2735-
/* add delimiter between path and file/dir name */
2736-
if (tmpSz + 1 < bufSz) {
2737-
buf[tmpSz] = WS_DELIM;
2738-
buf[tmpSz + 1] = '\0';
2739-
}
2740-
WSTRNCAT(buf, out->fName, bufSz + 1);
2764+
if (!special) {
2765+
WSNPRINTF(buf, bufSz, "%s/%s", dirName, out->fName);
27412766
}
27422767
else {
2743-
WSTRNCAT(buf, realFileName, bufSz + 1);
2744-
WSTRNCAT(buf, "\\", bufSz + 1);
2768+
WSNPRINTF(buf, bufSz, "%s/", realFileName);
27452769
}
27462770
if (SFTP_GetAttributes(ssh->fs, buf, &out->atrb, 0, ssh->ctx->heap)
27472771
!= WS_SUCCESS) {
@@ -2797,13 +2821,16 @@ static int wolfSSH_SFTPNAME_readdir(WOLFSSH* ssh, WDIR* dir, WS_SFTPNAME* out,
27972821
char r[WOLFSSH_MAX_FILENAME];
27982822
char s[WOLFSSH_MAX_FILENAME];
27992823

2800-
r[0] = '\0';
2801-
WSTRNCAT(r, dirName, sizeof(r));
2802-
WSTRNCAT(r, "/", sizeof(r));
2803-
WSTRNCAT(r, out->fName, sizeof(r));
2804-
r[sizeof(r) - 1] = 0;
2824+
if ((WSTRLEN(dirName) + WSTRLEN(out->fName) + 2) > sizeof(r)) {
2825+
WLOG(WS_LOG_SFTP, "Path length too large");
2826+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
2827+
return WS_FATAL_ERROR;
2828+
}
2829+
WSNPRINTF(r, sizeof(r), "%s/%s", dirName, out->fName);
28052830

28062831
if (wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sizeof(s)) < 0) {
2832+
WLOG(WS_LOG_SFTP, "Error cleaning path to get attributes");
2833+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
28072834
return WS_FATAL_ERROR;
28082835
}
28092836
if (SFTP_GetAttributes(ssh->fs, s, &out->atrb, 0, ssh->ctx->heap)
@@ -2861,13 +2888,16 @@ static int wolfSSH_SFTPNAME_readdir(WOLFSSH* ssh, WDIR* dir, WS_SFTPNAME* out,
28612888
char r[WOLFSSH_MAX_FILENAME];
28622889
char s[WOLFSSH_MAX_FILENAME];
28632890

2864-
r[0] = '\0';
2865-
WSTRNCAT(r, dirName, sizeof(r));
2866-
WSTRNCAT(r, "/", sizeof(r));
2867-
WSTRNCAT(r, out->fName, sizeof(r));
2868-
r[sizeof(r) - 1] = 0;
2891+
if ((WSTRLEN(dirName) + WSTRLEN(out->fName) + 2) > sizeof(r)) {
2892+
WLOG(WS_LOG_SFTP, "Path length too large");
2893+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
2894+
return WS_FATAL_ERROR;
2895+
}
2896+
WSNPRINTF(r, sizeof(r), "%s/%s", dirName, out->fName);
28692897

28702898
if (wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sizeof(s)) < 0) {
2899+
WFREE(out->fName, out->heap, DYNTYPE_SFTP);
2900+
WLOG(WS_LOG_SFTP, "Error cleaning path to get attributes");
28712901
return WS_FATAL_ERROR;
28722902
}
28732903

@@ -3739,10 +3769,8 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
37393769
return WS_BUFFER_E;
37403770
}
37413771

3742-
if (GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz,
3743-
name, sizeof(name)) < 0) {
3744-
ret = WS_BAD_FILE_E;
3745-
}
3772+
ret = GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz,
3773+
name, sizeof(name));
37463774

37473775
if (ret == WS_SUCCESS) {
37483776
#ifndef USE_WINDOWS_API
@@ -3826,19 +3854,21 @@ int wolfSSH_SFTP_RecvRename(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
38263854
if (sz > maxSz - idx) {
38273855
ret = WS_BUFFER_E;
38283856
}
3829-
if (GetAndCleanPath(ssh->sftpDefaultPath,
3830-
data + idx, sz, old, sizeof(old)) < 0) {
3831-
ret = WS_FATAL_ERROR;
3857+
if (ret == WS_SUCCESS) {
3858+
ret = GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz,
3859+
old, sizeof(old));
38323860
}
3833-
idx += sz;
3834-
/* get new file name */
3835-
ato32(data + idx, &sz); idx += UINT32_SZ;
3836-
if (sz > maxSz - idx) {
3837-
ret = WS_BUFFER_E;
3861+
if (ret == WS_SUCCESS) {
3862+
idx += sz;
3863+
/* get new file name */
3864+
ato32(data + idx, &sz); idx += UINT32_SZ;
3865+
if (sz > maxSz - idx) {
3866+
ret = WS_BUFFER_E;
3867+
}
38383868
}
3839-
if (GetAndCleanPath(ssh->sftpDefaultPath,
3840-
data + idx, sz, name, sizeof(name)) < 0) {
3841-
ret = WS_BUFFER_E;
3869+
if (ret == WS_SUCCESS) {
3870+
ret = GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz,
3871+
name, sizeof(name));
38423872
}
38433873

38443874
if (ret == WS_SUCCESS) {

‎tests/api.c‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1086,6 +1086,44 @@ static void DoRealPathTestCase(const char* path, struct RealPathTestCase* tc)
10861086
}
10871087
}
10881088

1089+
1090+
struct RealPathTestFailCase {
1091+
const char* defaultPath;
1092+
const char* in;
1093+
word32 checkPathSz;
1094+
int expErr;
1095+
};
1096+
struct RealPathTestFailCase realPathFail[] = {
1097+
/* Output size less than default path length. */
1098+
{ "12345678", "12345678", 4, WS_INVALID_PATH_E },
1099+
/* Output size equal to default path length. */
1100+
{ "12345678", "12345678", 8, WS_INVALID_PATH_E },
1101+
/* Copy segment will not fit in output. */
1102+
{ "1234567", "12345678", 8, WS_INVALID_PATH_E },
1103+
};
1104+
1105+
static void DoRealPathTestFailCase(struct RealPathTestFailCase* tc)
1106+
{
1107+
char testPath[128];
1108+
char checkPath[128];
1109+
int err;
1110+
1111+
WSTRNCPY(testPath, tc->in, sizeof(testPath) - 1);
1112+
testPath[sizeof(testPath) - 1] = 0;
1113+
WMEMSET(checkPath, 0, sizeof checkPath);
1114+
err = wolfSSH_RealPath(tc->defaultPath, testPath,
1115+
checkPath, tc->checkPathSz);
1116+
if (err != tc->expErr) {
1117+
fprintf(stderr, "RealPath fail check failure (%d)\n"
1118+
" defaultPath: %s\n"
1119+
" input: %s\n"
1120+
" checkPathSz: %u\n"
1121+
" expected: %d\n", err,
1122+
tc->defaultPath, tc->in, tc->checkPathSz, tc->expErr);
1123+
}
1124+
}
1125+
1126+
10891127
static void test_wolfSSH_RealPath(void)
10901128
{
10911129
word32 testCount;
@@ -1100,6 +1138,11 @@ static void test_wolfSSH_RealPath(void)
11001138
for (i = 0; i < testCount; i++) {
11011139
DoRealPathTestCase(NULL, realPathNull + i);
11021140
}
1141+
1142+
testCount = (sizeof realPathFail)/(sizeof(struct RealPathTestFailCase));
1143+
for (i = 0; i < testCount; i++) {
1144+
DoRealPathTestFailCase(realPathFail + i);
1145+
}
11031146
}
11041147
#else
11051148
static void test_wolfSSH_RealPath(void) { ; }

0 commit comments

Comments
 (0)