[9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)

[9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)

[igor]

Below is a patch (inline) that fixes two variants of a suicide (use
after free, double free) in Mail/mbox.c on latest 9front 'Mit
Fruchtgeschmack' (explanation with reproducible test instructions
below) when messages are deleted/moved in bulk:

<patch>
diff -r 4dfbef4fa4ac sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Sat Apr 03 19:32:47 2021 +0200
+++ b/sys/src/cmd/upas/Mail/mbox.c	Fri Apr 09 02:54:10 2021 +0200
@@ -675,10 +675,11 @@
 static void
 mbflush(char **, int)
 {
-	int i, j, ln, fd;
+	int i, j, ln, fd, nmesg;
 	char *path;
 	Mesg *m, *p;
 
+	nmesg = mbox.nmesg;
 	path = estrjoin(maildir, "/ctl", nil);
 	fd = open(path, OWRITE);
 	free(path);
@@ -708,11 +709,13 @@
 			mbredraw(m->child[j], 1, 1);
  	}
 
-	for(i = 0, j = 0; i < mbox.nmesg; i++){
+	for(i = 0, j = 0; i < nmesg; i++){
 		m = mbox.mesg[i];
-		if((m->state & Szap) != 0)
+		if((m->state & Szap) != 0){
 			mesgfree(m);
-		else
+			mbox.mesg[i] = nil;
+			mbox.nmesg--;
+		}else
 			mbox.mesg[j++] = m;
 	}
</patch>

To reproduce a suicude create a dummy IMAP folder, copy some emails
to it, and then select all emails and middle click 'Delmesg' followed by
middle clicking 'Put'.

Here is how to create a dummy IMAP folder 'Einhorn' and copy 50 mails
from your Inbox to it without leaving the comfort of 9front (replace
mail.9lab.org with your IMAP server):

<snip>
term% tlsclient tcp!mail.9lab.org!imaps
a  login YOURUSER YOURPASSWORD
b  create Einhorn
c  select Inbox
d  copy 1:50 Einhorn
e  logout
</snap>

Now you can open the 'Einhorn' folder (replace mail.9lab.org with your
URL and igor with your user name):

<snip>
term% echo open /imaps/mail.9lab.org/igor/Einhorn Einhorn  >/mail/fs/ctl
</snap>

Finally, select and middle click 'Mail Einhorn' in acme.  Now you can
try to remove all mails from the 'Einhorn' folder by selecting all
messages and middle clicking 'Delmesg' followed by 'Put'.

There is a very high chance upas/Mail will commit suicide like this:

<snip>
term% broke
echo kill>/proc/1136/ctl # Mail
term% acid 1136
/proc/1136/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: lstk()
strcmp(s2=0x463e88)+0x12 /sys/src/libc/port/strcmp.c:10
mesgmatch(m=0x412b00,name=0x463e88,digest=0x412c80)+0x24 /sys/src/cmd/upas/Mail/mesg.c:483
mesglookup(digest=0x412c80)+0x56 /sys/src/cmd/upas/Mail/mesg.c:496
	buf=0x2f33
	i=0x2f3300000001
delete(digest=0x412c80)+0x13 /sys/src/cmd/upas/Mail/mbox.c:336
changemesg(pm=0x4236b8)+0x181 /sys/src/cmd/upas/Mail/mbox.c:838
	digest=0x412c80
	action=0x412b00
	r=0x40fe00
mbmain(cmd=0x40fc80)+0x240 /sys/src/cmd/upas/Mail/mbox.c:966
	a=0x40fda0
	ev=0x40ee90
	psee=0x4236b8
	pshow=0xfefefefefefefefe
	psend=0xfefefefefefefefe
launcheramd64(arg=0x40fc80,f=0x2023d5)+0x10 /sys/src/libthread/amd64.c:11
</snap>

With the above commands to copy mails between folders you can create
more variations of deleting messages in bulk from the 'Einhorn' test
folder without breaking a sweat. Chances are high you will run into
another suicide like this one:

<snip>
panic: D2B called on non-block 40f9e8 (double-free?)
Mail 1967: suicide: sys: trap: fault read addr=0x0 pc=0x20fbdd
...
term% broke
echo kill>/proc/1967/ctl # Mail
term% acid 1967
/proc/1967/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: lstk()
abort()+0x0 /sys/src/libc/9sys/abort.c:6
ppanic(fmt=0x4055b1)+0x14b /sys/src/libc/port/malloc.c:166
	pv=0x407068
	msg=0x406f50
	v=0x46a810
	n=0x46a8100000002d
D2B(v=0x40f168)+0x6a /sys/src/libc/port/pool.c:926
	a=0x40f160
poolfreel(v=0x40f168,p=0x4001d8)+0x23 /sys/src/libc/port/pool.c:1153
	ab=0x407068
poolfree(p=0x4001d8,v=0x40f168)+0x3e /sys/src/libc/port/pool.c:1287
free()+0x26 /sys/src/libc/port/malloc.c:250
mesgclear(m=0x47ed80)+0x52 /sys/src/cmd/upas/Mail/mesg.c:26
	i=0x202ce000000001
mesgfree(m=0x47ed80)+0x19 /sys/src/cmd/upas/Mail/mesg.c:47
mbflush()+0x217 /sys/src/cmd/upas/Mail/mbox.c:714
	path=0x47fd00
	fd=0x10
	i=0x20231400000004
	p=0x0
	m=0x47ed80
	j=0x400000000
doevent(ev=0x494040)+0x11f /sys/src/cmd/upas/Mail/mbox.c:913
	f=0x494058
	nf=0x49405800000001
	p=0x406b98
mbmain(cmd=0x40fc80)+0x205 /sys/src/cmd/upas/Mail/mbox.c:962
	a=0x40fda0
	ev=0x494040
	psee=0x494e60
	pshow=0xfefefefefefefefe
	psend=0xfefefefefefefefe
launcheramd64(arg=0x40fc80,f=0x2023d5)+0x10 /sys/src/libthread/amd64.c:11
</snap>

The first suicide is caused by use after free, the second suicude is caused by
a double free.

After reviewing the code paths outlined in the stack trace as well as
some surrounding logic, I think the problem is due to not decrementing
'mbox.nmesg' when messages are freed. Hence logic that iterates over
already deleted messages might examine already deleted/freed emails.

A quick grep in cmd/upas/Mail confirms this hypothesis:

<snip>
term% g nmesg
mail.h:134: 	int	nmesg;
mail.h:137: 	Mesg	*openmesg;
mesg.c:383: 	m->qnext = mbox.openmesg;
mesg.c:384: 	mbox.openmesg = m;
mesg.c:422: 	for(pm = &mbox.openmesg; *pm != nil; pm = &(*pm)->qnext)
mesg.c:495: 	for(i = 0; i < mbox.nmesg; i++)
mbox.c:154: 	for(i = 0; i < mbox.nmesg; i++){
mbox.c:167: 	assert(n + o <= mbox.nmesg);
mbox.c:199: 	for(i = 0; i < mbox.nmesg; i++)
mbox.c:259: 	if(mbox.nmesg == mbox.mesgsz){
mbox.c:266: 		idx = mbox.nmesg;
mbox.c:267: 	memmove(&mbox.mesg[idx + 1], &mbox.mesg[idx], (mbox.nmesg - idx)*sizeof(Mesg*));
mbox.c:269: 	mbox.nmesg++;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mbox.c:274: 	if(mbox.hashsz <= 2*(mbox.nmesg + mbox.ndead)){
mbox.c:277: 		if(mbox.hashsz <= 2*mbox.nmesg)
mbox.c:439: 	qsort(mbox.mesg, mbox.nmesg, sizeof(Mesg*), cmpmesg);	
mbox.c:687: 	for(i = 0; i < mbox.nmesg; i++){
mbox.c:711: 	for(i = 0, j = 0; i < mbox.nmesg; i++){
mbox.c:749: 	for(i = 0; i < mbox.nmesg; i++){
mbox.c:771: 	for(m = mbox.openmesg; m != nil; m = m->qnext)
mbox.c:789: 	for(i = idx; i < mbox.nmesg; i++)
</snap>

We only increment 'mbox.nmesg' (see highlighted line).  There does not
seem to be a place where it is decremented.

Since 'mbox.nmesg' is not decremented after freeing/deleting emails,
any for loop like this one:

	for(i = 0; i < mbox.nmesg; i++)
		if(cmpmesg(&mbox.mesg[i], &m) >= 0)
			break;

executed after freeing/deleting emails, may access already freed data.

The fix proposed in the above patch decrements 'mbox.nmesg' when
deleting emails to avoid use after free suicides.

I have been running with this fix for a while and have not observed a
suicide since. I delete/free mails in bulk a lot after copying them
from my IMAP Inbox folder to other IMAP folders. The way to move emails
in IMAP is to copy and then delete. To copy mails I use some hacked up
rc scripts that can be middle clicked in acme to automate the process.

Danke für das neueste Release, ich mag Fruchtgeschmack ☺

Cheers,
Igor

PS.: This is a second improved attempt to send this patch. The email
I sent last week has been marked as SPAM (see /n/lists/9front/1617575697.00).

Re: [9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)

[ori]

Quoth igor@9lab.org:
> Below is a patch (inline) that fixes two variants of a suicide (use
> after free, double free) in Mail/mbox.c on latest 9front 'Mit
> Fruchtgeschmack' (explanation with reproducible test instructions
> below) when messages are deleted/moved in bulk:
> 
> <patch>
> diff -r 4dfbef4fa4ac sys/src/cmd/upas/Mail/mbox.c
> --- a/sys/src/cmd/upas/Mail/mbox.c	Sat Apr 03 19:32:47 2021 +0200
> +++ b/sys/src/cmd/upas/Mail/mbox.c	Fri Apr 09 02:54:10 2021 +0200
> @@ -675,10 +675,11 @@
>  static void
>  mbflush(char **, int)
>  {
> -	int i, j, ln, fd;
> +	int i, j, ln, fd, nmesg;
>  	char *path;
>  	Mesg *m, *p;
>  
> +	nmesg = mbox.nmesg;
>  	path = estrjoin(maildir, "/ctl", nil);
>  	fd = open(path, OWRITE);
>  	free(path);
> @@ -708,11 +709,13 @@
>  			mbredraw(m->child[j], 1, 1);
>   	}
>  
> -	for(i = 0, j = 0; i < mbox.nmesg; i++){
> +	for(i = 0, j = 0; i < nmesg; i++){
>  		m = mbox.mesg[i];
> -		if((m->state & Szap) != 0)
> +		if((m->state & Szap) != 0){
>  			mesgfree(m);
> -		else
> +			mbox.mesg[i] = nil;
> +			mbox.nmesg--;
> +		}else
>  			mbox.mesg[j++] = m;
>  	}

I think I prefer this patch:

diff -r 503c5ef2d2b5 sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 11 20:20:41 2021 +0200
+++ b/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 11 19:58:55 2021 -0700
@@ -715,6 +715,7 @@
 		else
 			mbox.mesg[j++] = m;
 	}
+	mbox.nmesg = j;
 		
 	close(fd);
 	fprint(mbox.ctl, "clean\n");

Re: [9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)

[igor]

Quoth ori@eigenstate.org:
> Quoth igor@9lab.org:
> > Below is a patch (inline) that fixes two variants of a suicide (use
> > after free, double free) in Mail/mbox.c on latest 9front 'Mit
> > Fruchtgeschmack' (explanation with reproducible test instructions
> > below) when messages are deleted/moved in bulk:
> > 
> > <patch>
> > diff -r 4dfbef4fa4ac sys/src/cmd/upas/Mail/mbox.c
> > --- a/sys/src/cmd/upas/Mail/mbox.c	Sat Apr 03 19:32:47 2021 +0200
> > +++ b/sys/src/cmd/upas/Mail/mbox.c	Fri Apr 09 02:54:10 2021 +0200
> > @@ -675,10 +675,11 @@
> >  static void
> >  mbflush(char **, int)
> >  {
> > -	int i, j, ln, fd;
> > +	int i, j, ln, fd, nmesg;
> >  	char *path;
> >  	Mesg *m, *p;
> >  
> > +	nmesg = mbox.nmesg;
> >  	path = estrjoin(maildir, "/ctl", nil);
> >  	fd = open(path, OWRITE);
> >  	free(path);
> > @@ -708,11 +709,13 @@
> >  			mbredraw(m->child[j], 1, 1);
> >   	}
> >  
> > -	for(i = 0, j = 0; i < mbox.nmesg; i++){
> > +	for(i = 0, j = 0; i < nmesg; i++){
> >  		m = mbox.mesg[i];
> > -		if((m->state & Szap) != 0)
> > +		if((m->state & Szap) != 0){
> >  			mesgfree(m);
> > -		else
> > +			mbox.mesg[i] = nil;
> > +			mbox.nmesg--;
> > +		}else
> >  			mbox.mesg[j++] = m;
> >  	}
> 
> I think I prefer this patch:
>
> diff -r 503c5ef2d2b5 sys/src/cmd/upas/Mail/mbox.c
> --- a/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 11 20:20:41 2021 +0200
> +++ b/sys/src/cmd/upas/Mail/mbox.c	Sun Apr 11 19:58:55 2021 -0700
> @@ -715,6 +715,7 @@
>  		else
>  			mbox.mesg[j++] = m;
>  	}
> +	mbox.nmesg = j;
>  		
>  	close(fd);
>  	fprint(mbox.ctl, "clean\n");
> 

I agree, this is simpler.

Can we set the slot of the deleted message to nil as well to avoid
keeping around pointers to already freed memory?

The following expresses this in a patch:

<snip>
diff -r 4dfbef4fa4ac sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Sat Apr 03 19:32:47 2021 +0200
+++ b/sys/src/cmd/upas/Mail/mbox.c	Mon Apr 12 14:35:54 2021 +0200
@@ -710,12 +710,14 @@
 
 	for(i = 0, j = 0; i < mbox.nmesg; i++){
 		m = mbox.mesg[i];
-		if((m->state & Szap) != 0)
+		if((m->state & Szap) != 0){
 			mesgfree(m);
-		else
+			mbox.mesg[i] = nil;
+		}else
 			mbox.mesg[j++] = m;
 	}
-		
+	mbox.nmesg = j;
+
 	close(fd);
 	fprint(mbox.ctl, "clean\n");
 }
</snip>

This is an optional request, if you disagree with it I am fine
with your proposal.
 

Re: [9front] upas/Mail: fix multiple suicides (use after free, double free) in mbox.c (patch)

[cinap_lenrek]

thats idiotic. if you only zero the entries that you free,
you can still have valid pointers after index mbox.nmesg.

if you want really zero all unused entries, you can do it
unconditionally after reading, or just add a second loop
like:

	...
}

mbox.nmesg = j;
while(j < i) mbox.mesg[j++] = nil;

// or use memset?
// memset(&mbox.mesg[j], 0, (i-j)*sizeof(mbox.mesg[0]));

but really, i do not think it is neccesary... tho it can
help when trying to find memory leaks and avoid these
false references.

--
cinap