Skip to content

Commit

Permalink
Fix use after free in sh_funstaks() (re: 7e317c5)
Browse files Browse the repository at this point in the history
The referenced commit introduced the NIL (NULL) assignment in:

	stakdelete(slpold->slptr);
	slpold->slptr = NIL(Stak_t*);

First the stack is closed/freed with stakdelete() a.k.a.
stkclose(), then its pointer is reset. Looks correct, right?

Wrong: slpold may itself be in the allocated region that
slpold->slptr points to. That's because we're dealing with a linked
list of stacks, in which a pointer on each stack points to the next
stack. So there are scenarios in which, after the stakdelete()
call, dereferencing slpold is a use after free.

Most systems quietly tolerate this use after free. But, according
to @JohnoKing's testing, this bug was causing 23 crashes in the
regression tests after compiling ksh with AddressSanitizer enabled.

src/cmd/ksh93/sh/parse.c: sh_funstaks():
- Save the value of slpold->slptr and reset that pointer before
  calling stakdelete() a.k.a. stkclose().

Resolves: #517
  • Loading branch information
McDutchie committed Aug 19, 2022
1 parent 9f30314 commit 59a5672
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/argnod.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ struct comnod
#define COMSCAN (01<<COMBITS)
#define COMFIXED (02<<COMBITS)

struct slnod /* struct for link list of stacks */
struct slnod /* struct for linked list of stacks */
{
struct slnod *slnext;
struct slnod *slchild;
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.1.0-alpha" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2022-08-16" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2022-08-19" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
10 changes: 9 additions & 1 deletion src/cmd/ksh93/sh/parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,16 @@ void sh_funstaks(register struct slnod *slp,int flag)
{
if(flag<=0)
{
stakdelete(slpold->slptr);
/*
* Since we're dealing with a linked list of stacks, slpold may be inside the allocated region
* pointed to by slpold->slptr, meaning the stakdelete() call may invalidate slpold as well as
* slpold->slptr. So if we do 'stakdelete(slpold->slptr); slpold->slptr = NIL(Stak_t*)' as may
* seem obvious, the assignment may be a use-after-free of slpold. Therefore, save the pointer
* value and reset the pointer before closing/freeing the stack.
*/
Stak_t *sp = slpold->slptr;
slpold->slptr = NIL(Stak_t*);
stakdelete(sp);
}
else
staklink(slpold->slptr);
Expand Down

0 comments on commit 59a5672

Please sign in to comment.