Skip to content

Commit e1cda74

Browse files
committed
SECURITY: fix integer overflows related to file descriptor parsing
bug initially found by Pawel Wylecial (LP#1440685) additional bug found and suggested fix by enh (elliott hughes) This commit also renames struct ioword.flag to ioflag to disambiguate it from other members named “flag”, changes it to an unsigned type, and packs ioflag and unit into shorts each, to make the struct smaller (aligned even: 16 bytes on 32-bit systems) and reviews some of the code involved in fd handling, though there wasn’t much to be found.
1 parent 3c5c5d0 commit e1cda74

File tree

7 files changed

+74
-67
lines changed

7 files changed

+74
-67
lines changed

eval.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
#include "sh.h"
2525

26-
__RCSID("$MirOS: src/bin/mksh/eval.c,v 1.166 2015/02/20 07:14:29 tg Exp $");
26+
__RCSID("$MirOS: src/bin/mksh/eval.c,v 1.167 2015/04/11 22:03:29 tg Exp $");
2727

2828
/*
2929
* string expansion
@@ -1353,7 +1353,7 @@ comsub(Expand *xp, const char *cp, int fn MKSH_A_UNUSED)
13531353
struct ioword *io = *t->ioact;
13541354
char *name;
13551355

1356-
if ((io->flag & IOTYPE) != IOREAD)
1356+
if ((io->ioflag & IOTYPE) != IOREAD)
13571357
errorf("%s: %s", "funny $() command",
13581358
snptreef(NULL, 32, "%R", io));
13591359
shf = shf_open(name = evalstr(io->name, DOTILDE), O_RDONLY, 0,

exec.c

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
#include "sh.h"
2525

26-
__RCSID("$MirOS: src/bin/mksh/exec.c,v 1.147 2015/03/20 23:37:54 tg Exp $");
26+
__RCSID("$MirOS: src/bin/mksh/exec.c,v 1.148 2015/04/11 22:03:29 tg Exp $");
2727

2828
#ifndef MKSH_DEFAULT_EXECSHELL
2929
#define MKSH_DEFAULT_EXECSHELL "/bin/sh"
@@ -92,7 +92,7 @@ execute(struct op * volatile t,
9292
t->ioact != NULL && t->ioact[0] != NULL &&
9393
t->ioact[1] == NULL &&
9494
/* of type "here document" (or "here string") */
95-
(t->ioact[0]->flag & IOTYPE) == IOHERE &&
95+
(t->ioact[0]->ioflag & IOTYPE) == IOHERE &&
9696
/* the variable assignment begins with a valid varname */
9797
(ccp = skip_wdvarname(t->vars[0], true)) != t->vars[0] &&
9898
/* and has no right-hand side (i.e. "varname=") */
@@ -1319,7 +1319,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
13191319
{
13201320
int u = -1;
13211321
char *cp = iop->name;
1322-
int iotype = iop->flag & IOTYPE;
1322+
int iotype = iop->ioflag & IOTYPE;
13231323
bool do_open = true, do_close = false;
13241324
int flags = 0;
13251325
struct ioword iotmp;
@@ -1331,7 +1331,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
13311331
/* Used for tracing and error messages to print expanded cp */
13321332
iotmp = *iop;
13331333
iotmp.name = (iotype == IOHERE) ? NULL : cp;
1334-
iotmp.flag |= IONAMEXP;
1334+
iotmp.ioflag |= IONAMEXP;
13351335

13361336
if (Flag(FXTRACE)) {
13371337
change_xtrace(2, false);
@@ -1354,7 +1354,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
13541354
* The stat() is here to allow redirections to
13551355
* things like /dev/null without error.
13561356
*/
1357-
if (Flag(FNOCLOBBER) && !(iop->flag & IOCLOB) &&
1357+
if (Flag(FNOCLOBBER) && !(iop->ioflag & IOCLOB) &&
13581358
(stat(cp, &statb) < 0 || S_ISREG(statb.st_mode)))
13591359
flags |= O_EXCL;
13601360
break;
@@ -1379,7 +1379,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
13791379
u = 1009;
13801380
do_close = true;
13811381
} else if ((u = check_fd(cp,
1382-
X_OK | ((iop->flag & IORDUP) ? R_OK : W_OK),
1382+
X_OK | ((iop->ioflag & IORDUP) ? R_OK : W_OK),
13831383
&emsg)) < 0) {
13841384
char *sp;
13851385

@@ -1388,7 +1388,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
13881388
afree(sp, ATEMP);
13891389
return (-1);
13901390
}
1391-
if (u == iop->unit)
1391+
if (u == (int)iop->unit)
13921392
/* "dup from" == "dup to" */
13931393
return (0);
13941394
break;
@@ -1416,7 +1416,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
14161416
/* Do not save if it has already been redirected (i.e. "cat >x >y"). */
14171417
if (e->savefd[iop->unit] == 0) {
14181418
/* If these are the same, it means unit was previously closed */
1419-
if (u == iop->unit)
1419+
if (u == (int)iop->unit)
14201420
e->savefd[iop->unit] = -1;
14211421
else
14221422
/*
@@ -1431,7 +1431,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
14311431

14321432
if (do_close)
14331433
close(iop->unit);
1434-
else if (u != iop->unit) {
1434+
else if (u != (int)iop->unit) {
14351435
if (ksh_dup2(u, iop->unit, true) < 0) {
14361436
int eno;
14371437
char *sp;
@@ -1453,7 +1453,7 @@ iosetup(struct ioword *iop, struct tbl *tp)
14531453
* causes the shell to close its copies
14541454
*/
14551455
else if (tp && tp->type == CSHELL && tp->val.f == c_exec) {
1456-
if (iop->flag & IORDUP)
1456+
if (iop->ioflag & IORDUP)
14571457
/* possible exec <&p */
14581458
coproc_read_close(u);
14591459
else
@@ -1522,7 +1522,7 @@ herein(struct ioword *iop, char **resbuf)
15221522
}
15231523

15241524
/* lexer substitution flags */
1525-
i = (iop->flag & IOEVAL) ? (ONEWORD | HEREDOC) : 0;
1525+
i = (iop->ioflag & IOEVAL) ? (ONEWORD | HEREDOC) : 0;
15261526

15271527
/* skip all the fd setup if we just want the value */
15281528
if (resbuf != NULL)

lex.c

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
#include "sh.h"
2525

26-
__RCSID("$MirOS: src/bin/mksh/lex.c,v 1.198 2015/03/20 23:37:39 tg Exp $");
26+
__RCSID("$MirOS: src/bin/mksh/lex.c,v 1.199 2015/04/11 22:03:30 tg Exp $");
2727

2828
/*
2929
* states while lexing word
@@ -921,42 +921,41 @@ yylex(int cf)
921921
if (!ksh_isdigit(dp[c2 + 1]))
922922
goto no_iop;
923923
iop->unit = (iop->unit * 10) + dp[c2 + 1] - '0';
924+
if (iop->unit >= FDBASE)
925+
goto no_iop;
924926
}
925927

926-
if (iop->unit >= FDBASE)
927-
goto no_iop;
928-
929928
if (c == '&') {
930929
if ((c2 = getsc()) != '>') {
931930
ungetsc(c2);
932931
goto no_iop;
933932
}
934933
c = c2;
935-
iop->flag = IOBASH;
934+
iop->ioflag = IOBASH;
936935
} else
937-
iop->flag = 0;
936+
iop->ioflag = 0;
938937

939938
c2 = getsc();
940939
/* <<, >>, <> are ok, >< is not */
941940
if (c == c2 || (c == '<' && c2 == '>')) {
942-
iop->flag |= c == c2 ?
941+
iop->ioflag |= c == c2 ?
943942
(c == '>' ? IOCAT : IOHERE) : IORDWR;
944-
if (iop->flag == IOHERE) {
943+
if (iop->ioflag == IOHERE) {
945944
if ((c2 = getsc()) == '-') {
946-
iop->flag |= IOSKIP;
945+
iop->ioflag |= IOSKIP;
947946
c2 = getsc();
948947
} else if (c2 == '<')
949-
iop->flag |= IOHERESTR;
948+
iop->ioflag |= IOHERESTR;
950949
ungetsc(c2);
951950
if (c2 == '\n')
952-
iop->flag |= IONDELIM;
951+
iop->ioflag |= IONDELIM;
953952
}
954953
} else if (c2 == '&')
955-
iop->flag |= IODUP | (c == '<' ? IORDUP : 0);
954+
iop->ioflag |= IODUP | (c == '<' ? IORDUP : 0);
956955
else {
957-
iop->flag |= c == '>' ? IOWRITE : IOREAD;
956+
iop->ioflag |= c == '>' ? IOWRITE : IOREAD;
958957
if (c == '>' && c2 == '|')
959-
iop->flag |= IOCLOB;
958+
iop->ioflag |= IOCLOB;
960959
else
961960
ungetsc(c2);
962961
}
@@ -1124,7 +1123,7 @@ gethere(bool iseof)
11241123
struct ioword **p;
11251124

11261125
for (p = heres; p < herep; p++)
1127-
if (iseof && !((*p)->flag & IOHERESTR))
1126+
if (iseof && !((*p)->ioflag & IOHERESTR))
11281127
/* only here strings at EOF */
11291128
return;
11301129
else
@@ -1145,7 +1144,7 @@ readhere(struct ioword *iop)
11451144
char *xp;
11461145
int xpos;
11471146

1148-
if (iop->flag & IOHERESTR) {
1147+
if (iop->ioflag & IOHERESTR) {
11491148
/* process the here string */
11501149
iop->heredoc = xp = evalstr(iop->delim, DOBLANK);
11511150
xpos = strlen(xp) - 1;
@@ -1154,9 +1153,9 @@ readhere(struct ioword *iop)
11541153
return;
11551154
}
11561155

1157-
eof = iop->flag & IONDELIM ? "<<" : evalstr(iop->delim, 0);
1156+
eof = iop->ioflag & IONDELIM ? "<<" : evalstr(iop->delim, 0);
11581157

1159-
if (!(iop->flag & IOEVAL))
1158+
if (!(iop->ioflag & IOEVAL))
11601159
ignore_backslash_newline++;
11611160

11621161
Xinit(xs, xp, 256, ATEMP);
@@ -1165,7 +1164,7 @@ readhere(struct ioword *iop)
11651164
/* beginning of line */
11661165
eofp = eof;
11671166
xpos = Xsavepos(xs, xp);
1168-
if (iop->flag & IOSKIP) {
1167+
if (iop->ioflag & IOSKIP) {
11691168
/* skip over leading tabs */
11701169
while ((c = getsc()) == '\t')
11711170
; /* nothing */
@@ -1227,7 +1226,7 @@ readhere(struct ioword *iop)
12271226
Xput(xs, xp, '\0');
12281227
iop->heredoc = Xclose(xs, xp);
12291228

1230-
if (!(iop->flag & IOEVAL))
1229+
if (!(iop->ioflag & IOEVAL))
12311230
ignore_backslash_newline--;
12321231
}
12331232

main.c

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
#include <locale.h>
3535
#endif
3636

37-
__RCSID("$MirOS: src/bin/mksh/main.c,v 1.290 2015/03/14 05:23:15 tg Exp $");
37+
__RCSID("$MirOS: src/bin/mksh/main.c,v 1.291 2015/04/11 22:03:30 tg Exp $");
3838

3939
extern char **environ;
4040

@@ -1453,13 +1453,20 @@ closepipe(int *pv)
14531453
int
14541454
check_fd(const char *name, int mode, const char **emsgp)
14551455
{
1456-
int fd, fl;
1456+
int fd = 0, fl;
14571457

14581458
if (name[0] == 'p' && !name[1])
14591459
return (coproc_getfd(mode, emsgp));
1460-
for (fd = 0; ksh_isdigit(*name); ++name)
1460+
while (ksh_isdigit(*name)) {
14611461
fd = (fd * 10) + *name - '0';
1462-
if (*name || fd >= FDBASE) {
1462+
if (fd >= FDBASE) {
1463+
if (emsgp)
1464+
*emsgp = "file descriptor too large";
1465+
return (-1);
1466+
}
1467+
++name;
1468+
}
1469+
if (*name) {
14631470
if (emsgp)
14641471
*emsgp = "illegal file descriptor name";
14651472
return (-1);

sh.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@
169169
#endif
170170

171171
#ifdef EXTERN
172-
__RCSID("$MirOS: src/bin/mksh/sh.h,v 1.721 2015/03/20 23:37:55 tg Exp $");
172+
__RCSID("$MirOS: src/bin/mksh/sh.h,v 1.722 2015/04/11 22:03:31 tg Exp $");
173173
#endif
174174
#define MKSH_VERSION "R50 2015/03/20"
175175

@@ -1341,11 +1341,11 @@ struct op {
13411341
* IO redirection
13421342
*/
13431343
struct ioword {
1344-
int unit; /* unit affected */
1345-
int flag; /* action (below) */
1346-
char *name; /* file name (unused if heredoc) */
1347-
char *delim; /* delimiter for <<,<<- */
1348-
char *heredoc; /* content of heredoc */
1344+
char *name; /* filename (unused if heredoc) */
1345+
char *delim; /* delimiter for <<, <<- */
1346+
char *heredoc; /* content of heredoc */
1347+
unsigned short ioflag; /* action (below) */
1348+
short unit; /* unit (fd) affected */
13491349
};
13501350

13511351
/* ioword.flag - type of redirection */

syn.c

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
#include "sh.h"
2525

26-
__RCSID("$MirOS: src/bin/mksh/syn.c,v 1.99 2015/03/14 05:23:18 tg Exp $");
26+
__RCSID("$MirOS: src/bin/mksh/syn.c,v 1.100 2015/04/11 22:03:32 tg Exp $");
2727

2828
struct nesting_state {
2929
int start_token; /* token than began nesting (eg, FOR) */
@@ -189,24 +189,24 @@ synio(int cf)
189189
return (NULL);
190190
ACCEPT;
191191
iop = yylval.iop;
192-
if (iop->flag & IONDELIM)
192+
if (iop->ioflag & IONDELIM)
193193
goto gotnulldelim;
194-
ishere = (iop->flag & IOTYPE) == IOHERE;
194+
ishere = (iop->ioflag & IOTYPE) == IOHERE;
195195
musthave(LWORD, ishere ? HEREDELIM : 0);
196196
if (ishere) {
197197
iop->delim = yylval.cp;
198198
if (*ident != 0) {
199199
/* unquoted */
200200
gotnulldelim:
201-
iop->flag |= IOEVAL;
201+
iop->ioflag |= IOEVAL;
202202
}
203203
if (herep > &heres[HERES - 1])
204204
yyerror("too many %ss\n", "<<");
205205
*herep++ = iop;
206206
} else
207207
iop->name = yylval.cp;
208208

209-
if (iop->flag & IOBASH) {
209+
if (iop->ioflag & IOBASH) {
210210
char *cp;
211211

212212
nextiop = alloc(sizeof(*iop), ATEMP);
@@ -220,9 +220,9 @@ synio(int cf)
220220
*cp++ = '0' + (iop->unit % 10);
221221
*cp = EOS;
222222

223-
iop->flag &= ~IOBASH;
223+
iop->ioflag &= ~IOBASH;
224224
nextiop->unit = 2;
225-
nextiop->flag = IODUP;
225+
nextiop->ioflag = IODUP;
226226
nextiop->delim = NULL;
227227
nextiop->heredoc = NULL;
228228
}
@@ -996,9 +996,10 @@ dbtestp_isa(Test_env *te, Test_meta meta)
996996
ret = c == /*(*/ ')' ? TO_NONNULL : TO_NONOP;
997997
else if (meta == TM_UNOP || meta == TM_BINOP) {
998998
if (meta == TM_BINOP && c == REDIR &&
999-
(yylval.iop->flag == IOREAD || yylval.iop->flag == IOWRITE)) {
999+
(yylval.iop->ioflag == IOREAD ||
1000+
yylval.iop->ioflag == IOWRITE)) {
10001001
ret = TO_NONNULL;
1001-
save = wdcopy(yylval.iop->flag == IOREAD ?
1002+
save = wdcopy(yylval.iop->ioflag == IOREAD ?
10021003
db_lthan : db_gthan, ATEMP);
10031004
} else if (uqword && (ret = test_isop(meta, ident)))
10041005
save = yylval.cp;

0 commit comments

Comments
 (0)