ntlm: avoid breaking anti-aliasing rules

From: Antonio Quartulli <[email protected]...>

The problem is visible when compiling with -O2:

ntlm.c: In function ‘ntlm_phase_3’:
ntlm.c:305:9: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

         if ((*((long *)&buf2[0x14]) & 0x00800000) == 0x00800000)

The spec suggests to interpret those 4 bytes as a long, but this needs to be done carefully.

Signed-off-by Antonio Quartulli <[email protected]...>

---

I have tried to make the statement more clear and "aliasing safe" at the same time. However, this is only compiled tested as I have no way to test the whole NTLM thing. Sorry :(

Cheers,

 src/openvpn/ntlm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

src/openvpn/ntlm.c +0 -0

index 65c1cbf5..a251c11d 100644

Hi,

Quoted 29 lines · Antonio Quartulli

From: Antonio Quartulli <[email protected]...>

The problem is visible when compiling with -O2:

ntlm.c: In function ‘ntlm_phase_3’:
ntlm.c:305:9: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

         if ((*((long *)&buf2[0x14]) & 0x00800000) == 0x00800000)

The spec suggests to interpret those 4 bytes as a long, but this needs to be done carefully.

Signed-off-by Antonio Quartulli <[email protected]...>

---

I have tried to make the statement more clear and "aliasing safe" at the same time. However, this is only compiled tested as I have no way to test the whole NTLM thing. Sorry :(

Cheers,

 src/openvpn/ntlm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

src/openvpn/ntlm.c +0 -0

index 65c1cbf5..a251c11d 100644

src/openvpn/ntlm.c ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, · line 309

Quoted 9 lines

src/openvpn/ntlm.c +2 -1

@@ -302,7 +302,8 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2,
         /* Add target information block to the blob */
 
         /* Check for Target Information block */
-        if ((*((long *)&buf2[0x14]) & 0x00800000) == 0x00800000)
+        unsigned long *flags = (unsigned long *)(buf2 + 0x14);
+        if ((*flags & htonl(0x00800000)) == htonl(0x00800000))
         {
             tib_len = buf2[0x28];            /* Get Target Information block size */
             if (tib_len > 96)

I *think* this will break the functionality. Consider the following test code:

#include <arpa/inet.h>
#include <stdio.h>

int main(char *argv[], int argc)

{
    unsigned char buf[4] = {0, 0, 0x80, 0};

    if ((*((long *)&buf[0]) & 0x00800000) == 0x00800000)
        printf("old: true\n");
    else
        printf("old: false\n");

    unsigned long *flags = (unsigned long *)(buf);
    if ((*flags & htonl(0x00800000)) == htonl(0x00800000))
        printf("new: true\n");
    else
        printf("new: false\n");

    return 0;
}

On amd64, this prints:

old: true
new: false

I'd expect some 'ntohl' call in this code somewhere, but that assumes that the NTLM spec was designed somewhat sane and uses network byte order...

For now, I'd suggest to stick to just fixing the type-punned dereference and leave out any htonl/ntohl calls.

From: Antonio Quartulli <[email protected]...>

The problem is visible when compiling with -O2:

ntlm.c: In function ‘ntlm_phase_3’:
ntlm.c:305:9: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

         if ((*((long *)&buf2[0x14]) & 0x00800000) == 0x00800000)

The spec suggests to interpret those 4 bytes as a long, but this needs to be done carefully.

Signed-off-by Antonio Quartulli <[email protected]...>

---

v2: remove htonl/ntohl conversion as there is none in the original code
v3: correct author's email
v4: code used in v3 was not really alias-safe (thanks Steffan), fix it

 src/openvpn/ntlm.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

src/openvpn/ntlm.c +8 -1

index 167c10b8..077fa3e2 100644
@@ -302,7 +302,21 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2,
         /* Add target information block to the blob */
 
         /* Check for Target Information block */
-        if ((*((long *)&buf2[0x14]) & 0x00800000) == 0x00800000)
+        /* The NTLM spec instructs to interpret these 4 consecutive bytes as a
+         * 32bit long integer. However, no endianness is specified.
+         * The code here and that found in other NTLM implementations point
+         * towards the assumption that the byte order on the wire has to
+         * match the order on the sending and receiving hosts. Probably NTLM has
+         * been thought to be always running on x86_64/i386 machine thus
+         * implying Little-Endian everywhere.
+         *