Hi,
On 16-08-17 14:18, Antonio Quartulli wrote:
> From: Antonio Quartulli <anto...@openvpn.net>
>
> 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 <anto...@openvpn.net>
> ---
>
> 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(-)
>
> diff --git a/src/openvpn/ntlm.c b/src/openvpn/ntlm.c
> index 167c10b8..077fa3e2 100644
> --- a/src/openvpn/ntlm.c
> +++ b/src/openvpn/ntlm.c
> @@ -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.
> + *
> + * This said, in case of future changes, we should keep in mind that
> the
> + * byte order on the wire for the NTLM header is LE.
> + */
> + const size_t hoff = 0x14;
> + unsigned long flags = buf2[hoff] | (buf2[hoff + 1] << 8) |
> + (buf2[hoff + 2] << 16) | (buf2[hoff + 3] <<
> 24);
> + if ((flags & 0x00800000) == 0x00800000)
> {
> tib_len = buf2[0x28]; /* Get Target Information block
> size */
> if (tib_len > 96)
>
This fixes the strict aliasing, makes the LE assumption explicit, and
doesn't change the behaviour on LE machines. Good enough for me, ACK.
-Steffan
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
