itfspread: fix buffer overflow - plan9port - [fork] Plan 9 from user space Err mx1.adamsgaard.dk 70 hgit clone git://src.adamsgaard.dk/plan9port URL:git://src.adamsgaard.dk/plan9port mx1.adamsgaard.dk 70 1Log /src/plan9port/log.gph mx1.adamsgaard.dk 70 1Files /src/plan9port/files.gph mx1.adamsgaard.dk 70 1Refs /src/plan9port/refs.gph mx1.adamsgaard.dk 70 1README /src/plan9port/file/README.md.gph mx1.adamsgaard.dk 70 1LICENSE /src/plan9port/file/LICENSE.gph mx1.adamsgaard.dk 70 i--- Err mx1.adamsgaard.dk 70 1commit 878b30c0bc1446ba933dc4539438512766183500 /src/plan9port/commit/878b30c0bc1446ba933dc4539438512766183500.gph mx1.adamsgaard.dk 70 1parent 88a87fadae6629932d9c160f53ad5d79775f8f94 /src/plan9port/commit/88a87fadae6629932d9c160f53ad5d79775f8f94.gph mx1.adamsgaard.dk 70 hAuthor: Günther Noack URL:mailto:guenther@unix-ag.uni-kl.de mx1.adamsgaard.dk 70 iDate: Mon, 17 Aug 2020 21:11:56 +0200 Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 ifspread: fix buffer overflow Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 iWithout this fix, fspread is trusting the server to return as much Err mx1.adamsgaard.dk 70 idata as requested, or less. If a server responds with more data Err mx1.adamsgaard.dk 70 itthough, fspread writes beyond the bounds of the buffer to fill, which Err mx1.adamsgaard.dk 70 iis passed in by the caller. It depends on the caller of fspread() Err mx1.adamsgaard.dk 70 iwhere that buffer is, so there are various possible attack vectors. Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 iIn the Plan9 kernel, I found this implemented in devmnt.c, where Err mx1.adamsgaard.dk 70 ioverly large responses are truncated to the size requested before Err mx1.adamsgaard.dk 70 icopying, so I assume that this strategy works here too. Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 iThis also affects fsread() and fsreadn(), which are based on Err mx1.adamsgaard.dk 70 ifspread(). Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 iDiffstat: Err mx1.adamsgaard.dk 70 i M src/lib9pclient/read.c | 13 +++++++++---- Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 i1 file changed, 9 insertions(+), 4 deletions(-) Err mx1.adamsgaard.dk 70 i--- Err mx1.adamsgaard.dk 70 1diff --git a/src/lib9pclient/read.c b/src/lib9pclient/read.c /src/plan9port/file/src/lib9pclient/read.c.gph mx1.adamsgaard.dk 70 it@@ -13,6 +13,7 @@ fspread(CFid *fid, void *buf, long n, vlong offset) Err mx1.adamsgaard.dk 70 i Fcall tx, rx; Err mx1.adamsgaard.dk 70 i void *freep; Err mx1.adamsgaard.dk 70 i uint msize; Err mx1.adamsgaard.dk 70 i+ long nr; Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 i msize = fid->fs->msize - IOHDRSZ; Err mx1.adamsgaard.dk 70 i if(n > msize) Err mx1.adamsgaard.dk 70 it@@ -34,17 +35,21 @@ fspread(CFid *fid, void *buf, long n, vlong offset) Err mx1.adamsgaard.dk 70 i free(freep); Err mx1.adamsgaard.dk 70 i return -1; Err mx1.adamsgaard.dk 70 i } Err mx1.adamsgaard.dk 70 i- if(rx.count){ Err mx1.adamsgaard.dk 70 i- memmove(buf, rx.data, rx.count); Err mx1.adamsgaard.dk 70 i+ nr = rx.count; Err mx1.adamsgaard.dk 70 i+ if(nr > n) Err mx1.adamsgaard.dk 70 i+ nr = n; Err mx1.adamsgaard.dk 70 i+ Err mx1.adamsgaard.dk 70 i+ if(nr){ Err mx1.adamsgaard.dk 70 i+ memmove(buf, rx.data, nr); Err mx1.adamsgaard.dk 70 i if(offset == -1){ Err mx1.adamsgaard.dk 70 i qlock(&fid->lk); Err mx1.adamsgaard.dk 70 i- fid->offset += rx.count; Err mx1.adamsgaard.dk 70 i+ fid->offset += nr; Err mx1.adamsgaard.dk 70 i qunlock(&fid->lk); Err mx1.adamsgaard.dk 70 i } Err mx1.adamsgaard.dk 70 i } Err mx1.adamsgaard.dk 70 i free(freep); Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 i- return rx.count; Err mx1.adamsgaard.dk 70 i+ return nr; Err mx1.adamsgaard.dk 70 i } Err mx1.adamsgaard.dk 70 i Err mx1.adamsgaard.dk 70 i long Err mx1.adamsgaard.dk 70 .