[kbd] [PATCH] Validate psfu headers to avoid integer overflows.

[kbd] [PATCH] Validate psfu headers to avoid integer overflows.

[Tobias Stoeckmann]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

The psfu parser does not properly validate parsed values:

* unsigned int values are casted to signed int values when
  parameters are supplied, therefore they must be checked against
  INT_MAX (local size_t variables are used)
* fontwidth must not be larger than INT_MAX - 7, otherwise later
  alignment codes would overflow, e.g. (fontwidth + 7) / 8
* "ftoffset + fontlen * charsize" is prone to overflow, make sure
  that it does not; later on it will be checked against file size
* when parsing multiple files, make sure that the sum of all
  fonts won't overflow

---
Attached are two files which will crash the current code:

$ setfont setfont-fpe.psfu # font width too large
Floating point exception
$ psfxtable -i psfxtable-segfault.psfu # on 32 bit archs
Segmentation fault

Maybe there are more ways to trigger overflows, which makes it a
good target for fuzzing.
---
 src/psffontop.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/psffontop.c b/src/psffontop.c
index 9d7ee54..e86d3cd 100644
--- a/src/psffontop.c
+++ b/src/psffontop.c
@@ -2,6 +2,7 @@
  * psffontop.c - aeb@cwi.nl, 990921
  */
 
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -275,6 +276,27 @@ readpsffont(FILE *fontf, char **allbufp, int *allszp,
 		fprintf(stderr, u, progname);
 		exit(EX_DATAERR);
 	}
+	if (INT_MAX - 7 < fontwidth) {
+		char *u = _("%s: Input file: font width too large\n");
+		fprintf(stderr, u, progname);
+		exit(EX_DATAERR);
+	}
+
+	/* validate header to avoid integer overflows */
+	if ((size_t)(INT_MAX - fontpos0) < fontlen ||
+	    INT_MAX / sizeof(struct unicode_list) < fontpos0 + fontlen ||
+	    INT_MAX / charsize < fontlen) {
+		char *u = _("%s: too many glyphs to load\n");
+		fprintf(stderr, u, progname);
+		exit(EX_DATAERR);
+	}
+	if (ftoffset > inputbuflth ||
+	    INT_MAX - ftoffset < fontlen * charsize) {
+		char *u = _("%s: Input file: bad offset\n");
+		fprintf(stderr, u, progname);
+		exit(EX_DATAERR);
+	}
+
 	i = ftoffset + fontlen * charsize;
 	if (i > inputlth || (!hastable && i != inputlth)) {
 		char *u = _("%s: Input file: bad input length (%d)\n");
-- 
2.5.0


[-- Attachment #2: setfont-fpe.psfu --]
[-- Type: application/octet-stream, Size: 32 bytes --]

[-- Attachment #3: psfxtable-segfault.psfu --]
[-- Type: application/octet-stream, Size: 39 bytes --]

[kbd] [PATCH] Validate psfu headers to avoid integer overflows.

[Tobias Stoeckmann]

[-- Attachment #1: Type: text/plain, Size: 2289 bytes --]

The psfu parser does not properly validate parsed values:

* unsigned int values are casted to signed int values when
  parameters are supplied, therefore they must be checked against
  INT_MAX (local size_t variables are used)
* fontwidth must not be larger than INT_MAX - 7, otherwise later
  alignment codes would overflow, e.g. (fontwidth + 7) / 8
* "ftoffset + fontlen * charsize" is prone to overflow, make sure
  that it does not; later on it will be checked against file size
* when parsing multiple files, make sure that the sum of all
  fonts won't overflow

---
I've sent this mail in August 2015 already. Based on the upcoming
release, it might be a good idea to re-evaluate it.

Attached are two files which will crash the current code:

$ setfont setfont-fpe.psfu # font width too large
Floating point exception
$ psfxtable -i psfxtable-segfault.psfu # on 32 bit archs
Segmentation fault

Maybe there are more ways to trigger overflows, which makes it a
good target for fuzzing.

---
 src/psffontop.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/psffontop.c b/src/psffontop.c
index 9d7ee54..e86d3cd 100644
--- a/src/psffontop.c
+++ b/src/psffontop.c
@@ -2,6 +2,7 @@
  * psffontop.c - aeb@cwi.nl, 990921
  */
 
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -275,6 +276,27 @@ readpsffont(FILE *fontf, char **allbufp, int *allszp,
 		fprintf(stderr, u, progname);
 		exit(EX_DATAERR);
 	}
+	if (INT_MAX - 7 < fontwidth) {
+		char *u = _("%s: Input file: font width too large\n");
+		fprintf(stderr, u, progname);
+		exit(EX_DATAERR);
+	}
+
+	/* validate header to avoid integer overflows */
+	if ((size_t)(INT_MAX - fontpos0) < fontlen ||
+	    INT_MAX / sizeof(struct unicode_list) < fontpos0 + fontlen ||
+	    INT_MAX / charsize < fontlen) {
+		char *u = _("%s: too many glyphs to load\n");
+		fprintf(stderr, u, progname);
+		exit(EX_DATAERR);
+	}
+	if (ftoffset > inputbuflth ||
+	    INT_MAX - ftoffset < fontlen * charsize) {
+		char *u = _("%s: Input file: bad offset\n");
+		fprintf(stderr, u, progname);
+		exit(EX_DATAERR);
+	}
+
 	i = ftoffset + fontlen * charsize;
 	if (i > inputlth || (!hastable && i != inputlth)) {
 		char *u = _("%s: Input file: bad input length (%d)\n");
-- 
2.10.2


[-- Attachment #2: psfxtable-segfault.psfu --]
[-- Type: application/octet-stream, Size: 39 bytes --]

[-- Attachment #3: setfont-fpe.psfu --]
[-- Type: application/octet-stream, Size: 32 bytes --]

Re: [kbd] [PATCH] Validate psfu headers to avoid integer overflows.

[Alexey Gladkov]

On Sun, Nov 20, 2016 at 06:15:43PM +0100, Tobias Stoeckmann wrote:
> The psfu parser does not properly validate parsed values:
> 
> * unsigned int values are casted to signed int values when
>   parameters are supplied, therefore they must be checked against
>   INT_MAX (local size_t variables are used)
> * fontwidth must not be larger than INT_MAX - 7, otherwise later
>   alignment codes would overflow, e.g. (fontwidth + 7) / 8
> * "ftoffset + fontlen * charsize" is prone to overflow, make sure
>   that it does not; later on it will be checked against file size
> * when parsing multiple files, make sure that the sum of all
>   fonts won't overflow

I like the idea, but I don't like this patch. I consider it a bad idea to
collect all the checks in one place. Most of them looks like black magic
if you don't know the rest of source code.

Right now Oleg is developing the implementation of the library which will
replace this buggy code. I hope this library will be ready for next
release (not upcoming release).

> ---
> I've sent this mail in August 2015 already. Based on the upcoming
> release, it might be a good idea to re-evaluate it.

I guess I lost it. Sorry.

> Attached are two files which will crash the current code:
> 
> $ setfont setfont-fpe.psfu # font width too large
> Floating point exception
> $ psfxtable -i psfxtable-segfault.psfu # on 32 bit archs
> Segmentation fault

Thanks for test cases!

-- 
Rgrds, legion