Skip to content

Commit 79bc501

Browse files
rdmarkrdmark
committed
afpd,testsuite: replace unsafe strcpy/sprintf with bounded equivalents
Replace remaining sprintf/strcpy/strcat calls flagged by the OpenBSD linker with snprintf/strlcpy/strlcat/memcpy throughout afpd and the test suite. Key fixes beyond mechanical substitution: - dtfile(): build the creator/hex suffix in a fixed local buffer and append via strlcat, closing a buffer overflow in the *p++ writes that could follow a near-full snprintf result - appl.c afp_addappl/afp_rmvappl: guard against NULL return from dtfile() before passing dtf to strlcpy; rmvappl also closes sdt_fd on this new error path to match existing cleanup - file.c set_name: save strlen(name) into tp_len at the strdup site so the restore memcpy uses tp_len+1 instead of a redundant strlen
1 parent 3a36ac9 commit 79bc501

6 files changed

Lines changed: 82 additions & 63 deletions

File tree

‎etc/afpd/appl.c‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,11 @@ int afp_addappl(AFPObj *obj, char *ibuf, size_t ibuflen _U_, char *rbuf _U_,
272272
}
273273

274274
dtf = dtfile(vol, creator, ".appl.temp");
275+
276+
if (!dtf) {
277+
return AFPERR_PARAM;
278+
}
279+
275280
tempfile = obj->oldtmp;
276281
strlcpy(tempfile, dtf, AFPOBJ_TMPSIZ + 1);
277282

@@ -400,6 +405,13 @@ int afp_rmvappl(AFPObj *obj, char *ibuf, size_t ibuflen _U_, char *rbuf _U_,
400405
}
401406

402407
dtf = dtfile(vol, creator, ".appl.temp");
408+
409+
if (!dtf) {
410+
close(sa.sdt_fd);
411+
sa.sdt_fd = -1;
412+
return AFPERR_PARAM;
413+
}
414+
403415
/* Use existing tempfile buffers but check if the path fits */
404416
tempfile = obj->oldtmp;
405417

