diff mbox

Improve DST handling (Bug 23102, Bug 21942, Bug 18018, Bug, 23259, CVE-2011-0536 ).

Message ID d391bdd2-eb20-2b0c-23ba-9f949e16c360@redhat.com
State Superseded
Headers

Commit Message

Florian Weimer June 7, 2018, 11:38 a.m. UTC 
  On 06/06/2018 10:18 PM, Carlos O'Donell wrote:
> +is_dst (const char *input, const char *ref)

Maybe we should fix the strncmp bug and use that?

In rtld, the actual definitions are not built with the proper name, but 
aliases for use in IFUNCs, which we don't use in rtld.

The attached patch fixes that for x86-64 and i686 (multi-arch in both 
cases).  I don't think other architectures are affected (verified for 
ppc64le, build-many-glibcs.py is still running).

If you want to use strncmp to express more clearly what the code is 
about, I can commit the fix separately (with a proper ChangeLog entry etc.).

Thanks,
Florian

Comments

Carlos O'Donell June 8, 2018, 2:13 a.m. UTC | #1 
On 06/07/2018 07:38 AM, Florian Weimer wrote:
> On 06/06/2018 10:18 PM, Carlos O'Donell wrote:
>> +is_dst (const char *input, const char *ref)
> 
> Maybe we should fix the strncmp bug and use that?
> 
> In rtld, the actual definitions are not built with the proper name,
> but aliases for use in IFUNCs, which we don't use in rtld.
> 
> The attached patch fixes that for x86-64 and i686 (multi-arch in both
> cases).  I don't think other architectures are affected (verified for
> ppc64le, build-many-glibcs.py is still running).
> 
> If you want to use strncmp to express more clearly what the code is
> about, I can commit the fix separately (with a proper ChangeLog entry
> etc.).

I would like to exclude this for now.

I am happy to discuss using strncmp *after* we get the first patch in
which we all agree implements the semantics we want.

Note, I just took your new comment bit "(excluding the $ sign but 
including curly braces, if any)" and added it to my v4 patch which
I'll post after going through your other review. Thanks for that.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index ff7a95bee2..ff271c0bdf 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -192,12 +192,12 @@  is_trusted_path_normalize (const char *path, size_t len)
    * Must follow first two characters with zero or more [A-Za-z0-9_]
      (enforced by caller) or '}' (end curly quoted name).
 
-   If the sequence is a DST matching REF then the length of the DST is
+   If the sequence is a DST matching REF then the length of the DST
+   (excluding the $ sign but including curly braces, if any) is
    returned, otherwise 0.  */
 static size_t
 is_dst (const char *input, const char *ref)
 {
-  size_t ilen, rlen;
   bool is_curly = false;
 
   /* Is a ${...} input sequence?  */
@@ -207,35 +207,23 @@  is_dst (const char *input, const char *ref)
       ++input;
     }
 
-  /* Find longest valid input sequence.  */
-  ilen = 0;
-  while ((input[ilen] >= 'A' && input[ilen] <= 'Z')
-	 || (input[ilen] >= 'a' && input[ilen] <= 'z')
-	 || (input[ilen] >= '0' && input[ilen] <= '9')
-	 || (input[ilen] == '_'))
-    ++ilen;
-
-  rlen = strlen (ref);
-
-  /* Can't be the DST we are looking for.  */
-  if (rlen != ilen)
-    return 0;
-
-  /* Compare the DST (no strncmp this early in startup).  */
-  if (memcmp (input, ref, ilen) != 0)
+  /* Check for matching name, following closing curly brace (if
+     required), or trailing characters which are part of an
+     identifier.  */
+  size_t rlen = strlen (ref);
+  if (strncmp (input, ref, rlen) != 0
+      || (is_curly && input[rlen] != '}')
+      || ((input[rlen] >= 'A' && input[rlen] <= 'Z')
+	  || (input[rlen] >= 'a' && input[rlen] <= 'z')
+	  || (input[rlen] >= '0' && input[rlen] <= '9')
+	  || (input[rlen] == '_')))
     return 0;
 
   if (is_curly)
-    {
-      /* Invalid curly sequence!  */
-      if (input[ilen] != '}')
-	return 0;
-
-      /* Count the two curly braces.  */
-      ilen += 2;
-    }
-
-  return ilen;
+    /* Count the two curly braces.  */
+    return rlen + 2;
+  else
+    return rlen;
 }
 
 /* INPUT is the start of a DST sequence at the first '$' occurrence.
diff --git a/sysdeps/i386/i686/multiarch/strncmp-c.c b/sysdeps/i386/i686/multiarch/strncmp-c.c
index cc059da494..2e3eca9b2b 100644
--- a/sysdeps/i386/i686/multiarch/strncmp-c.c
+++ b/sysdeps/i386/i686/multiarch/strncmp-c.c
@@ -1,4 +1,4 @@ 
-#ifdef SHARED
+#if defined (SHARED) && IS_IN (libc)
 # define STRNCMP __strncmp_ia32
 # undef libc_hidden_builtin_def
 # define libc_hidden_builtin_def(name)  \
diff --git a/sysdeps/x86_64/multiarch/strncmp-sse2.S b/sysdeps/x86_64/multiarch/strncmp-sse2.S
index cc5252d826..0e1f52dea9 100644
--- a/sysdeps/x86_64/multiarch/strncmp-sse2.S
+++ b/sysdeps/x86_64/multiarch/strncmp-sse2.S
@@ -18,10 +18,13 @@ 
 
 #include <sysdep.h>
 
-#define STRCMP	__strncmp_sse2
-
-#undef libc_hidden_builtin_def
-#define libc_hidden_builtin_def(strcmp)
+#if IS_IN (libc)
+# define STRCMP	__strncmp_sse2
+# undef libc_hidden_builtin_def
+# define libc_hidden_builtin_def(strcmp)
+#else
+# define STRCMP strncmp
+#endif
 
 #define USE_AS_STRNCMP
 #include <sysdeps/x86_64/strcmp.S>