Created attachment 21847 [details] [review] Proposed method for fixing overflow situation Forwarding this bug report from a Ubuntu user: https://bugs.edge.launchpad.net/ubuntu/+source/x11-xkb-utils/+bug/309013 [Problem] Ubuntu is now using the fortify-source gcc compilation options to help find potential buffer overflows, and found the following in the xkbcomp code. #0 0x00007f6c4a726f85 in raise () from /lib/libc.so.6 No symbol table info available. #1 0x00007f6c4a728af3 in abort () from /lib/libc.so.6 No symbol table info available. #2 0x00007f6c4a766218 in ?? () from /lib/libc.so.6 No symbol table info available. #3 0x00007f6c4a7f3307 in __fortify_fail () from /lib/libc.so.6 No symbol table info available. #4 0x00007f6c4a7f11b0 in __chk_fail () from /lib/libc.so.6 No symbol table info available. #5 0x00007f6c4aa6cad5 in WriteCHdrKeyTypes (file=0x19f1410, dpy=0x0, xkb=0x19e58d0) at /usr/include/bits/string3.h:106 i = 22 n = 3 type = (XkbKeyTypePtr) 0x19e5d70 prefix = "SEPARATE_CAPS_AND_SHIFT_ALPHABET" #6 0x00007f6c4aa6dbe2 in WriteCHdrKeymap (file=0x19f1410, result=<value optimized out>) at ../../src/cout.c:335 ok = <value optimized out> xkb = (XkbDescPtr) 0x19e58d0 #7 0x00007f6c4aa6ab9e in XkbWriteCFile (out=0x19f1410, name=0x19f165a "", result=0x7fff531b15d0) at ../../src/cout.c:1027 tmp = 0x19f165a "" hdrdef = <value optimized out> ok = <value optimized out> xkb = (XkbDescPtr) 0x19e58d0 func = (int (*)(FILE *, XkbFileInfo *)) 0x7f6c4aa6db30 <WriteCHdrKeymap> Warning: Could not load keyboard geometry for :0 BadAlloc (insufficient resources for operation) Resulting keymap file will not describe geometry *** buffer overflow detected ***: xkbcomp terminated ======= Backtrace: ========= /lib/libc.so.6(__fortify_fail+0x37)[0x7f3bd4803307] /lib/libc.so.6[0x7f3bd48011b0] /usr/lib/libxkbfile.so.1[0x7f3bd4a7cad5] /usr/lib/libxkbfile.so.1[0x7f3bd4a7dbe2] /usr/lib/libxkbfile.so.1(XkbWriteCFile+0x26e)[0x7f3bd4a7ab9e] xkbcomp[0x41d566] /lib/libc.so.6(__libc_start_main+0xe6)[0x7f3bd4722586] xkbcomp[0x4027f9] ======= Memory map: ======== 00400000-0042e000 r-xp 00000000 08:01 8233554 /usr/bin/xkbcomp 0062e000-00630000 rw-p 0002e000 08:01 8233554 /usr/bin/xkbcomp 00630000-00631000 rw-p 00630000 00:00 0 00a54000-00a9b000 rw-p 00a54000 00:00 0 [heap] 7f3bd3ac2000-7f3bd3ad8000 r-xp 00000000 08:01 4907635 /lib/libgcc_s.so.1 7f3bd3ad8000-7f3bd3cd8000 ---p 00016000 08:01 4907635 /lib/libgcc_s.so.1 7f3bd3cd8000-7f3bd3cd9000 r--p 00016000 08:01 4907635 /lib/libgcc_s.so.1 7f3bd3cd9000-7f3bd3cda000 rw-p 00017000 08:01 4907635 /lib/libgcc_s.so.1 7f3bd3cda000-7f3bd3cdf000 r-xp 00000000 08:01 8235629 /usr/lib/libXdmcp.so.6.0.0 7f3bd3cdf000-7f3bd3ede000 ---p 00005000 08:01 8235629 /usr/lib/libXdmcp.so.6.0.0 7f3bd3ede000-7f3bd3edf000 rw-p 00004000 08:01 8235629 /usr/lib/libXdmcp.so.6.0.0 7f3bd3edf000-7f3bd3ee1000 r-xp 00000000 08:01 8233819 /usr/lib/libXau.so.6.0.0 7f3bd3ee1000-7f3bd40e0000 ---p 00002000 08:01 8233819 /usr/lib/libXau.so.6.0.0 7f3bd40e0000-7f3bd40e1000 r--p 00001000 08:01 8233819 /usr/lib/libXau.so.6.0.0 7f3bd40e1000-7f3bd40e2000 rw-p 00002000 08:01 8233819 /usr/lib/libXau.so.6.0.0 7f3bd40e2000-7f3bd40e4000 r-xp 00000000 08:01 18729431 /lib/libdl-2.9.so 7f3bd40e4000-7f3bd42e4000 ---p 00002000 08:01 18729431 /lib/libdl-2.9.so 7f3bd42e4000-7f3bd42e5000 r--p 00002000 08:01 18729431 /lib/libdl-2.9.so 7f3bd42e5000-7f3bd42e6000 rw-p 00003000 08:01 18729431 /lib/libdl-2.9.so 7f3bd42e6000-7f3bd4301000 r-xp 00000000 08:01 8235651 /usr/lib/libxcb.so.1.0.0 7f3bd4301000-7f3bd4500000 ---p 0001b000 08:01 8235651 /usr/lib/libxcb.so.1.0.0 7f3bd4500000-7f3bd4501000 r--p 0001a000 08:01 8235651 /usr/lib/libxcb.so.1.0.0 7f3bd4501000-7f3bd4502000 rw-p 0001b000 08:01 8235651 /usr/lib/libxcb.so.1.0.0 7f3bd4502000-7f3bd4503000 r-xp 00000000 08:01 8235655 /usr/lib/libxcb-xlib.so.0.0.0 7f3bd4503000-7f3bd4702000 ---p 00001000 08:01 8235655 /usr/lib/libxcb-xlib.so.0.0.0 7f3bd4702000-7f3bd4703000 r--p 00000000 08:01 8235655 /usr/lib/libxcb-xlib.so.0.0.0 7f3bd4703000-7f3bd4704000 rw-p 00001000 08:01 8235655 /usr/lib/libxcb-xlib.so.0.0.0 7f3bd4704000-7f3bd486c000 r-xp 00000000 08:01 18729428 /lib/libc-2.9.so 7f3bd486c000-7f3bd4a6c000 ---p 00168000 08:01 18729428 /lib/libc-2.9.so 7f3bd4a6c000-7f3bd4a70000 r--p 00168000 08:01 18729428 /lib/libc-2.9.so 7f3bd4a70000-7f3bd4a71000 rw-p 0016c000 08:01 18729428 /lib/libc-2.9.so 7f3bd4a71000-7f3bd4a76000 rw-p 7f3bd4a71000 00:00 0 7f3bd4a76000-7f3bd4a99000 r-xp 00000000 08:01 8236737 /usr/lib/libxkbfile.so.1.0.2 7f3bd4a99000-7f3bd4c98000 ---p 00023000 08:01 8236737 /usr/lib/libxkbfile.so.1.0.2 7f3bd4c98000-7f3bd4c9a000 rw-p 00022000 08:01 8236737 /usr/lib/libxkbfile.so.1.0.2 7f3bd4c9a000-7f3bd4d9d000 r-xp 00000000 08:01 8235085 /usr/lib/libX11.so.6.2.0 7f3bd4d9d000-7f3bd4f9d000 ---p 00103000 08:01 8235085 /usr/lib/libX11.so.6.2.0 7f3bd4f9d000-7f3bd4f9e000 r--p 00103000 08:01 8235085 /usr/lib/libX11.so.6.2.0 7f3bd4f9e000-7f3bd4fa2000 rw-p 00104000 08:01 8235085 /usr/lib/libX11.so.6.2.0 7f3bd4fa2000-7f3bd4fc2000 r-xp 00000000 08:01 18729425 /lib/ld-2.9.so 7f3bd519e000-7f3bd51a2000 rw-p 7f3bd519e000 00:00 0 7f3bd51bd000-7f3bd51c1000 rw-p 7f3bd51bd000 00:00 0 7f3bd51c1000-7f3bd51c2000 r--p 0001f000 08:01 18729425 /lib/ld-2.9.so 7f3bd51c2000-7f3bd51c3000 rw-p 00020000 08:01 18729425 /lib/ld-2.9.so 7fffdd1ae000-7fffdd1c3000 rw-p 7ffffffea000 00:00 0 [stack] 7fffdd1ff000-7fffdd200000 r-xp 7fffdd1ff000 00:00 0 [vdso] ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall] Aborted Stepping through in gdb... 98 return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt, 192 ../../src/cout.c: No such file or directory. in ../../src/cout.c 193 in ../../src/cout.c 194 in ../../src/cout.c 195 in ../../src/cout.c 192 in ../../src/cout.c 195 in ../../src/cout.c 98 return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt, 192 ../../src/cout.c: No such file or directory. in ../../src/cout.c 98 return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt, 192 ../../src/cout.c: No such file or directory. in ../../src/cout.c 202 in ../../src/cout.c 204 in ../../src/cout.c 207 in ../../src/cout.c 98 return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt, 211 ../../src/cout.c: No such file or directory. in ../../src/cout.c 184 in ../../src/cout.c 185 in ../../src/cout.c 106 return __builtin___strcpy_chk (__dest, __src, __bos (__dest)); 185 ../../src/cout.c: No such file or directory. in ../../src/cout.c 106 return __builtin___strcpy_chk (__dest, __src, __bos (__dest)); (gdb) *** buffer overflow detected ***: /usr/bin/xkbcomp terminated So seems to be src/cout.c line 185: strcpy(prefix,XkbAtomText(dpy,type->name,XkbCFile)); Proposed patch from Kees Cook attached. I notice a number of other strpcy() usage in this code associated with static length string buffers, that perhaps would be worth further review to see if they need similar fixes. [Original Report] I'm using the NEO keyboard layout (from https://svn.neo-layout.org/linux/X/de) in Ubuntu 8.10. This is a non-standard keyboard layout, I wanted to create a keymap for rdesktop. A file is generated, but xkbcomp seems to have a problem, this is the error message I see: $ xkbcomp -C :0 *** buffer overflow detected ***: xkbcomp terminated ======= Backtrace: ========= /lib/tls/i686/cmov/libc.so.6(__fortify_fail+0x48)[0xb7e1c558] /lib/tls/i686/cmov/libc.so.6[0xb7e1a680] /lib/tls/i686/cmov/libc.so.6(__strcpy_chk+0x44)[0xb7e19944] /usr/lib/libxkbfile.so.1[0xb7e86c8f] /usr/lib/libxkbfile.so.1[0xb7e883ab] /usr/lib/libxkbfile.so.1(XkbWriteCFile+0x2ee)[0xb7e8431e] xkbcomp[0x8065d53] /lib/tls/i686/cmov/libc.so.6(__libc_start_main+0xe5)[0xb7d38685] xkbcomp[0x8049d41] ======= Memory map: ======== 08048000-08073000 r-xp 00000000 08:07 17196718 /usr/bin/xkbcomp 08073000-08075000 rw-p 0002b000 08:07 17196718 /usr/bin/xkbcomp 0996e000-0998f000 rw-p 0996e000 00:00 0 [heap] b7cda000-b7ce7000 r-xp 00000000 08:07 50332952 /lib/libgcc_s.so.1 b7ce7000-b7ce8000 r--p 0000c000 08:07 50332952 /lib/libgcc_s.so.1 b7ce8000-b7ce9000 rw-p 0000d000 08:07 50332952 /lib/libgcc_s.so.1 b7cf8000-b7cf9000 rw-p b7cf8000 00:00 0 b7cf9000-b7cfd000 r-xp 00000000 08:07 172681 /usr/lib/libXdmcp.so.6.0.0 b7cfd000-b7cfe000 rw-p 00003000 08:07 172681 /usr/lib/libXdmcp.so.6.0.0 b7cfe000-b7cff000 rw-p b7cfe000 00:00 0 b7cff000-b7d01000 r-xp 00000000 08:07 172670 /usr/lib/libXau.so.6.0.0 b7d01000-b7d02000 rw-p 00001000 08:07 172670 /usr/lib/libXau.so.6.0.0 b7d02000-b7d04000 r-xp 00000000 08:07 16857011 /lib/tls/i686/cmov/libdl-2.8.90.so b7d04000-b7d05000 r--p 00001000 08:07 16857011 /lib/tls/i686/cmov/libdl-2.8.90.so b7d05000-b7d06000 rw-p 00002000 08:07 16857011 /lib/tls/i686/cmov/libdl-2.8.90.so b7d06000-b7d1d000 r-xp 00000000 08:07 736558 /usr/lib/libxcb.so.1.0.0 b7d1d000-b7d1e000 r--p 00016000 08:07 736558 /usr/lib/libxcb.so.1.0.0 b7d1e000-b7d1f000 rw-p 00017000 08:07 736558 /usr/lib/libxcb.so.1.0.0 b7d1f000-b7d20000 r-xp 00000000 08:07 736556 /usr/lib/libxcb-xlib.so.0.0.0 b7d20000-b7d21000 r--p 00000000 08:07 736556 /usr/lib/libxcb-xlib.so.0.0.0 b7d21000-b7d22000 rw-p 00001000 08:07 736556 /usr/lib/libxcb-xlib.so.0.0.0 b7d22000-b7e7a000 r-xp 00000000 08:07 16857005 /lib/tls/i686/cmov/libc-2.8.90.so b7e7a000-b7e7c000 r--p 00158000 08:07 16857005 /lib/tls/i686/cmov/libc-2.8.90.so b7e7c000-b7e7d000 rw-p 0015a000 08:07 16857005 /lib/tls/i686/cmov/libc-2.8.90.so b7e7d000-b7e81000 rw-p b7e7d000 00:00 0 b7e81000-b7ea3000 r-xp 00000000 08:07 736560 /usr/lib/libxkbfile.so.1.0.2 b7ea3000-b7ea5000 rw-p 00021000 08:07 736560 /usr/lib/libxkbfile.so.1.0.2 b7ea5000-b7f90000 r-xp 00000000 08:07 421439 /usr/lib/libX11.so.6.2.0 b7f90000-b7f91000 r--p 000ea000 08:07 421439 /usr/lib/libX11.so.6.2.0 b7f91000-b7f93000 rw-p 000eb000 08:07 421439 /usr/lib/libX11.so.6.2.0 b7f93000-b7f94000 rw-p b7f93000 00:00 0 b7fa2000-b7fa5000 rw-p b7fa2000 00:00 0 b7fa5000-b7fbf000 r-xp 00000000 08:07 50332845 /lib/ld-2.8.90.so b7fbf000-b7fc0000 r-xp b7fbf000 00:00 0 [vdso] b7fc0000-b7fc1000 r--p 0001a000 08:07 50332845 /lib/ld-2.8.90.so b7fc1000-b7fc2000 rw-p 0001b000 08:07 50332845 /lib/ld-2.8.90.so bfdad000-bfdc2000 rw-p bffeb000 00:00 0 [stack] Aborted ------------------------------ // package version of x11-xkb-utils $ apt-cache policy x11-xkb-utils x11-xkb-utils: Installed: 7.4+1 ProblemType: Bug Architecture: i386 DistroRelease: Ubuntu 8.10 NonfreeKernelModules: fglrx Package: x11-xkb-utils 7.4+1 ProcEnviron: LC_PAPER=en_GB.UTF-8 SHELL=/bin/bash PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games LANG=en_US.UTF-8 SourcePackage: x11-xkb-utils Uname: Linux 2.6.27-9-generic i686
Created attachment 21848 [details] [review] handles other crashing strcpy's
patch looks good in principle, minus the problem that asprintf isn't available on at least Solaris. Can you re-do the patch avoiding the use of asprintf please? (and while you're at it, a git-formatted patch would be nice :)
I used asprintf because I have no idea what the maximum length of the returned string from XkbAtomText is. I assume you could fix this issue using a corrected maximum size to the local stack buffer. How long can the result string be expected to be?
> I used asprintf because I have no idea what the maximum length of the returned > string from XkbAtomText is. I assume you could fix this issue using a > corrected maximum size to the local stack buffer. How long can the result > string be expected to be? shouldn't something like -- char* foo = XkbAtomText(..); char* bar = malloc(strlen(foo)); strcpy(bar, foo); -- do the same job as asprintf?
Created attachment 21912 [details] [review] use strdup Argh. Sorry, I've been debugging sprintf issues in other code for so long that I forgot about strdup. Duh. This should be portable.
getting there. instead of perror it's probably better to use _XkbLibError. and exit(1) in a library is a big no-no. Just return 0. (yes, I know, I should have caught that in the first review)
Created attachment 21913 [details] [review] use XkbLibError, don't exit How about this? Looks like WriteTypeInitFunc is void, so there's nothing to do but return after the error report.
> How about this? Looks like WriteTypeInitFunc is void, so there's nothing to do > but return after the error report. I think it' be better to just break out of the loop, since otherwise we leave a dangling { in the output. The same with the rest of it, leaving unclosed { around is probably bad. (I'm not sure how much that'll affect it though since the output will be mostly empty anyway.) Other than that, fine with me. Please attach a git-formatted patch so I can push it easily.
Created attachment 21956 [details] [review] break from loop after writing out an #error In an effort to put errors somewhere when the alloc fails, we can just write out an error to the C file, and continue the loop. Attach as a git patch.
Pushed as dd9514fe714d81b881a1bd6bd88d4287adc5fc7e. Thanks. (Now, if you have the time to figure out why the geometry still has BadAllocs in some cases, that'd be much appreciated :)
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.