| Summary: | Off-by-one under-read in FontFileMakeDir() | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | Nathan <nathans.spam.and.friends> | ||||||
| Component: | Security | Assignee: | X.Org Security <xorg_security> | ||||||
| Status: | NEW --- | QA Contact: | X.Org Security <xorg_security> | ||||||
| Severity: | normal | ||||||||
| Priority: | medium | ||||||||
| Version: | git | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| i915 platform: | i915 features: | ||||||||
| Attachments: |
|
||||||||
Created attachment 142634 [details] signature.asc On Sun, Nov 25, 2018 at 09:23:07PM +0000, bugzilla-daemon@freedesktop.org wrote: > https://bugs.freedesktop.org/show_bug.cgi?id=108859 > int > BuiltinReadDirectory (const char *directory, FontDirectoryPtr *pdir) > { > FontDirectoryPtr dir; > int i; > > static BuiltinDirPtr saved_builtin_dir; > static BuiltinAliasPtr saved_builtin_alias; > > dir = FontFileMakeDir ("", builtin_dir_count); > > As you can see, we call FontFileMakeDir() with an empty string. Let's see how > FontFileMakeDir() handles an empty string: > > FontDirectoryPtr > FontFileMakeDir(const char *dirName, int size) > { > FontDirectoryPtr dir; > int dirlen; > int needslash = 0; > const char *attrib; > int attriblen; > > #if !defined(WIN32) > attrib = strchr(dirName, ':'); > #else > /* OS/2 uses the colon in the drive letter descriptor, skip this */ > attrib = strchr(dirName+2, ':'); > #endif Does anything prevent hitting the #else branch with an empty input string? Thanks Good question! Answer is no, which is also another bug for WIN32 platforms ;)
We should add something akin to:
...
const char *attrib = NULL;
...
#if !defined(WIN32)
attrib = strchr(dirName, ':');
#else
/* OS/2 uses the colon in the drive letter descriptor, skip this */
if (strlen(dirName) > 2)
attrib = strchr(dirName+2, ':');
#endif
Created attachment 142635 [details] [review] Proposed patch Here is the proposed patch for the off-by-one and the strchr() pointer arithmetic logic bug. Comment on attachment 142635 [details] [review] Proposed patch >Date: Sun, 25 Nov 2018 13:32:02 -0800 >Subject: [PATCH] libXfont: Fix ASAN errors in FontFileMakeDir() > >FontFileMakeDir() will automatically read one byte before >the dirName buffer if an empty string is passed in, which >one of its callers automatically does. Add a check to make >sure we don't subtrct one from the array index unless it is >non-zero. In addition, on WIN32 platforms, we will read >past the buffer via an unchecked strchr() call. Add a check >to ensure the pointer arithmetic does not reach beyond the >buffer. >--- > src/fontfile/fontdir.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > >diff --git a/src/fontfile/fontdir.c b/src/fontfile/fontdir.c >index 7271603..ab87068 100644 >--- a/src/fontfile/fontdir.c >+++ b/src/fontfile/fontdir.c >@@ -105,28 +105,25 @@ FontDirectoryPtr > FontFileMakeDir(const char *dirName, int size) > { > FontDirectoryPtr dir; >- int dirlen; >+ int dirlen = (int)strlen(dirName); > int needslash = 0; >- const char *attrib; >+ const char *attrib = NULL; > int attriblen; > > #if !defined(WIN32) > attrib = strchr(dirName, ':'); > #else > /* OS/2 uses the colon in the drive letter descriptor, skip this */ >- attrib = strchr(dirName+2, ':'); >+ if (dirlen > 2) >+ attrib = strchr(dirName+2, ':'); > #endif > if (attrib) { > dirlen = attrib - dirName; > attriblen = strlen(attrib); > } else { >- dirlen = strlen(dirName); > attriblen = 0; > } >- if (dirName[dirlen - 1] != '/') >-#ifdef NCD >- if (dirlen) /* leave out slash for builtins */ >-#endif >+ if (dirlen && dirName[dirlen - 1] != '/') > needslash = 1; > dir = malloc(sizeof *dir + dirlen + needslash + 1 + > (attriblen ? attriblen + 1 : 0)); >-- >2.17.1 > |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
While running libXfont under ASAN, I encountered the following error whenever X server is starting up: 2018-11-25_12:57:42.22488-0800 ================================================================= 2018-11-25_12:57:42.22503-0800 ==20811==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f04f177e6df at pc 0x7f04f172ced9 bp 0x7ffd99d47d40 sp 0x7ffd99d47d38 2018-11-25_12:57:42.22509-0800 READ of size 1 at 0x7f04f177e6df thread T0 2018-11-25_12:57:42.24335-0800 #0 0x7f04f172ced8 in FontFileMakeDir (/usr/lib64/libXfont.so.1+0x18ed8) 2018-11-25_12:57:42.24342-0800 #1 0x7f04f175dfeb in BuiltinReadDirectory (/usr/lib64/libXfont.so.1+0x49feb) 2018-11-25_12:57:42.24344-0800 #2 0x7f04f175ea1c (/usr/lib64/libXfont.so.1+0x4aa1c) 2018-11-25_12:57:42.24348-0800 #3 0x46c1e3 (/usr/bin/Xorg+0x46c1e3) 2018-11-25_12:57:42.24350-0800 #4 0x47167f in SetDefaultFontPath (/usr/bin/Xorg+0x47167f) 2018-11-25_12:57:42.24352-0800 #5 0x472c72 (/usr/bin/Xorg+0x472c72) 2018-11-25_12:57:42.24353-0800 #6 0x7f04efc3e78f in __libc_start_main (/lib64/libc.so.6+0x2078f) 2018-11-25_12:57:42.24430-0800 #7 0x42e918 in _start (/usr/bin/Xorg+0x42e918) 2018-11-25_12:57:42.24436-0800 2018-11-25_12:57:42.24501-0800 0x7f04f177e6df is located 32 bytes to the right of global variable '*.LC10' defined in 'pcfwrite.c' (0x7f04f177e6a0) of size 31 2018-11-25_12:57:42.24509-0800 '*.LC10' is ascii string 'can't go backwards... %d > %d 2018-11-25_12:57:42.24510-0800 ' 2018-11-25_12:57:42.24512-0800 0x7f04f177e6df is located 1 bytes to the left of global variable '*.LC0' defined in 'dir.c' (0x7f04f177e6e0) of size 1 2018-11-25_12:57:42.24514-0800 '*.LC0' is ascii string '' 2018-11-25_12:57:42.24517-0800 SUMMARY: AddressSanitizer: global-buffer-overflow ??:0 FontFileMakeDir 2018-11-25_12:57:42.24525-0800 Shadow bytes around the buggy address: 2018-11-25_12:57:42.24526-0800 0x0fe11e2e7c80: f9 f9 f9 f9 00 00 00 00 00 07 f9 f9 f9 f9 f9 f9 2018-11-25_12:57:42.24528-0800 0x0fe11e2e7c90: 00 00 00 00 00 00 02 f9 f9 f9 f9 f9 00 00 00 00 2018-11-25_12:57:42.24530-0800 0x0fe11e2e7ca0: 00 00 06 f9 f9 f9 f9 f9 00 00 00 00 00 00 03 f9 2018-11-25_12:57:42.24533-0800 0x0fe11e2e7cb0: f9 f9 f9 f9 00 00 00 00 00 00 02 f9 f9 f9 f9 f9 2018-11-25_12:57:42.24535-0800 0x0fe11e2e7cc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 06 f9 2018-11-25_12:57:42.24537-0800 =>0x0fe11e2e7cd0: f9 f9 f9 f9 00 00 00 07 f9 f9 f9[f9]01 f9 f9 f9 2018-11-25_12:57:42.24539-0800 0x0fe11e2e7ce0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 04 f9 f9 f9 2018-11-25_12:57:42.24541-0800 0x0fe11e2e7cf0: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9 00 05 f9 f9 2018-11-25_12:57:42.24543-0800 0x0fe11e2e7d00: f9 f9 f9 f9 00 07 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 2018-11-25_12:57:42.24545-0800 0x0fe11e2e7d10: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00 2018-11-25_12:57:42.24547-0800 0x0fe11e2e7d20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2018-11-25_12:57:42.24551-0800 Shadow byte legend (one shadow byte represents 8 application bytes): 2018-11-25_12:57:42.24553-0800 Addressable: 00 2018-11-25_12:57:42.24561-0800 Partially addressable: 01 02 03 04 05 06 07 2018-11-25_12:57:42.24563-0800 Heap left redzone: fa 2018-11-25_12:57:42.24565-0800 Heap right redzone: fb 2018-11-25_12:57:42.24567-0800 Freed heap region: fd 2018-11-25_12:57:42.24569-0800 Stack left redzone: f1 2018-11-25_12:57:42.24573-0800 Stack mid redzone: f2 2018-11-25_12:57:42.24575-0800 Stack right redzone: f3 2018-11-25_12:57:42.24577-0800 Stack partial redzone: f4 2018-11-25_12:57:42.24579-0800 Stack after return: f5 2018-11-25_12:57:42.24581-0800 Stack use after scope: f8 2018-11-25_12:57:42.24584-0800 Global redzone: f9 2018-11-25_12:57:42.24589-0800 Global init order: f6 2018-11-25_12:57:42.24591-0800 Poisoned by user: f7 2018-11-25_12:57:42.24594-0800 Container overflow: fc 2018-11-25_12:57:42.24596-0800 Array cookie: ac 2018-11-25_12:57:42.24598-0800 Intra object redzone: bb 2018-11-25_12:57:42.24600-0800 ASan internal: fe 2018-11-25_12:57:42.24602-0800 ==20811==ABORTING The root cause is easy to spot. As the backtrace says, we first call into BuiltinReadDirectory(): int BuiltinReadDirectory (const char *directory, FontDirectoryPtr *pdir) { FontDirectoryPtr dir; int i; static BuiltinDirPtr saved_builtin_dir; static BuiltinAliasPtr saved_builtin_alias; dir = FontFileMakeDir ("", builtin_dir_count); As you can see, we call FontFileMakeDir() with an empty string. Let's see how FontFileMakeDir() handles an empty string: FontDirectoryPtr FontFileMakeDir(const char *dirName, int size) { FontDirectoryPtr dir; int dirlen; int needslash = 0; const char *attrib; int attriblen; #if !defined(WIN32) attrib = strchr(dirName, ':'); #else /* OS/2 uses the colon in the drive letter descriptor, skip this */ attrib = strchr(dirName+2, ':'); #endif if (attrib) { dirlen = attrib - dirName; attriblen = strlen(attrib); } else { dirlen = strlen(dirName); attriblen = 0; } if (dirName[dirlen - 1] != '/') If the path doesn't have a ':' in it, we calculate dirlen as the length of the string. We take that length, subtract one, and look for a slash. So, if the length of the string is zero (which it is in this case), you have a OOB under-read. Same issue would happen if the string started with a ':'. Fix is trivial, change the check to: if (dirlen && dirName[dirlen - 1] != '/')