‎etc/afpd/desktop.c‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ typedef void (*desk_entry_fn)(const char *path, void *ctx);
6969
static void desk_foreach(DIR *desk, desk_entry_fn fn, void *ctx)
7070
{
7171
char modbuf[12 + 1], *m;
72-
struct dirent *deskp, *subp;
72+
const struct dirent *deskp, *subp;
7373
DIR *sub;
7474

7575
for (deskp = readdir(desk); deskp != NULL; deskp = readdir(desk)) {
@@ -805,7 +805,6 @@ static const char hexdig[] = "0123456789abcdef";
805805
char *dtfile(const struct vol *vol, uint8_t creator[], char *ext)
806806
{
807807
static char path[MAXPATHLEN + 1];
808-
char *p;
809808
unsigned int i;
810809

811810
if (vol == NULL || vol->v_dbpath == NULL) {
@@ -819,29 +818,36 @@ char *dtfile(const struct vol *vol, uint8_t creator[], char *ext)
819818
return NULL;
820819
}
821820

822-
for (p = path; *p != '\0'; p++)
823-
;
821+
/* Build "X/CCCC" suffix (2-char hex dir + '/' + up to 8-char creator + NUL) */
822+
char suffix[12];
823+
char *q = suffix;
824824

825825
if (!isprint(creator[0]) || creator[0] == '/') {
826-
*p++ = hexdig[(creator[0] & 0xf0) >> 4];
827-
*p++ = hexdig[creator[0] & 0x0f];
826+
*q++ = hexdig[(creator[0] & 0xf0) >> 4];
827+
*q++ = hexdig[creator[0] & 0x0f];
828828
} else {
829-
*p++ = creator[0];
829+
*q++ = creator[0];
830830
}
831831

832-
*p++ = '/';
832+
*q++ = '/';
833833

834834
for (i = 0; i < sizeof(CreatorType); i++) {
835835
if (!isprint(creator[i]) || creator[i] == '/') {
836-
*p++ = hexdig[(creator[i] & 0xf0) >> 4];
837-
*p++ = hexdig[creator[i] & 0x0f];
836+
*q++ = hexdig[(creator[i] & 0xf0) >> 4];
837+
*q++ = hexdig[creator[i] & 0x0f];
838838
} else {
839-
*p++ = creator[i];
839+
*q++ = creator[i];
840840
}
841841
}
842842

843-
*p = '\0';
844-
strlcat(path, ext, sizeof(path));
843+
*q = '\0';
844+
845+
if (strlcat(path, suffix, sizeof(path)) >= sizeof(path) ||
846+
strlcat(path, ext, sizeof(path)) >= sizeof(path)) {
847+
LOG(log_error, logtype_afpd, "dtfile: path too long");
848+
return NULL;
849+
}
850+
845851
return path;
846852
}
847853

‎etc/afpd/file.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ char *set_name(const struct vol *vol, char *data, cnid_t pid, char *name,
158158
cnid_t id, uint32_t utf8)
159159
{
160160
uint32_t aint;
161+
uint32_t tp_len = 0;
161162
char *tp = NULL;
162163
char *src = name;
163164
aint = strlen(name);
@@ -168,6 +169,7 @@ char *set_name(const struct vol *vol, char *data, cnid_t pid, char *name,
168169
/* but name is an utf8 mac name */
169170
char *u, *m;
170171
/* global static variable... */
172+
tp_len = aint;
171173
tp = strdup(name);
172174

173175
if (!(u = mtoupath(vol, name, pid, 1)) || !(m = utompath(vol, u, id, 0))) {
@@ -203,7 +205,7 @@ char *set_name(const struct vol *vol, char *data, cnid_t pid, char *name,
203205
data += aint;
204206

205207
if (tp) {
206-
strcpy(name, tp);
208+
memcpy(name, tp, tp_len + 1);
207209
free(tp);
208210
}
209211

‎test/testsuite/FPGetFileDirParms.c‎

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -615,13 +615,13 @@ STATIC void test308()
615615
}
616616
}
617617

618-
sprintf(temp, "t307#%X", ntohl(dir));
618+
snprintf(temp, sizeof(temp), "t307#%X", ntohl(dir));
619619

620620
if (!FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, 0, bitmap)) {
621621
test_failed();
622622
}
623623

624-
sprintf(temp, "t308#%X", ntohl(dir));
624+
snprintf(temp, sizeof(temp), "t308#%X", ntohl(dir));
625625

626626
if (!FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, 0, bitmap)) {
627627
test_failed();
@@ -683,10 +683,10 @@ STATIC void test324()
683683
test_nottested();
684684
}
685685

686-
sprintf(temp1, "#%X.txt", ntohl(id));
686+
snprintf(temp1, sizeof(temp1), "#%X.txt", ntohl(id));
687687
memset(temp, 0, sizeof(temp));
688688
strncpy(temp, name, 31 - strlen(temp1));
689-
strcat(temp, temp1);
689+
strlcat(temp, temp1, sizeof(temp));
690690

691691
if (FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0)) {
692692
test_failed();
@@ -738,10 +738,10 @@ STATIC void test326()
738738
goto test_exit;
739739
}
740740

741-
sprintf(temp1, "#%X.txt", ntohl(id));
741+
snprintf(temp1, sizeof(temp1), "#%X.txt", ntohl(id));
742742
memset(temp, 0, sizeof(temp));
743743
strncpy(temp, name, 31 - strlen(temp1));
744-
strcat(temp, temp1);
744+
strlcat(temp, temp1, sizeof(temp));
745745
ret = FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0);
746746

747747
if ((Conn->afp_version >= 30 && ret != ntohl(AFPERR_NOOBJ))
@@ -789,19 +789,19 @@ STATIC void test333()
789789
test_nottested();
790790
}
791791

792-
sprintf(temp1, "#%X.txt", ntohl(id));
792+
snprintf(temp1, sizeof(temp1), "#%X.txt", ntohl(id));
793793
memset(temp, 0, sizeof(temp));
794794
strncpy(temp, name, 31 - strlen(temp1));
795-
strcat(temp, temp1);
795+
strlcat(temp, temp1, sizeof(temp));
796796

797797
if (FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0)) {
798798
test_failed();
799799
}
800800

