]>
Commit | Line | Data |
---|---|---|
12788f63 MT |
1 | From libc-alpha-return-25252-listarch-libc-alpha=sources dot redhat dot com at sourceware dot org Thu Feb 16 16:21:17 2012 |
2 | Return-Path: <libc-alpha-return-25252-listarch-libc-alpha=sources dot redhat dot com at sourceware dot org> | |
3 | Delivered-To: listarch-libc-alpha at sources dot redhat dot com | |
4 | Received: (qmail 5187 invoked by alias); 16 Feb 2012 16:21:14 -0000 | |
5 | Delivered-To: moderator for libc-alpha at sourceware dot org | |
6 | Received: (qmail 2174 invoked by uid 22791); 16 Feb 2012 16:17:18 -0000 | |
7 | X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 | |
8 | tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,TW_TV,TW_VB,TW_VF,T_RP_MATCHES_RCVD | |
9 | X-Spam-Check-By: sourceware.org | |
10 | Date: Thu, 16 Feb 2012 08:16:13 -0800 | |
11 | From: Kees Cook <kees at outflux dot net> | |
12 | To: "Ryan S dot Arnold" <ryan dot arnold at gmail dot com> | |
13 | Cc: libc-alpha at sourceware dot org, Paul Eggert <eggert at cs dot ucla dot edu>, | |
14 | Roland McGrath <roland at hack dot frob dot com>, | |
15 | Andreas Schwab <schwab at linux-m68k dot org> | |
16 | Subject: Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap | |
17 | Message-ID: <20120216161613.GZ20420@outflux.net> | |
18 | References: <20120206062537.GM4979@outflux.net> | |
19 | <20120207000509 dot GP4989 at outflux dot net> | |
20 | <20120210192457 dot GF20420 at outflux dot net> | |
21 | <CAAKybw8AgkGsKAx=kvX4Tsi74f+HtuVnatTCB0VfsHi7vVFi1Q at mail dot gmail dot com> | |
22 | <20120214223048 dot GM20420 at outflux dot net> | |
23 | <CAAKybw_HS+cav+YcDw3ns7UXu6_xA7EHPrkiB87P+OGwEB0PVQ at mail dot gmail dot com> | |
24 | <20120214224543 dot GN20420 at outflux dot net> | |
25 | MIME-Version: 1.0 | |
26 | Content-Type: text/plain; charset=us-ascii | |
27 | Content-Disposition: inline | |
28 | In-Reply-To: <20120214224543 dot GN20420 at outflux dot net> | |
29 | X-MIMEDefang-Filter: outflux$Revision: 1.316 $ | |
30 | X-HELO: www.outflux.net | |
31 | Mailing-List: contact libc-alpha-help at sourceware dot org; run by ezmlm | |
32 | Precedence: bulk | |
33 | List-Id: <libc-alpha.sourceware.org> | |
34 | List-Subscribe: <mailto:libc-alpha-subscribe at sourceware dot org> | |
35 | List-Archive: <http://sourceware.org/ml/libc-alpha/> | |
36 | List-Post: <mailto:libc-alpha at sourceware dot org> | |
37 | List-Help: <mailto:libc-alpha-help at sourceware dot org>, <http://sourceware dot org/ml/#faqs> | |
38 | Sender: libc-alpha-owner at sourceware dot org | |
39 | Delivered-To: mailing list libc-alpha at sourceware dot org | |
40 | ||
41 | The nargs value can overflow when doing allocations, allowing arbitrary | |
42 | memory writes via format strings, bypassing _FORTIFY_SOURCE: | |
43 | http://www.phrack.org/issues.html?issue=67&id=9 | |
44 | ||
45 | This checks for nargs overflow and possibly allocates from heap instead of | |
46 | stack, and adds a regression test for the situation. | |
47 | ||
48 | I have FSF assignment via Google. (Sent from @outflux since that's how I'm | |
49 | subscribed here, but CL shows @chromium.org as part of my Google work.) | |
50 | ||
51 | This version disables the useless test on non-32-bit platforms. | |
52 | ||
53 | 2012-02-16 Kees Cook <keescook@chromium.org> | |
54 | ||
55 | [BZ #13656] | |
56 | * stdio-common/vfprintf.c (vfprintf): Check for nargs overflow and | |
57 | possibly allocate from heap instead of stack. | |
58 | * stdio-common/bug-vfprintf-nargs.c: New file. | |
59 | * stdio-common/Makefile (tests): Add nargs overflow test. | |
60 | ||
61 | ||
62 | diff -rup a/stdio-common/Makefile b/stdio-common/Makefile | |
63 | --- a/stdio-common/Makefile 2010-05-04 05:27:23.000000000 -0600 | |
64 | +++ b/stdio-common/Makefile 2012-02-20 21:57:52.983040992 -0700 | |
65 | @@ -60,7 +60,7 @@ tests := tstscanf test_rdwr test-popen t | |
66 | tst-popen tst-unlockedio tst-fmemopen2 tst-put-error tst-fgets \ | |
67 | tst-fwrite bug16 bug17 tst-swscanf tst-sprintf2 bug18 bug18a \ | |
68 | bug19 bug19a tst-popen2 scanf13 scanf14 scanf15 bug20 bug21 bug22 \ | |
69 | - scanf16 scanf17 tst-setvbuf1 | |
70 | + scanf16 scanf17 tst-setvbuf1 bug-vfprintf-nargs | |
71 | ||
72 | test-srcs = tst-unbputc tst-printf | |
73 | ||
74 | diff --git a/stdio-common/bug-vfprintf-nargs.c b/stdio-common/bug-vfprintf-nargs.c | |
75 | new file mode 100644 | |
76 | index 0000000..13c66c0 | |
77 | --- /dev/null | |
78 | +++ b/stdio-common/bug-vfprintf-nargs.c | |
79 | @@ -0,0 +1,78 @@ | |
80 | +/* Test for vfprintf nargs allocation overflow (BZ #13656). | |
81 | + Copyright (C) 2012 Free Software Foundation, Inc. | |
82 | + This file is part of the GNU C Library. | |
83 | + Contributed by Kees Cook <keescook@chromium.org>, 2012. | |
84 | + | |
85 | + The GNU C Library is free software; you can redistribute it and/or | |
86 | + modify it under the terms of the GNU Lesser General Public | |
87 | + License as published by the Free Software Foundation; either | |
88 | + version 2.1 of the License, or (at your option) any later version. | |
89 | + | |
90 | + The GNU C Library is distributed in the hope that it will be useful, | |
91 | + but WITHOUT ANY WARRANTY; without even the implied warranty of | |
92 | + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | |
93 | + Lesser General Public License for more details. | |
94 | + | |
95 | + You should have received a copy of the GNU Lesser General Public | |
96 | + License along with the GNU C Library; if not, write to the Free | |
97 | + Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA | |
98 | + 02111-1307 USA. */ | |
99 | + | |
100 | +#include <stdio.h> | |
101 | +#include <stdlib.h> | |
102 | +#include <stdint.h> | |
103 | +#include <unistd.h> | |
104 | +#include <inttypes.h> | |
105 | +#include <string.h> | |
106 | +#include <signal.h> | |
107 | + | |
108 | +static int | |
109 | +format_failed (const char *fmt, const char *expected) | |
110 | +{ | |
111 | + char output[80]; | |
112 | + | |
113 | + printf ("%s : ", fmt); | |
114 | + | |
115 | + memset (output, 0, sizeof output); | |
116 | + /* Having sprintf itself detect a failure is good. */ | |
117 | + if (sprintf (output, fmt, 1, 2, 3, "test") > 0 | |
118 | + && strcmp (output, expected) != 0) | |
119 | + { | |
120 | + printf ("FAIL (output '%s' != expected '%s')\n", output, expected); | |
121 | + return 1; | |
122 | + } | |
123 | + puts ("ok"); | |
124 | + return 0; | |
125 | +} | |
126 | + | |
127 | +static int | |
128 | +do_test (void) | |
129 | +{ | |
130 | + int rc = 0; | |
131 | + char buf[64]; | |
132 | + | |
133 | + /* Regular positionals work. */ | |
134 | + if (format_failed ("%1$d", "1") != 0) | |
135 | + rc = 1; | |
136 | + | |
137 | + /* Regular width positionals work. */ | |
138 | + if (format_failed ("%1$*2$d", " 1") != 0) | |
139 | + rc = 1; | |
140 | + | |
141 | + /* Positional arguments are constructed via read_int, so nargs can only | |
142 | + overflow on 32-bit systems. On 64-bit systems, it will attempt to | |
143 | + allocate a giant amount of memory and possibly crash, which is the | |
144 | + expected situation. Since the 64-bit behavior is arch-specific, only | |
145 | + test this on 32-bit systems. */ | |
146 | + if (sizeof (long int) == 4) | |
147 | + { | |
148 | + sprintf (buf, "%%1$d %%%" PRIdPTR "$d", UINT32_MAX / sizeof (int)); | |
149 | + if (format_failed (buf, "1 %$d") != 0) | |
150 | + rc = 1; | |
151 | + } | |
152 | + | |
153 | + return rc; | |
154 | +} | |
155 | + | |
156 | +#define TEST_FUNCTION do_test () | |
157 | +#include "../test-skeleton.c" | |
158 | diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c | |
159 | index 863cd5d..022e72b 100644 | |
160 | --- a/stdio-common/vfprintf.c | |
161 | +++ b/stdio-common/vfprintf.c | |
162 | @@ -235,6 +235,9 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap) | |
163 | 0 if unknown. */ | |
164 | int readonly_format = 0; | |
165 | ||
166 | + /* For the argument descriptions, which may be allocated on the heap. */ | |
167 | + void *args_malloced = NULL; | |
168 | + | |
169 | /* This table maps a character into a number representing a | |
170 | class. In each step there is a destination label for each | |
171 | class. */ | |
172 | @@ -1647,9 +1650,10 @@ do_positional: | |
173 | determine the size of the array needed to store the argument | |
174 | attributes. */ | |
175 | size_t nargs = 0; | |
176 | - int *args_type; | |
177 | - union printf_arg *args_value = NULL; | |
178 | + size_t bytes_per_arg; | |
179 | + union printf_arg *args_value; | |
180 | int *args_size; | |
181 | + int *args_type; | |
182 | ||
183 | /* Positional parameters refer to arguments directly. This could | |
184 | also determine the maximum number of arguments. Track the | |
185 | @@ -1698,13 +1702,33 @@ do_positional: | |
186 | ||
187 | /* Determine the number of arguments the format string consumes. */ | |
188 | nargs = MAX (nargs, max_ref_arg); | |
189 | + bytes_per_arg = sizeof (*args_value) + sizeof (*args_size) | |
190 | + + sizeof (*args_type); | |
191 | + | |
192 | + /* Check for potential integer overflow. */ | |
193 | + if (nargs > SIZE_MAX / bytes_per_arg) | |
194 | + { | |
195 | + done = -1; | |
196 | + goto all_done; | |
197 | + } | |
198 | ||
199 | /* Allocate memory for the argument descriptions. */ | |
200 | - args_type = alloca (nargs * sizeof (int)); | |
201 | + if (__libc_use_alloca (nargs * bytes_per_arg)) | |
202 | + args_value = alloca (nargs * bytes_per_arg); | |
203 | + else | |
204 | + { | |
205 | + args_value = args_malloced = malloc (nargs * bytes_per_arg); | |
206 | + if (args_value == NULL) | |
207 | + { | |
208 | + done = -1; | |
209 | + goto all_done; | |
210 | + } | |
211 | + } | |
212 | + | |
213 | + args_size = &args_value[nargs].pa_int; | |
214 | + args_type = &args_size[nargs]; | |
215 | memset (args_type, s->_flags2 & _IO_FLAGS2_FORTIFY ? '\xff' : '\0', | |
216 | - nargs * sizeof (int)); | |
217 | - args_value = alloca (nargs * sizeof (union printf_arg)); | |
218 | - args_size = alloca (nargs * sizeof (int)); | |
219 | + nargs * sizeof (*args_type)); | |
220 | ||
221 | /* XXX Could do sanity check here: If any element in ARGS_TYPE is | |
222 | still zero after this loop, format is invalid. For now we | |
223 | @@ -1973,8 +1997,8 @@ do_positional: | |
224 | } | |
225 | ||
226 | all_done: | |
227 | - if (__builtin_expect (workstart != NULL, 0)) | |
228 | - free (workstart); | |
229 | + free (args_malloced); | |
230 | + free (workstart); | |
231 | /* Unlock the stream. */ | |
232 | _IO_funlockfile (s); | |
233 | _IO_cleanup_region_end (0); | |
234 | -- | |
235 | 1.7.5.4 | |
236 | ||
237 | -- | |
238 | Kees Cook @outflux.net | |
239 |