commit 878b30c0bc1446ba933dc4539438512766183500
from: Günther Noack <guenther@unix-ag.uni-kl.de>
via: Dan Cross <crossd@gmail.com>
date: Mon Apr 05 09:19:49 2021 UTC

fspread: fix buffer overflow

Without this fix, fspread is trusting the server to return as much
data as requested, or less. If a server responds with more data
though, fspread writes beyond the bounds of the buffer to fill, which
is passed in by the caller. It depends on the caller of fspread()
where that buffer is, so there are various possible attack vectors.

In the Plan9 kernel, I found this implemented in devmnt.c, where
overly large responses are truncated to the size requested before
copying, so I assume that this strategy works here too.

This also affects fsread() and fsreadn(), which are based on
fspread().

commit - 88a87fadae6629932d9c160f53ad5d79775f8f94
commit + 878b30c0bc1446ba933dc4539438512766183500
blob - ea94e9aa406e1798a7f760edd51b4386e499ca9c
blob + aaf843262640c26208373bcb2b1dffdc421002e4
--- src/lib9pclient/read.c
+++ src/lib9pclient/read.c
@@ -13,6 +13,7 @@ fspread(CFid *fid, void *buf, long n, vlong offset)
 	Fcall tx, rx;
 	void *freep;
 	uint msize;
+	long nr;
 
 	msize = fid->fs->msize - IOHDRSZ;
 	if(n > msize)
@@ -34,17 +35,21 @@ fspread(CFid *fid, void *buf, long n, vlong offset)
 		free(freep);
 		return -1;
 	}
-	if(rx.count){
-		memmove(buf, rx.data, rx.count);
+	nr = rx.count;
+	if(nr > n)
+		nr = n;
+
+	if(nr){
+		memmove(buf, rx.data, nr);
 		if(offset == -1){
 			qlock(&fid->lk);
-			fid->offset += rx.count;
+			fid->offset += nr;
 			qunlock(&fid->lk);
 		}
 	}
 	free(freep);
 
-	return rx.count;
+	return nr;
 }
 
 long