801-
sprintf(temp1, "#0%X.txt", ntohl(id));
801+
snprintf(temp1, sizeof(temp1), "#0%X.txt", ntohl(id));
802802
memset(temp, 0, sizeof(temp));
803803
strncpy(temp, name, 31 - strlen(temp1));
804-
strcat(temp, temp1);
804+
strlcat(temp, temp1, sizeof(temp));
805805

806806
if (!FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0)) {
807807
test_failed();
@@ -846,19 +846,19 @@ STATIC void test334()
846846
test_nottested();
847847
}
848848

849-
sprintf(temp1, "#%X", ntohl(id));
849+
snprintf(temp1, sizeof(temp1), "#%X", ntohl(id));
850850
memset(temp, 0, sizeof(temp));
851851
strncpy(temp, name, 31 - strlen(temp1));
852-
strcat(temp, temp1);
852+
strlcat(temp, temp1, sizeof(temp));
853853

854854
if (FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0)) {
855855
test_failed();
856856
}
857857

858-
sprintf(temp1, "#%X.", ntohl(id));
858+
snprintf(temp1, sizeof(temp1), "#%X.", ntohl(id));
859859
memset(temp, 0, sizeof(temp));
860860
strncpy(temp, name, 31 - strlen(temp1));
861-
strcat(temp, temp1);
861+
strlcat(temp, temp1, sizeof(temp));
862862

863863
if (!FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0)) {
864864
test_failed();
@@ -912,10 +912,10 @@ STATIC void test335()
912912
test_nottested();
913913
}
914914

915-
sprintf(temp1, "#%X.txt", ntohl(id));
915+
snprintf(temp1, sizeof(temp1), "#%X.txt", ntohl(id));
916916
memset(temp, 0, sizeof(temp));
917917
strncpy(temp, name, 31 - strlen(temp1));
918-
strcat(temp, temp1);
918+
strlcat(temp, temp1, sizeof(temp));
919919

920920
if (FPGetFileDirParams(Conn, vol, DIRDID_ROOT, temp, bitmap, 0)) {
921921
test_failed();
@@ -1250,7 +1250,6 @@ static void do_pdinfo_test(char *fname, const char *fourcc, uint8_t expect_type,
12501250
int ofs = 3 * sizeof(uint16_t);
12511251
const DSI *dsi = &Conn->dsi;
12521252
struct afp_filedir_parms filedir = { 0 };
1253-
const unsigned char *buf;
12541253
const char *creator = "pdos";
12551254
uint8_t prodos_type;
12561255
uint16_t prodos_aux;
@@ -1343,7 +1342,6 @@ STATIC void test441()
13431342
int ofs = 3 * sizeof(uint16_t);
13441343
const DSI *dsi = &Conn->dsi;
13451344
struct afp_filedir_parms filedir = { 0 };
1346-
const unsigned char *buf;
13471345
uint8_t prodos_type;
13481346
uint16_t prodos_aux;
13491347
ENTER_TEST
@@ -1404,7 +1402,6 @@ STATIC void test442()
14041402
int ofs = 3 * sizeof(uint16_t);
14051403
const DSI *dsi = &Conn->dsi;
14061404
struct afp_filedir_parms filedir = { 0 };
1407-
const unsigned char *buf;
14081405
uint8_t prodos_type;
14091406
uint16_t prodos_aux;
14101407
int dir;

‎test/testsuite/FPSetFileParms.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,7 @@ STATIC void test433()
513513
uint16_t bitmap_date = (1 << FILPBIT_CDATE) | (1 << FILPBIT_BDATE) |
514514
(1 << FILPBIT_MDATE);
515515
int ofs = 3 * sizeof(uint16_t);
516-
DSI *dsi = &Conn->dsi;
516+
const DSI *dsi = &Conn->dsi;
517517
struct afp_filedir_parms filedir;
518518
ENTER_TEST
519519

@@ -600,7 +600,7 @@ STATIC void test434()
600600
uint16_t bitmap_date = (1 << FILPBIT_CDATE) | (1 << FILPBIT_BDATE) |
601601
(1 << FILPBIT_MDATE);
602602
int ofs = 3 * sizeof(uint16_t);
603-
DSI *dsi = &Conn->dsi;
603+
const DSI *dsi = &Conn->dsi;
604604
struct afp_filedir_parms filedir;
605605
ENTER_TEST
606606

0 commit comments

Comments
 (0)