ref: a6af3069b32670ef0902625424070087a06f8c1b
parent: 3092ac09e43d2eaeb33be3ec37dba2e8477418e6
author: Igor Böhm <igor@9lab.org>
date: Mon Oct 10 12:28:44 EDT 2022
upas/fs/imap.c: additional sanity checking during `Expunge` to avoid suicide
We have to ensure the size we compute for memmove(...) in `Expunge` is
properly bounded. For certain combinations of inputs we compute an
illegal size causing a suicide:
upas/fs: imap: fetchrsp: fetchrsp: bad idx 7
upas/fs: user: igor; note: sys: trap: fault read addr=0x0 pc=0x214f92
fs 531: suicide: sys: trap: fault read addr=0x0 pc=0x214f92
Stack trace including state of data-structures:
term% acid -l /sys/src/cmd/upas/fs/imap.acid 531
/proc/531/text:amd64 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/amd64
acid: lstk()
memmove(p2=0x8e5928,n=0xffffffffffffffe8)+0x42 /sys/src/libc/amd64/memmove.s:36
imap4resp0(imap=0x423c80,mb=0x0,m=0x0)+0x448 /sys/src/cmd/upas/fs/imap.c:522
unexp=0x58b00000000
p=0x425d17
ep=0x425d17
line=0x425d0b
n=0x425d100000058c
verb=0x425d10
op=0x2e6d6f63
imap4resp()+0x1b /sys/src/cmd/upas/fs/imap.c:556
imap4read(imap=0x423c80,mb=0x4239e0)+0x30 /sys/src/cmd/upas/fs/imap.c:928
s=0x425d29
f=0x425d29
n=0x58b0000058c
ll=0x5008a34b0
i=0x425d290000058b
m=0x171bd1f85150e2a6
imap4sync(mb=0x4239e0)+0x62 /sys/src/cmd/upas/fs/imap.c:1059
imap=0x423c80
err=0x0
syncmbox(mb=0x4239e0,doplumb=0x70616d6900000001)+0x5c /sys/src/cmd/upas/fs/mbox.c:76
a=0x58f
n=0x400bc800000000
d=0x0
y=0x0
next=0x171bd206499a9366
m=0x66939a4906d21b17
reader()+0xde /sys/src/cmd/upas/fs/fs.c:1488
t=0x204ca163404153
mb=0x692e6d68656f622f
io()+0x212 /sys/src/cmd/upas/fs/fs.c:1416
n=0x0
main(argc=0x0,argv=0x7fffffffef58)+0x31e /sys/src/cmd/upas/fs/fs.c:353
mboxfile=0x7fffffffef7a
nodflt=0x300000000
srvpost=0x0
v=0x7fffffffef38
_argc=0x6d
_args=0x40d7bd
p=0x400000003
maildir=0x0
mbox=0x0
srvfile=0x0
_main+0x40 /sys/src/libc/amd64/main9.s:15
acid: Imap(0x423c80)
mbox 0x0000000000428b90
freep 0x0000000000423c20
host 0x0000000000423c27
user 0x0000000000423c36
refreshtime 60
cap 0x00
flags 0x05
tag 872
validity 605040277
newvalidity 605040277
nmsg 1419
size 0
f 0x00000000008dd408
nuid 1420
muid 1420
Biobuf bin {
Biobufhdr {
icount -28
ocount 0
rdline 16
runesize 0
state 1
fid 10
flag 0
offset 280380
bsize 8192
bbuf 0x0000000000423d35
ebuf 0x0000000000425d35
gbuf 0x0000000000425cd1
errorf 0x0000000000000000
iof 0x0000000000228d17
aux 0x0000000000000000
}
b end+0x388
}
Biobuf bout {
Biobufhdr {
icount 0
ocount -8192
rdline 0
runesize 0
state 2
fid 10
flag 0
offset 33362
bsize 8192
bbuf 0x0000000000425d9d
ebuf 0x0000000000427d9d
gbuf 0x0000000000427d9d
errorf 0x0000000000000000
iof 0x0000000000228d3a
aux 0x0000000000000000
}
b 0x425d98
}
binit 1
fd 10
The root cause is an integer underflow where we subtract -1 from 0
using an unsigned type.
The above acid trace shows the following values for key local variables:
• nmsg ... 1419
• muid ... 1420
• n ... 1420
• idx ... 1419
The key section of code is /sys/src/cmd/upas/fs/imap.c:516,522
case Expunge:
if(n < 1 || n > imap->muid){
snprint(error, sizeof(error), "bad expunge %d (nmsg %d)", n, imap->nuid);
return error;
}
idx = n - 1;
memmove(&imap->f[idx], &imap->f[idx + 1], (imap->nmsg - idx - 1)*sizeof(imap->f[0]));
Plugging the above values into the call to memmove(...) demonstrates the
issue:
memmove(&imap->f[1419], &imap->f[1419 + 1], (1419 - 1419 - 1)*sizeof(imap->f[0]));
^^^^^^^^^^^^^^^
The third argument of memmove(...) is an unsigned size type causing
the size to be a value that is way too large (the stack trace shows
the size to be the unsigned value 0xffffffffffffffe8).
The `Expunge` case can be fixed by amending the bounds checking
condition before the memmove(...):
case Expunge:
if(n < 1 || n > imap->muid || (n - 1) >= imap->nmsg){
^^^^^^^^^^^^^^^^^^^^^
snprint(error, sizeof(error), "bad expunge %d (nmsg %d)", n, imap->nuid);
return error;
}
idx = n - 1;
memmove(&imap->f[idx], &imap->f[idx + 1], (imap->nmsg - idx - 1)*sizeof(imap->f[0]));
To add some additional context, this issue has been introduced in
revision:
term% git/export 84c4c81ceecfa8f51949787fc2dbe7b14164a353
Date: Mon, 02 Dec 2019 01:12:19 +0000
Subject: [PATCH] upas/fs imap fixes and improvements
do incremental imap fetches after startup, fixes validity handling,
record flags correctly when we aren't in the process of directly
updating a message, fixes off by one in flag parsing, fixes
mis-indexing messages in sync when we get an unsolicited fetch
response.
...
--- a/sys/src/cmd/upas/fs/imap.c
+++ b/sys/src/cmd/upas/fs/imap.c
@@ -514,7 +514,7 @@
imap->nuid = n;
break;
case Expunge:
- if(n < 1 || n > imap->muid){+ if(n < 1 || n > imap->muid || (n - 1) >= imap->nmsg){snprint(error, sizeof(error), "bad expunge %d (nmsg %d)", n, imap->nuid);
return error;
}