Hi linux-modules@,
Chrome OS received a report of a buffer overflow in modprobe a few
weeks ago. We never got around to sending it upstream, so here it
goes.
A buffer overflow in modprobe is not very useful in cases in which
full root privileges are required to run modprobe itself. On Chrome OS
this was relevant as a persistence vector since modprobe was being
called with attacker-influenceable parameters on boot.
Cheers,
Jorge
=====
Modprobe versions, across a relatively large period include a heap
based buffer overflow in their parsing of the pre and post options to
the softdep statements.
The following file is sufficient to trigger the bug:
00000000 73 6f 66 74 64 65 70 20 30 20 20 70 6f 73 74 3a |softdep 0
post:| 00000010 20 30 0b |
0.|
00000013
Here both the double space and some kind of space at the end of the
line are needed to trigger the bug. This bug is quite controllable, we
can control both length and content. For example we can overflow 32
bytes of content using:
00000000 73 6f 66 74 64 65 70 20 30 20 20 70 6f 73 74 3a |softdep 0
post:| 00000010 20 41 41 41 41 41 41 41 41 41 41 41 41 41 41 41 |
AAAAAAAAAAAAAAA| 00000020 41 41 41 41 41 41 41 41 41 41 41 41 41 41
41 41 |AAAAAAAAAAAAAAAA| 00000030 41 20 0a
|A .| 00000033
The only constraints imposed on our content are that we can't write
null bytes, or bytes for which isspace(c) is true directly through
this overflow.
The bug is located in kmod/libkmod/libkmod-config.c where the function
kmod_config_add_softdep attempts to populate the following structure:
47 struct kmod_softdep {
48 char *name;
49 const char **pre;
50 const char **post;
51 unsigned int n_pre;
52 unsigned int n_post;
53 };
In particular this is attempted with a single call to malloc, making
two iterations over the line of the config file itself. On the first
pass the function calculates the total size of allocation needed for
the struct itself, both string arrays and all the actual string bytes.
On the second pass the contents of these arrays are are then copied
and populated. The bug however is caused by the ability to induce a
mis-match between these two loops.
207 static int kmod_config_add_softdep(struct kmod_config *config,
208 const
char *modname,
209 const
char *line)
210 {
211 struct kmod_list *list;
212 struct kmod_softdep *dep;
213 const char *s ,*p;
214 char *itr;
215 unsigned int n_pre = 0 ,n_post = 0;
216 size_t modnamelen = strlen(modname) + 1;
217 size_t buflen = 0;
[0]218 bool was_space = false;
219 enum { S_NONE ,S_PRE ,S_POST } mode = S_NONE;
220 DBG(config->ctx ,"modname=%s\n" ,modname);
221 /* analyze and count */
222 for (p = s = line; ; s++) {
223 size_t plen;
224 if (*s != '\0') {
[1]225 if (!isspace(*s)) {
226 was_space = false;
227 continue;
[2]228 }
229 if (was_space) {
230 p = s + 1;
231 continue;
[3]232 }
233 was_space = true;
234 if (p >= s)
235 continue;
236 }
237 plen = s - p;
238 if (plen == sizeof("pre:") - 1 &&
239 memcmp(p ,"pre:"
,sizeof("pre:") - 1) == 0)
240 mode = S_PRE;
[4]241 else if (plen == sizeof("post:") - 1 &&
242 memcmp(p ,"post:"
,sizeof("post:") - 1) == 0)
243 mode = S_POST;
244 else if (*s != '\0' || (*s == '\0' && !was_space)) {
245 if (mode == S_PRE) {
246 buflen += plen + 1;
247 n_pre++;
248 } else if (mode == S_POST) {
249 buflen += plen + 1;
250 n_post++;
251 }
252 }
253 p = s + 1;
254 if (*s == '\0')
255 break;
256 }
258 DBG(config->ctx ,"%u pre ,%u post\n" ,n_pre ,n_post);
[5]259 dep = malloc(sizeof(struct kmod_softdep) + modnamelen +
260 n_pre * sizeof(const char *) +
261 n_post * sizeof(const char *) +
262 buflen);
264 if (dep == NULL) {
265 ERR(config->ctx ,"out-of-memory modname=%s\n" ,modname);
266 return -ENOMEM;
267 }
268 dep->n_pre = n_pre;
269 dep->n_post = n_post;
270 dep->pre = (const char **)((char *)dep + sizeof(struct
kmod_softdep));
271 dep->post = dep->pre + n_pre;
272 dep->name = (char *)(dep->post + n_post);
273 memcpy(dep->name ,modname ,modnamelen);
274 /* copy strings */
275 itr = dep->name + modnamelen;
276 n_pre = 0;
277 n_post = 0;
278 mode = S_NONE;
279 for (p = s = line; ; s++) {
280 size_t plen;
281 if (*s != '\0') {
282 if (!isspace(*s)) {
283 was_space = false;
284 continue;
285 }
[6]286 if (was_space) {
287 p = s + 1;
288 continue;
289 }
290 was_space = true;
291 if (p >= s)
292 continue;
293 }
294 plen = s - p;
295 if (plen == sizeof("pre:") - 1 &&
296 memcmp(p ,"pre:"
,sizeof("pre:") - 1) == 0)
297 mode = S_PRE;
[7]298 else if (plen == sizeof("post:") - 1 &&
299 memcmp(p ,"post:"
,sizeof("post:") - 1) == 0)
300 mode = S_POST;
301 else if (*s != '\0' || (*s == '\0' && !was_space)) {
302 if (mode == S_PRE) {
303 dep->pre[n_pre] = itr;
304 memcpy(itr ,p ,plen);
305 itr[plen] = '\0';
306 itr += plen + 1;
307 n_pre++;
308 } else if (mode == S_POST) {
309 dep->post[n_post] = itr;
[8]311 memcpy(itr ,p ,plen);
312 itr[plen] = '\0';
313 itr += plen + 1;
314 n_post++;
315 }
316 }
317 p = s + 1;
318 if (*s == '\0')
319 break;
320 }
321 list = kmod_list_append(config->softdeps ,dep);
322 if (list == NULL) {
323 free(dep);
324 return -ENOMEM;
325 }
326 config->softdeps = list;
328 return 0;
329 }
When we enter the function, line will be set to the string "
post:....", because of the double space in the input line.
[0] was_space is initalised to false.
[1] isspace(*s) is true. Since was_space isn't set to true [3] until
after the modification of p to advance it at [2] this doesn't happen
on the first pass around the line, so p will continue to start with a
space.
[4] plen will not match the expected length for the string "post:"
because there's an extra space. So the loop here doesn't count the
post softdeps in the 'analyse and count' pass, any bytes we have here
won't be counted inside the malloc call. The final space at the end of
the line however means that when we complete this pass was_space will
be left set true and doesn't get reset before starting the 'copy
strings' pass.
This failure to reset the state of was_space between passes is the
root of the bug and the simplest patch to fix it is to reset it to
false around line 275. Hence, despite the fact that the two loops
within the function are superficially identical they can be coerced
into behaving differently on an identical string input. Inside the
'copy strings' pass we iterate over the string again, however because
was_space is now true instead of false as it was first time around we
enter the conditional at [6] and move the pointer to the start of the
string forward one character to skip the space.
This in turn means that by the time we reach [7] plen is correct to
match "post:", unlike in the first iteration.
So by the time we hit [8] we're in a position that we're copying bytes
and making array entries that weren't accounted for and aren't checked
against the previous counts.
Mitigation
Reset the state of was_space to false between passes.
For your assistance, we have provided the following code:
--- libkmod-config.c 2018-06-21 17:59:48.633600181 +0100
+++ libkmod-config.fixed 2018-09-12 21:42:34.499453017 +0100 @@
-333,6 +333,8 @@
memcpy(dep->name ,modname ,modnamelen);
+ was_space = false;
+ /* copy strings */
itr = dep->name + modnamelen;
n_pre = 0;
