Bug 20665 - xcb_auth.c doesn't treat ipv6-mapped ipv4-addresses correctly
Summary: xcb_auth.c doesn't treat ipv6-mapped ipv4-addresses correctly
Status: RESOLVED MOVED
Alias: None
Product: XCB
Classification: Unclassified
Component: Library (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: xcb mailing list dummy
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-14 13:10 UTC by Christoph Pfister
Modified: 2019-02-16 19:41 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
patch (944 bytes, patch)
2009-03-15 02:20 UTC, Julien Danjou
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph Pfister 2009-03-14 13:10:47 UTC
from irc:

<no_where> I believe I have found a bug in libxcb: src/xcb_auth.c. Line 190 reads:
<no_where>     APPEND(info->data, j, si6->sin6_addr.s6_addr[12]);
<no_where> As far as I can see, this will copy exactly one byte, because sizeof(si6->sin6_addr.s6_addr[12]) is 1. But it should copy four bytes, namely the IPv6-mapped IPv4-address from bytes 12 through 15.
<no_where> Can some knowledgable person confirm this, and if this analysis is correct, file a bug report?
<christoph4> no_where: hmm, your statement makes sense
<no_where> christoph4: at least in the old libX11/.../ConnDis.c, 4 bytes were copied explicitly

i've checked the xcb_auth.c code of current git head, and it matches the description
Comment 1 Julien Danjou 2009-03-15 02:20:08 UTC
Created attachment 23866 [details] [review]
patch

This patch should fix it. Can someone test it ?
Comment 2 no where 2009-03-15 07:29:33 UTC
I have implemented exactly the same patch and can confirm that it works.

Regards,

no where
Comment 3 Julien Danjou 2009-03-15 11:58:38 UTC
commit fcb433db80315a44154248a9229c9cfcbab63f04
Author: Julien Danjou <julien@danjou.info>
Date:   Sun Mar 15 10:18:50 2009 +0100

    Copy full IPv4 mapping (Bug #20665)
    
    Signed-off-by: Julien Danjou <julien@danjou.info>

Comment 4 Christoph Pfister 2009-03-15 13:21:37 UTC
xcb_auth.c: In function 'compute_auth':
xcb_auth.c:190: warning: passing argument 2 of 'do_append' makes pointer from integer without a cast

-do_append(info->data, j, &si6->sin6_addr.s6_addr[12], 4);
+do_append(info->data, &j, &si6->sin6_addr.s6_addr[12], 4);

Thanks for taking care of this, Julien.
Comment 5 no where 2009-03-15 13:52:56 UTC
blush...
yes, this is the line I have:
do_append(info->data, &j, &(si6->sin6_addr.s6_addr[12]), 4);

actually, I do this for all IPv6 addresses, so that the patch looks like this:

--- ./src/xcb_auth.c.ORIG       2008-08-28 13:49:21.000000000 +0200
+++ ./src/xcb_auth.c    2009-03-14 18:04:22.000000000 +0100
@@ -185,21 +185,13 @@
         case AF_INET6:
             /*block*/ {
             struct sockaddr_in6 *si6 = (struct sockaddr_in6 *) sockname;
-            if(IN6_IS_ADDR_V4MAPPED(SIN6_ADDR(sockname)))
-            {
-                APPEND(info->data, j, si6->sin6_addr.s6_addr[12]);
-                APPEND(info->data, j, si6->sin6_port);
-            }
-            else
-            {
-                /* XDM-AUTHORIZATION-1 does not handle IPv6 correctly.  Do the
-                   same thing Xlib does: use all zeroes for the 4-byte address
-                   and 2-byte port number. */
-                uint32_t fakeaddr = 0;
-                uint16_t fakeport = 0;
-                APPEND(info->data, j, fakeaddr);
-                APPEND(info->data, j, fakeport);
-            }
+           /* XXX
+              copy low-order address values only (not enough
+              space for a full IPv6 address)
+              note that the original code is false as it copies only one
+              byte of the address instead of four */
+           do_append(info->data, &j, &(si6->sin6_addr.s6_addr[12]), 4);
+           APPEND(info->data, j, si6->sin6_port);
         }
         break;
 #endif

and also for libX11 (although it's now unused):

--- ./src/ConnDis.c.ORIG        2009-02-17 15:24:40.000000000 +0100
+++ ./src/ConnDis.c     2009-03-14 17:35:11.000000000 +0100
@@ -1111,27 +1111,14 @@
 #endif /* AF_INET */
 #if defined(IPv6) && defined(AF_INET6)
        case AF_INET6:
-         /* XXX This should probably never happen */
          {
-           unsigned char ipv4mappedprefix[] = {
-               0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff };
-
-           /* In the case of v4 mapped addresses send the v4
-              part of the address - addr is already in network byte order */
-           if (memcmp((char*)addr+8, ipv4mappedprefix, 12) == 0) {
-               for (i = 20 ; i < 24; i++)
-                   xdmcp_data[j++] = ((char *)addr)[i];
-
-               /* Port number */
-               for (i=2; i<4; i++)
-                   xdmcp_data[j++] = ((char *)addr)[i];
-           } else {
-               /* Fake data to keep the data aligned. Otherwise the
-                  the server will bail about incorrect timing data */
-               for (i = 0; i < 6; i++) {
-                   xdmcp_data[j++] = 0;
-               }
-           }
+           /* XXX
+              copy low-order address values only (not enough
+              space for a full IPv6 address) */
+           for (i=20; i<24; i++)       /* do sin6_addr */
+               xdmcp_data[j++] = ((char *)addr)[i];
+           for (i=2; i<4; i++)         /* do sin6_port */
+               xdmcp_data[j++] = ((char *)addr)[i];
            break;
          }
 #endif /* AF_INET6 */

and then I can get XDM-AUTHORIZATION-1 working for IPv6 with both kdm and xdm:

--- ./kdm/backend/auth.c.ORIG   2009-03-14 16:45:56.000000000 +0100
+++ ./kdm/backend/auth.c        2009-03-14 16:59:53.000000000 +0100
@@ -628,12 +628,15 @@
                                debug( "Skipping IPv6 localhost address\n" );
                                continue;
                        }
+/* XXX */
+#if 0
                        /* Also skip XDM-AUTHORIZATION-1 */
                        if (auth->name_length == 19 &&
                            !memcmp( auth->name, "XDM-AUTHORIZATION-1", 19 )) {
                                debug( "Skipping IPv6 XDM-AUTHORIZATION-1\n" );
                                continue;
                        }
+#endif
                }
 # endif
                writeAddr( family, len, addr, file, auth, ok );
@@ -884,12 +887,15 @@
                                        debug( "Skipping IPv6 localhost address\n" );
                                        continue;
                                }
