Add a self test for verifying get_salt() buffer cleaning

[magnumripper]

This is for cases where salt size is actually a max size. As long as compare size == copy size, we sometimes pad salts with nulls. This is fine, but if they are not always padded with null (eg. when loading a shorter salt after a longer one), we might end up with a thrashed salt and false negatives. Maybe not during a crack, but certainly during pot sync or --show. And salt_hash() might end up wrong, which might not be too good...

[magnumripper]

Something like this

1. call get_salt()
2. write back a canary using the pointer that was returned, eg. memset(salt, 0xcc, params.salt_size)
3. call get_salt() again
4. verify canary is not still there

[jfoug]

78c137d

Please test. I just tested and saw no problems. But on an older version (in my VC), there were several that triggered the warning.

If you are happy with this change, close it. Pretty trivial stuff, as long as I understand what you were wanting.

[jfoug]

NOTE, this fails (from BF_std.h), until you memset it. That is due to structure packing, and 'random' values outside of the range of data.

typedef struct { BF_word salt[4]; unsigned char rounds; char subtype; } BF_salt;

[jfoug]

Starting to make salt pre-clean where needed: 8cf3844

More: 0bddce4

[magnumripper]

Yeah this looks just like what I imagined, and the fixes look just like what I thought could be needed. These bugs are really really nasty because they might not show up in self-test nor in TS. We have found some in the past by pure coincidence. Intermittent false negatives :scream:

I just tested and saw no problems.

Then why all fixes?!

[jfoug]

I am still working, but only have a couple left

2 that were REALLY nasty, but I will assure you would be bad, were the net-md5 / net-sha1. There we have 2 UNIQUE salt's. One from the thin and one from the dyna. So what I did, was simply memset the buffer (was doing that), and then instead of returning dyna's get_salt, I memcpy the return of get_salt (it will be a pointer only), into the cleaned buffer. That way dyna will salt-hash properly, since it is short compared to the thin formats salt structure.

Then why all fixes?!

I did a clean build and saw them.

[jfoug]

Note, when I get done, there will probably still be many left. This is just what cygwin-64 sees. I can also test when done, using ubuntu-64, and also with cygwin-32. The fixes are trivially easy, but there are many.

[magnumripper]

Wow, this might be my best self-test idea ever. That's a lot of formats! I'll concentrate on CUDA and OpenCL formats so we don't clash.

So bcrypt is a false positive? Are you seeing other false positives?

[jfoug]

here is the last that auto-showup in cygwin-64. NOTE, I did see other possible ones (and added memset to them also). 881d2ee I think we probably should audit all formats. If they contain a structure, they should memset. Only the ones which are simple char* where we have only 1 salt length should we leave them alone, and even there, there is NO problem doing a memset at salt() time. Salt is ONLY during load, and you will not see any slowdown at all with the memset.

[jfoug]

NOTE, I only look at the CPU formats. I would be 99% sure the GPU stuff needs done. Can you give those a go, I do not feel comfortable modifying code I do not have much experience running.

[jfoug]

SIP and 7z are now failing. Is this something that my format self_test change has killed ???

[magnumripper]

SIP and 7z are now failing. Is this something that my format self_test change has killed ???

Maybe they report a salt size larger than they actually allocate?

[magnumripper]

c9ef675 fixes CUDA (2 formats)

[magnumripper]

878d88d fixes OpenCL (6 formats)

[magnumripper]

SIP get_salt() is weird, needs several fixes. Are you working on it or should I?

[jfoug]

I fixed krb5-18, and mozilla format was coring. Mozilla was written horribly. I put a static salt on the stack in the get_salt() and the problem went away (and changed the SALT_SIZE macro. Not sure what fixed the overwrite, but it's gone. I do not think this was seen prior to this code. I bet the SALT_SIZE macro was the problem. But it could have been the sharing of the pointer. NOTE, the code allocated every get_salt() call before, and now simply uses the static.

[jfoug]

But SIP and 7z are both FAIL on ubuntu also.

[jfoug]

Im out for a while. Poker night. If you do not find the SIP/7z, I will dig into them later.

[magnumripper]

7z just needed the memset. I'll fix them.

[magnumripper]

7z and SIP done in f61ac75a. SIP was as weird as the Mozilla format.

[magnumripper]

Build bot is happy again

[magnumripper]

Full monty build: All 428 formats passed self-tests!

Good stuff. Many of these fixed formats possibly didn't actually have a problem (after all it was we who thrashed the salt buffer) but several of them certainly HAD problems, and now we are safe!

[magnumripper]

diffstat for this issue: 54 files changed, 135 insertions(+), 55 deletions(-)

[jfoug]

I agree, most did not have issues. BUT figuring out which had issues and which did not, AND then keeping that done over time as new formats are added, was not a good usage of time. Building automated test, then forcing formats to comform was a better way to go.

I bet there are still many formats that do not have memset, they just did not trip warning on systems they were built on. I think we can flush some of them out, by messing with compiler flags, things like structure alignment (setting it very high, like 32 bytes). That would make a executable we would not want to run, but which would (should?) point out formats not being salt cleaned.

[jfoug]

I added #pragma pack(16) to the tail end of memdbg.h I am not 100% sure it did anything, but after a rebuild, there were no new problem formats show. I am going to remove that pragma and stick a fork in it, saying we are done for now. This should be a nice canary warning when new formats are added that do not clean this up.

[magnumripper]

Hmm I wonder if we could/should do more or less the same test with binary()?

[jfoug]

Is there ever a case where binary returns less than BINARY_SIZE ???? I would think not, unless there were an intermix of 2 format types (say the salt told us to use MD5 or SHA1 in the process).

[magnumripper]

unless there were an intermix of 2 format types (say the salt told us to use MD5 or SHA1 in the process).

That is exactly the problem. I believe we have some formats where this applies. Dhiru is fond of supporting all sorts of stuff in one same format.

If nothing else, we should try this out and just see what happens. Unless it turns out useful, we don't need it committed.

[jfoug]

Should again be a non-noticeable issue to the end user. binary() is only done once at startup, and is pretty quickly. For most formats this would never be a bug, but the same could be said for the salt() function also. Very few formats was it a 'true' bug, but it would take a LONG time to get all the problem formats figured out, and memsets done. Then as soon as new formats were added, we would have to re-test. Having an automated 'smasher/tester' was very easy to do, and quickly showed us which format were candidates to possibly have problems.

If there are 3 formats where binary could also be an issue, then we certainly could put the smash/test code into binary also, outputting warnings so that when new formats are done, they stick out like a sore thumb.