Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 8 May 2013 07:41:35 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Core: undefined behavior in DES_std.c and MD5_std.c

On Tue, May 07, 2013 at 11:26:50PM +0400, Alexander Cherepanov wrote:
> I've tried to compile core john with clang -fsanitize=undefined and have 
> got several errors (on x86_64).
> 
> in bsdicrypt:
> DES_std.c:407:18: runtime error: left shift of 1 by 63 places cannot be 
> represented in type 'long'
[...]
> The problem is that trying to compute 1<<31 or (long)1<<63 leads to an 
> undefined behavior (per C11, 6.5.7p4). A tentative patch (for these and 
> some more) is attached

I've just reviewed and applied this patch.  Thanks!

> but it seems a thorough check of all the file is 
> due because it freely mixes signed and unsigned types.

It is one of the oldest files in JtR, and I am fully aware of this
shortcoming.  Other files are also affected - even some new code in
them, where I don't yet deviate from the approach that had been taken in
those same files.

> Is it really necessary to have ARCH_WORD signed?

I think there are a few places that require a signed type like this,
e.g. DES_bs_cmp_all().  BTW, its "~(ARCH_WORD)0" should probably be
"~(unsigned ARCH_WORD)0", its right shifts could be turned from signed
(usually arithmetic) to unsigned (logical) ones and the value only then
cast to a signed type for the negation.

All of this is just asking for a rewrite.  Some years ago I actually
thought of rewriting JtR from scratch, but I never found time.

Alexander

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.