Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Emil Velikov]

On 28 February 2017 at 22:52, Matthieu Herrb  wrote:
>
> On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:
>> Hi Matthieu,
>>
>> On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
>> > Provide the function definition for systems that don't have it.
>> >
>> > Signed-off-by: Matthieu Herrb 
>> > Reviewed-by: Alan Coopersmith 
>> > ---
>> >  configure.ac|  3 ++-
>> >  include/dix-config.h.in |  3 +++
>> >  include/os.h|  5 +
>> >  os/mitauth.c|  2 +-
>> >  os/timingsafe_memcmp.c  | 45 +
>> >  5 files changed, 56 insertions(+), 2 deletions(-)
>>
>> > --- /dev/null
>> > +++ b/os/timingsafe_memcmp.c
>> Shouldn't we add this new file to Makefile.am somewhere ?
>
> Hi,
>
> No; AC_REPLACE_FUNCS() takes completely care of it.
>
> In os/Makefile.am you have :
>
> libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)
>
> and LTLIBOBJS is expanded to the list of filenames corresponding to
> functions that need to be provided in the AC_REPLACE_FUNC() macro.

Indeed it does - thanks for the correction.
Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Matthieu Herrb]

On Wed, Mar 01, 2017 at 12:15:00AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 28-02-17 23:52, Matthieu Herrb wrote:
> > 
> > On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:
> > > Hi Matthieu,
> > > 
> > > On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
> > > > Provide the function definition for systems that don't have it.
> > > > 
> > > > Signed-off-by: Matthieu Herrb 
> > > > Reviewed-by: Alan Coopersmith 
> > > > ---
> > > >  configure.ac|  3 ++-
> > > >  include/dix-config.h.in |  3 +++
> > > >  include/os.h|  5 +
> > > >  os/mitauth.c|  2 +-
> > > >  os/timingsafe_memcmp.c  | 45 
> > > > +
> > > >  5 files changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > > --- /dev/null
> > > > +++ b/os/timingsafe_memcmp.c
> > > Shouldn't we add this new file to Makefile.am somewhere ?
> > 
> > Hi,
> > 
> > No; AC_REPLACE_FUNCS() takes completely care of it.
> > 
> > In os/Makefile.am you have :
> > 
> > libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)
> > 
> > and LTLIBOBJS is expanded to the list of filenames corresponding to
> > functions that need to be provided in the AC_REPLACE_FUNC() macro.
> 
> What about make dist for making the source tarbals ?
> 

Check the generated makefile. It's all autotools magic too :)

COMMON_DIST = ...

-- 
Matthieu Herrb


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Hans de Goede]

Hi,

On 28-02-17 23:52, Matthieu Herrb wrote:


On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:

Hi Matthieu,

On 28 February 2017 at 18:18, Matthieu Herrb  wrote:

Provide the function definition for systems that don't have it.

Signed-off-by: Matthieu Herrb 
Reviewed-by: Alan Coopersmith 
---
 configure.ac|  3 ++-
 include/dix-config.h.in |  3 +++
 include/os.h|  5 +
 os/mitauth.c|  2 +-
 os/timingsafe_memcmp.c  | 45 +
 5 files changed, 56 insertions(+), 2 deletions(-)



--- /dev/null
+++ b/os/timingsafe_memcmp.c

Shouldn't we add this new file to Makefile.am somewhere ?


Hi,

No; AC_REPLACE_FUNCS() takes completely care of it.

In os/Makefile.am you have :

libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)

and LTLIBOBJS is expanded to the list of filenames corresponding to
functions that need to be provided in the AC_REPLACE_FUNC() macro.


What about make dist for making the source tarbals ?

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Matthieu Herrb]

On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:
> Hi Matthieu,
> 
> On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
> > Provide the function definition for systems that don't have it.
> >
> > Signed-off-by: Matthieu Herrb 
> > Reviewed-by: Alan Coopersmith 
> > ---
> >  configure.ac|  3 ++-
> >  include/dix-config.h.in |  3 +++
> >  include/os.h|  5 +
> >  os/mitauth.c|  2 +-
> >  os/timingsafe_memcmp.c  | 45 +
> >  5 files changed, 56 insertions(+), 2 deletions(-)
> 
> > --- /dev/null
> > +++ b/os/timingsafe_memcmp.c
> Shouldn't we add this new file to Makefile.am somewhere ?

Hi,

No; AC_REPLACE_FUNCS() takes completely care of it.

In os/Makefile.am you have :

libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)

and LTLIBOBJS is expanded to the list of filenames corresponding to
functions that need to be provided in the AC_REPLACE_FUNC() macro.
-- 
Matthieu Herrb


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Emil Velikov]

Hi Matthieu,

