7z format set_key() modifies the caller's key

[solardiz]

I just went to relbench our upcoming release vs. 1.8.0-jumbo-1, but the 7z format of a --disable-openmp build of our current code segfaulted on me. Per gdb, the crash is on the assignment to the caller's key here:

static void sevenzip_set_key(char *key, int index)
{
        /* Convert key to utf-16-le format (--encoding aware) */
        int len;
        len = enc_to_utf16(saved_key[index], PLAINTEXT_LENGTH, (UTF8*)key, strlen(key));
        if (len <= 0) {
                key[-len] = 0; // match truncation
                len = strlen16(saved_key[index]);
        }
        len *= 2;
        saved_len[index] = len;

        new_keys = 1;
}

I think this is not allowed by our formats API, and we may be passing non-writable keys.

@kholia Can you (at least) comment on this, please? I think you wrote that format?

[solardiz]

With the key[-len] = 0; // match truncation line commented out, the format passes self-test for me. So why was this "match truncation" thing needed? If it's needed, then are we missing a test vector that would ensure we're doing everything that's needed?

(BTW, there's huge speedup for "Many salts" in this format. Does this mean it will actually crack multiple 7z archives at once roughly at the price of one? Have we tested that? We should.)

[solardiz]

The crash only happens with:

[Debug]
Benchmarks_1_8 = Y

[solardiz]

My guess is without proper truncation get_key won't match what's actually tested when a candidate password gets truncated in that place. This would be nasty. Now that I think of it more, I don't think we can detect that situation with our tests - we'd need to separately specify input plaintext and actually processed plaintext for that, and this doesn't fit in our current test vectors array.

[kholia]

I will be away for a while. This format has been heavily modified since I wrote it.

[solardiz]

I will be away for a while. This format has been heavily modified since I wrote it.

So we shouldn't expect any work from you on any of the issues assigned to you now, prior to our release?

[magnumripper]

The truncation could be eg. an invalid utf8 sequence. We should use ‘const’

[claudioandre-br]

The commit says: https://github.com/magnumripper/JohnTheRipper/commit/242931064b16bb07191a0a92aab44186a42e4fbc lei-april committed on 19 Aug 2015

7z: optimize SIMD performance

sort passwords by length before feeding them to SIMD SHA2 function

[magnumripper]

Get_key will be correct anyhow. Just drop that offendng truncation of key and keep the strlen16!

[magnumripper]

OpenCL version is correct

[solardiz]

The 7z format still appears to work after this fix, so I'll assume the fix is correct.