Bug#904749: make: patch of arscan.c fails dependency testing
Hi, the new patch looks good. It also passes the se_main compile and an entire rebuilt of the project. Thank you ! > Although alloca(INT_MAX) is not going to well! Reminds me of a popular song: "(Don't) Fear the OOM-Reaper" with the background vocals singing "just add more hardware"... Philipp Wolski
Bug#904749: make: patch of arscan.c fails dependency testing
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
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
---
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
Bug#904749: make: patch of arscan.c fails dependency testing
(pulled the gmail joker to get rid of the signature) Sorry me again, It just so came to me: name_len notwithstanding, name_off==0 might actually be valid as a pointer offset (since the if is denying name_off<1) Philipp Wolski
Bug#904749: make: patch of arscan.c fails dependency testing
(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) After reading the comments in this ifdef riddled code I'm not convinced that checking the name_len againt ARNAME_MAX ist correct in the is_mapped part. An earlier part above line 652 comments how traditionally the names are only 14 characters, while the namemap part sets long_name=1 and the comment hints to a GNU addition. This goes for the later(BSD?) code as well where name is actually alloc'ed to a potentially larger size than namebuf can hold. 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. Any idea where this can be found? Or am I completely off? (can't find any reference to ar being limited to 16 chars per se though, and removing the name_len check still works without segfault) I admit I haven't looked up the cause for the original name_len check patch since the upstream tar does not check it, so I might be noisy, sorry. Fun fact, make actually quits with "no rule to make target ../gen/gnmt.a(EnMeasOrig.o)" right after the above printf, These are the first lines of the dep file: se_main: //usr/lib/x86_64-linux-gnu/libm-2.27.a se_main: ../gen/gnmt.a(EnMeasOrig.o) se_main: ../gen/gnmt.a(EnMeasType.o) se_main: ../gen/gnmt.a(EnTapType.o) se_main: ../gen/gnmt.a(gnmt.o) [this goes on for a lot more .o, though se_main does not even use the A to D files like DlfRemoveControl ] Philipp Wolski - KISTERS AG - Haselriege 13 - 26125 Oldenburg - Germany Handelsregister Aachen, HRB-Nr. 7838 | Vorstand: Klaus Kisters, Hanns Kisters | Aufsichtsratsvorsitzender: Dr. Thomas Klevers Phone: +49 441 93602 -158 | Fax: +49 441 93602 -222 | E-Mail: philipp.wol...@kisters.de | WWW: http://www.kisters.de Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet. This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure or distribution of the material in this e-mail is strictly forbidden.
Bug#904749: make: patch of arscan.c fails dependency testing
On Fri, 2018-07-27 at 23:41 +0800, Ben Hutchings wrote:
[...]
> Sorry about this. You didn't give an example to reproduce this, but I
> was able to construct one. Please could you test that the attached
> patch also works for your real usage?
[...]
Really attached this time.
Ben.
--
Ben Hutchings
Power corrupts. Absolute power is kind of neat. - John Lehman
From 9f15821377ec57bd36ae28eb3d430a66b87c9906 Mon Sep 17 00:00:00 2001
From: Ben Hutchings
Date: Fri, 27 Jul 2018 22:40:49 +0800
Subject: [PATCH] Fix validation of GNU-style long names in archives
Commit bc9d72beb0cb "Resolve issues discovered by static code
analysis." added range checks on archive member name length. However,
in the case of GNU-style long names, it range-checked the *offset* in
the name map as if it was a length. This caused valid long names to
be rejected.
* Record the size of the name map, and validate offsets against that
* Ensure that the last entry in the name map is null-terminated
* Check the length after finding the name in the name map
Reported-by: Philipp Wolski
---
arscan.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arscan.c b/arscan.c
index 6bc5af2467c2..f424128c2a8b 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 || name_len > ARNAME_MAX)
+goto invalid;
long_name = 1;
}
else if (name[0] == '#'
@@ -747,10 +753,11 @@ ar_scan (const char *archive, ar_member_func_t function, const void *arg)
char *clear;
char *limit;
-namemap = alloca (eltsize);
+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 +772,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
Bug#904749: make: patch of arscan.c fails dependency testing
Control: tag -1 upstream patch Control: forwarded -1 https://savannah.gnu.org/bugs/index.php?54395 On Fri, 2018-07-27 at 15:31 +0200, Philipp Wolski wrote: > Package: make > Version: 4.2.1-1.1 > Severity: important > > Dear Maintainer, > > we are using dependency files when static linking .a file build with ar -U. > > I noticed that some libraries failed with "no rule to make target > xxx.a(yyy.o)" > which would change when I moved the o-files around within the lib. > > It did not happen with the upstream make-4.2.1 tar-ball > > By bisecting the patches applied I found a patch to arscan.c to be the > culprit. > > Around line 669 of the patched arscan.c file it reads: > > int name_off = atoi (name + 1); > if (name_off < 1 || name_off > ARNAME_MAX) > goto invalid; > > name = namemap + name_off; > > I recon name_off is a pointer index which is added to the namemap pointer. > In this case, the check for name length violation does/should not apply here, > or must > be safeguarded differently. > > Removing the if let us compile again. Sorry about this. You didn't give an example to reproduce this, but I was able to construct one. Please could you test that the attached patch also works for your real usage? I also uploaded a new source package with this patch to: https://people.debian.org/~benh/packages/make-dfsg_4.2.1-1.2.dsc Ben. -- Ben Hutchings Power corrupts. Absolute power is kind of neat. - John Lehman signature.asc Description: This is a digitally signed message part
Bug#904749: make: patch of arscan.c fails dependency testing
Package: make Version: 4.2.1-1.1 Severity: important Dear Maintainer, we are using dependency files when static linking .a file build with ar -U. I noticed that some libraries failed with "no rule to make target xxx.a(yyy.o)" which would change when I moved the o-files around within the lib. It did not happen with the upstream make-4.2.1 tar-ball By bisecting the patches applied I found a patch to arscan.c to be the culprit. Around line 669 of the patched arscan.c file it reads: int name_off = atoi (name + 1); if (name_off < 1 || name_off > ARNAME_MAX) goto invalid; name = namemap + name_off; I recon name_off is a pointer index which is added to the namemap pointer. In this case, the check for name length violation does/should not apply here, or must be safeguarded differently. Removing the if let us compile again. -- System Information: Debian Release: buster/sid APT prefers unstable-debug APT policy: (500, 'unstable-debug'), (500, 'unstable'), (500, 'testing'), (500, 'stable') Architecture: amd64 (x86_64) Foreign Architectures: i386 Kernel: Linux 4.17.0-1-amd64 (SMP w/4 CPU cores) Locale: LANG=de_DE.utf8, LC_CTYPE=de_DE.utf8 (charmap=UTF-8), LANGUAGE=de_DE.utf8 (charmap=UTF-8) Shell: /bin/sh linked to /bin/bash Init: systemd (via /run/systemd/system) Versions of packages make depends on: ii libc6 2.27-5 make recommends no packages. Versions of packages make suggests: pn make-doc -- no debconf information Philipp Wolski - KISTERS AG - Haselriege 13 - 26125 Oldenburg - Germany Handelsregister Aachen, HRB-Nr. 7838 | Vorstand: Klaus Kisters, Hanns Kisters | Aufsichtsratsvorsitzender: Dr. Thomas Klevers Phone: +49 441 93602 -158 | Fax: +49 441 93602 -222 | E-Mail: philipp.wol...@kisters.de | WWW: http://www.kisters.de Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet. This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure or distribution of the material in this e-mail is strictly forbidden.
