Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Aug 2011 01:01:17 +0200
From: magnum <rawsmooth@...dband.net>
To: john-dev@...ts.openwall.com
Subject: Re: valgrind vs rules

I tried the same thing. But I suspect we should actually memset it to 
non-zero. I think that test in rules.c is a shortcut for sometimes 
skipping a memchk().

So I just dropped my fix and posted to john-dev a couple of days ago and 
will let Solar decide.

magnum


On 2011-08-24 00:55, jfoug@....net wrote:
> This 'fixes' the warning (it is just a warning).  It stems all the way back to wordlist.c  This is the 'jumbo' code, not the core code, but I think the same issue is in core john.  This does appear to be an issue.  I wonder if 'last' needs to be memset each time, of if the single \0 is enough?   It it is enough, then likely this bug would have likely only affected a single  candidate word.
>
> Jim.
>
> void do_wordlist_crack(struct db_main *db, char *name, int rules)
> {
>          union {
>                  char buffer[2][LINE_BUFFER_SIZE + CACHE_BANK_SHIFT];
>                  ARCH_WORD dummy;
>          } aligned;
>          const char *line = aligned.buffer[0];
>          char *last = aligned.buffer[1];
>          struct rpp_context ctx;
>          char *prerule, *rule, *word;
>          char *(*apply)(const char *word, char *rule, int split, char *last);
>          long file_len;
>          int i, pipe_input=0, max_pipe_words=0, rules_keep=0, init_this_time=1, really_done=0;
>          char msg_buf[128];
>
> #ifdef HAVE_MPI
>          char file_line[LINE_BUFFER_SIZE];
>          long my_size = 0;
>          unsigned int myWordFileLines = 0;
> #endif
>
>          log_event("Proceeding with wordlist mode");
>
>          _db = db;
>
>          length = db->format->params.plaintext_length;
>
> +      memset(&aligned, 0, sizeof(aligned));
>
>          if (name) {
>
>
> ---- magnum<rawsmooth@...dband.net>  wrote:
>> I get this from valgrind when running wordlist + rules (even in plain
>> 1.7.8, no jumbo):
>>
>> ==10714== Conditional jump or move depends on uninitialised value(s)
>> ==10714==    at 0x426690: rules_apply (rules.c:917)
>> ==10714==    by 0x42AC35: do_wordlist_crack (wordlist.c:218)
>> ==10714==    by 0x420170: main (john.c:306)
>> ==10714==  Uninitialised value was created by a stack allocation
>> ==10714==    at 0x42AA6D: do_wordlist_crack (wordlist.c:133)
>>
>>
>> relevant part of rules.c:
>>
>> 905    out_OK:
>> 906        in[rules_max_length] = 0;
>> 907        if (last) {
>> 908            if (length>  rules_max_length)
>> 909                length = rules_max_length;
>> 910            if (length>= ARCH_SIZE - 1) {
>> 911                if (*(ARCH_WORD *)in != *(ARCH_WORD *)last)
>> 912                    return in;
>> 913                if (strcmp(&in[ARCH_SIZE - 1],&last[ARCH_SIZE - 1]))
>> 914                    return in;
>> 915                return NULL;
>> 916            }
>> 917            if (last[length])
>> 918                return in;
>> 919            if (memcmp(in, last, length))
>> 920                return in;
>> 921            return NULL;
>> 922        }
>> 923        return in;
>>
>> length here is the length of the current word. As I understand it, if
>> the current word is longer than last has ever been, last[length] is
>> uninitialized - and this is what valgrind complains about. I'm not sure
>> I understand the purpose of line 917 at all so I'm not sure this is a
>> problem at all?
>>
>> magnum
>

Powered by blists - more mailing lists

Your e-mail address:

Powered by Openwall GNU/*/Linux - Powered by OpenVZ