+/* XXX */
+#if 0
                                /* Also skip XDM-AUTHORIZATION-1 */
                                if (auth->name_length == 19 &&
                                    !memcmp( auth->name, "XDM-AUTHORIZATION-1", 19 )) {
                                        debug( "Skipping IPv6 XDM-AUTHORIZATION-1\n" );
                                        continue;
                                }
+#endif
                        }
 #endif
                }

--- ./auth.c.ORIG       2008-05-21 20:08:45.000000000 +0200
+++ ./auth.c    2009-03-14 18:09:11.000000000 +0100
@@ -756,12 +756,6 @@
                Debug ("Skipping IPv6 localhost address\n");
                continue;
            }
-           /* Also skip XDM-AUTHORIZATION-1 */
-           if (auth->name_length == 19 && 
-               strcmp(auth->name, "XDM-AUTHORIZATION-1") == 0) {
-               Debug ("Skipping IPv6 XDM-AUTHORIZATION-1\n");
-               continue;
-           }
        }
 #endif
        writeAddr(family, len, addr, file, auth);
@@ -1056,12 +1050,6 @@
                    Debug ("Skipping IPv6 localhost address\n");
                    continue;
                }
-               /* Also skip XDM-AUTHORIZATION-1 */
-               if (auth->name_length == 19 && 
-                   strcmp(auth->name, "XDM-AUTHORIZATION-1") == 0) {
-                   Debug ("Skipping IPv6 XDM-AUTHORIZATION-1\n");
-                   continue;
-               }
            }
 #endif
        }

Even though it may not be fully kosher, it's working nicely. (I do a lot of remote X programs via IPv6 and with XDM-AUTHORIZATION-1.)

Please forgive the mess I did to this bug report. :-)
Comment 6 Julien Danjou 2009-03-16 02:24:41 UTC
Err, my bad.

OTOH, Did you test your patch version for non mapped IPv6 address?
Comment 7 no where 2009-03-16 13:18:50 UTC
Yes, I am using them right now (all four patches, to libxcb, libX11, old school xdm, and kdm) on several machines.

Would probably need another similar patch for gdm.

It's not pretty, but XDM-AUTHORIZATION-1 cannot really handly v6 addresses, and I thought that simply using only to least significant part of the address is good enough in this case.

The nicer solution would have been to implement XDM-AUTHORIZATION-2 et al.
Comment 8 Alan Coopersmith 2009-03-16 13:24:12 UTC
XDM-AUTHORIZATION-2 got stuck in the "sounds like a nice standard, but what's
the point if no one cares enough to write code?" trap.   The draft is still
around, just waiting for someone to implement it and verify it works (and 
hopefully get someone with crypto background to verify it's a secure usage
of the specified cryptography).
Comment 9 no where 2009-03-16 13:28:07 UTC
Yes, I know.
Comment 10 GitLab Migration User 2019-02-16 19:41:50 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/lib/libxcb/issues/31.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.