On Fri, 2018-07-27 at 20:26 +0200, Philipp Wolski wrote: > (I hope this mail is sent as text, the corporate mailserver is kinda wonky > and the signatures are applied by administrative rule, though I do my best > to tinker with the clients settings to remove it.) > > Hi, > > I tested your patch and although it looks logically fine it would not > work. > > I'm sorry for not including an example but I frankly don't know how to > reproduce this but I suspect you need to create an a file with > filenames>16 chars and get ar to map them. > It happens only with one of eight libraries and the whole built system is > ancient legacy stuff. The lib is the largest of course, 252 objects with > an average filename length of 20 chars. > > Actually I tinkered around and finally added a printf and got this in your > name_len check (line 678): > > name_len=18 > name=DlfRemoveControl.o > name_off=18 > ARNAME_MAX=16 > > name_off is probably a coincidence. > ARNAME_MAX is 16 due to ar_hdr.ar_name[16] in ar.h (strangely on Android > ARNAME_MAX is hardcoded to 255 by the original patch in arscan.c)
Sorry, I thought it was 255. All the #ifdefs make it so hard to understand what's going on. And my test case happened to use a 16- character name, which is long enough for GNU ar to store it as a long name, but still within what make considers the maximum length of a short name. [...] > I don't believe (yeah "believe", no idea how this works) namebuf nor > ar_name are actually used for those mapped names. > > Therefore I question that name_len should check against the max size a > "mapped name" is allowed to be instead of ar_hdr size. [...] I agree. I'll go with INT_MAX as the maximum size (since offsets and sizes are mostly stored as ints here). Although alloca(INT_MAX) is not going to well! New patch attached. Ben. -- Ben Hutchings The two most common things in the universe are hydrogen and stupidity.
From dd759110a1914d9325bf57a9a83b3a61eac5420e Mon Sep 17 00:00:00 2001
From: Ben Hutchings <b...@decadent.org.uk>
Date: Sat, 28 Jul 2018 10:31:11 +0800
Subject: [PATCH] Fix validation of long names in archives
Commit bc9d72beb0cb "Resolve issues discovered by static code
analysis." added range checks on archive member name length. However,
on non-AIX systems it also checked BSD-style long names against the
short name limits and and checked the *offset* for GNU-style long
names against the short name limits. This caused valid long names to
be rejected.
* Record the size of the GNU name map, and validate offsets against that
* Ensure that the last entry in the name map is null-terminated
* Apply a maximum length of INT_MAX for long names
Reported-by: Philipp Wolski <philipp.wol...@kisters.de>
---
arscan.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arscan.c b/arscan.c
index 6bc5af2467c2..d63872f66ca2 100644
--- a/arscan.c
+++ b/arscan.c
@@ -414,6 +414,7 @@ ar_scan (const char *archive, ar_member_func_t function, const void *arg)
# endif
#endif
char *namemap = 0;
+ int namemap_size = 0;
int desc = open (archive, O_RDONLY, 0);
if (desc < 0)
return -1;
@@ -667,10 +668,15 @@ ar_scan (const char *archive, ar_member_func_t function, const void *arg)
&& namemap != 0)
{
int name_off = atoi (name + 1);
- if (name_off < 1 || name_off > ARNAME_MAX)
+ int name_len;
+
+ if (name_off < 0 || name_off >= namemap_size)
goto invalid;
name = namemap + name_off;
+ name_len = strlen (name);
+ if (name_len < 1)
+ goto invalid;
long_name = 1;
}
else if (name[0] == '#'
@@ -678,7 +684,8 @@ ar_scan (const char *archive, ar_member_func_t function, const void *arg)
&& name[2] == '/')
{
int name_len = atoi (name + 3);
- if (name_len < 1 || name_len > ARNAME_MAX)
+
+ if (name_len < 1 || name_len > INT_MAX)
goto invalid;
name = alloca (name_len + 1);
@@ -747,10 +754,13 @@ ar_scan (const char *archive, ar_member_func_t function, const void *arg)
char *clear;
char *limit;
- namemap = alloca (eltsize);
+ if (eltsize > INT_MAX)
+ goto invalid;
+ namemap = alloca (eltsize + 1);
EINTRLOOP (nread, read (desc, namemap, eltsize));
if (nread != eltsize)
goto invalid;
+ namemap_size = eltsize;
/* The names are separated by newlines. Some formats have
a trailing slash. Null terminate the strings for
@@ -765,6 +775,7 @@ ar_scan (const char *archive, ar_member_func_t function, const void *arg)
clear[-1] = '\0';
}
}
+ *limit = '\0';
is_namemap = 0;
}
signature.asc
Description: This is a digitally signed message part