On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
> Provide the function definition for systems that don't have it.
>
> Signed-off-by: Matthieu Herrb 
> Reviewed-by: Alan Coopersmith 
> ---
>  configure.ac|  3 ++-
>  include/dix-config.h.in |  3 +++
>  include/os.h|  5 +
>  os/mitauth.c|  2 +-
>  os/timingsafe_memcmp.c  | 45 +
>  5 files changed, 56 insertions(+), 2 deletions(-)

> --- /dev/null
> +++ b/os/timingsafe_memcmp.c
Shouldn't we add this new file to Makefile.am somewhere ?

-Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Matthieu Herrb]

Provide the function definition for systems that don't have it.

Signed-off-by: Matthieu Herrb 
Reviewed-by: Alan Coopersmith 
---
 configure.ac|  3 ++-
 include/dix-config.h.in |  3 +++
 include/os.h|  5 +
 os/mitauth.c|  2 +-
 os/timingsafe_memcmp.c  | 45 +
 5 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 4dcf8b5c2..f6a49302f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -221,7 +221,8 @@ AC_CHECK_FUNCS([backtrace ffs geteuid getuid issetugid 
getresuid \
mmap posix_fallocate seteuid shmctl64 strncasecmp vasprintf vsnprintf \
walkcontext setitimer poll epoll_create1])
 AC_CONFIG_LIBOBJ_DIR([os])
-AC_REPLACE_FUNCS([reallocarray strcasecmp strcasestr strlcat strlcpy strndup])
+AC_REPLACE_FUNCS([reallocarray strcasecmp strcasestr strlcat strlcpy strndup\
+   timingsafe_memcmp])
 AM_CONDITIONAL(POLL, [test "x$ac_cv_func_poll" = "xyes"])
 
 AC_CHECK_DECLS([program_invocation_short_name], [], [], [[#include ]])
diff --git a/include/dix-config.h.in b/include/dix-config.h.in
index 4f020e5d8..4b86c1a3c 100644
--- a/include/dix-config.h.in
+++ b/include/dix-config.h.in
@@ -238,6 +238,9 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_SYS_UTSNAME_H
 
+/* Define to 1 if you have the `timingsafe_memcmp' function. */
+#undef HAVE_TIMINGSAFE_MEMCMP
+
 /* Define to 1 if you have the  header file. */
 #undef HAVE_TSLIB_H
 
diff --git a/include/os.h b/include/os.h
index d2c41b402..aa231f550 100644
--- a/include/os.h
+++ b/include/os.h
@@ -590,6 +590,11 @@ extern _X_EXPORT char *
 strndup(const char *str, size_t n);
 #endif
 
+#ifndef HAVE_TIMINGSAFE_MEMCMP
+extern _X_EXPORT int
+timingsafe_memcmp(const void *b1, const void *b2, size_t len);
+#endif
+
 /* Logging. */
 typedef enum _LogParameter {
 XLOG_FLUSH,
diff --git a/os/mitauth.c b/os/mitauth.c
index 768a52a22..efae4404c 100644
--- a/os/mitauth.c
+++ b/os/mitauth.c
@@ -76,7 +76,7 @@ MitCheckCookie(unsigned short data_length,
 
 for (auth = mit_auth; auth; auth = auth->next) {
 if (data_length == auth->len &&
-memcmp(data, auth->data, (int) data_length) == 0)
+timingsafe_memcmp(data, auth->data, (int) data_length) == 0)
 return auth->id;
 }
 *reason = "Invalid MIT-MAGIC-COOKIE-1 key";
diff --git a/os/timingsafe_memcmp.c b/os/timingsafe_memcmp.c
new file mode 100644
index 0..36ab362a7
--- /dev/null
+++ b/os/timingsafe_memcmp.c
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2014 Google Inc.
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+int
+timingsafe_memcmp(const void *b1, const void *b2, size_t len)
+{
+const unsigned char *p1 = b1, *p2 = b2;
+size_t i;
+int res = 0, done = 0;
+
+for (i = 0; i < len; i++) {
+/* lt is -1 if p1[i] < p2[i]; else 0. */
+int lt = (p1[i] - p2[i]) >> CHAR_BIT;
+
+/* gt is -1 if p1[i] > p2[i]; else 0. */
+int gt = (p2[i] - p1[i]) >> CHAR_BIT;
+
+/* cmp is 1 if p1[i] > p2[i]; -1 if p1[i] < p2[i]; else 0. */
+int cmp = lt - gt;
+
+/* set res = cmp if !done. */
+res |= cmp & ~done;
+
+/* set done if p1[i] != p2[i]. */
+done |= lt | gt;
+}
+
+return (res);
+}
-- 
2.11.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel