john-dev - Re: "SSH private keys cracker" patch for JtR [first cut for GSoC]

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 28 Apr 2011 01:38:15 +0200
From: magnum <rawsmooth@...dband.net>
To: john-dev@...ts.openwall.com
Subject: Re: "SSH private keys cracker" patch for JtR [first cut
 for GSoC]

On 2011-04-27 00:17, Dhiru Kholia wrote:
> I have tried to address these issues in the new version of the patch
> (attached as well as uploaded to the wiki).

Hi Dhiru, thanks for your work. I was just going to report a segfault 
problem but it seems it was fixed with this revision. I have some 
suggestions though, if I may:

1. I reverted all the openssl #includes and init/cleanup stuff that you 
added to john.c and moved the init stuff into an init() in ssh_fmt.c 
where (I think) it belongs. This worked right away with no further 
tweaks. I'm not sure if there is (or should be introduced) a good place 
we could put the OpenSSL cleanup stuff, but I just dropped those lines 
and let the OS take care of it.


2. I reverted ALL your changes to loader.c *without* moving anything of 
it elsewhere. None of it is needed because the unique $ssh$ tag will 
take care of it! Auto-detection works fine, forced format too. --show 
and --show=left works like expected (without removing any newlines). 
There are a few formats that bloat loader.c (for example to support 
input files in other formats, like L0phtcrack) but the majority of all 
formats do not touch this file.


3. I can't see why you encode the base64 blob (further) into hex, 
doubling its size? I'm not sure how much info you need to save, but I 
think you should just re-pack the data into something like the following 
(here with linebreaks, but there should be none). You can put the 
original file name in the "user" field:

dsa_test.key:$ssh$AES-128-CBC,5A8068279C0F46E98207150A3C2EC614$/
uiTinJ4RUjoZv0+pY1iMv1ciVarCi4z/b6ZiLAaVYpyJ1hXT2tci+Y2f3LaaExco
5wr1aAFM47xmRmGo82I/vCHGA9Rxg5waGC9pWOGZVuUQrDsU6tcUkCMB+2Z4GS95
ODGCdDK2gNetDnb2JvHsqATsm4Cc63GdhiZ0sCFYLqym+We1wCYalxsO21W+OgoB
3ogFFIw2r2F+qJzqM7AUCyLFhi5zGmu6SNeXvU4GzxGPFJNG0mAOUIwaaN1aDjcN
2k4bCrfybq3z6nCe3DBs8K22iN+8uRnSAbCOqzZXEdYqUYY5Kkj2neCTRTXINdgQ
+aSSYg3y5Y7boKkjIOrvPUWHeOyduQ+tersAEw7nCVJzr9N8tRg2qV4PUv1CKfpO
IFwB7/9somjYIjqWoaSzjxK0c8Rwz0S1pmr+uqrbwy+Peou5M3semHlBkGiU27fw
ohKy+MUNHbsNjysSZSElNKsMIPqTIVzZE1mVFA/0uMGqdpQ3bzBOjX2Zo6edFCOm
J4wYavW5wM+jmuVKPVVNy99Zxyep0FEdLP5Kb2c4Zl0S9f14/b6h6APixVe7zuvV
bSkByfJnyzhIOYBIyT7Mdw14r4AXJV6+ZoEw09wiwM=

This would more than half the need for your huge increase of 
LINE_BUFFER_SIZE. Maybe you need the Proc-Type field too? If so, just 
add it (only what is needed from it) with another $ separator.


Last, there are still some hunks in your patch that just did not belong 
there, like spurious whitespace changes and even a reference to 
SybaseASE. Not a big deal but it makes your patch a pita to combine with 
other ones.


> For a while, I am switching to "adding support for FileVault and
> archive files cracking" activity. Will revisit this patch later.

I look forward to that too!
magnum