Fuzzing Secure Message #762

ilammy · 2021-01-24T06:55:54Z

Okay... it's time to bury this old branch of mine properly. As you might have guessed from the author's date on this commit and boilerplate comments, this changeset is quite old.

Here are some fuzzers for Secure Message cryptosystem. They follow the general patterns established by Secure Cell but take into account specifics:

two modes: encryption and signature verification
two key types: EC and RSA

Encryption is fuzzed by trying to encrypt and then decrypt a short test message, letting AFL mess with the keys and input, looking for crashes caused by most likely by invalid keys or subtle data dependency (Secure Message should not depend on plaintext content, but in case it does...)

Decryption is fuzzed by decrypting a message encrypted by the reference implementation which is then mutated by AFL, looking for crashes that can be caused by message corruption.

Test data has been generated by attached generation tools. AFL does not need too much data to work, so that's just message as a payload with some random keys.

I have been exercising those tests on my Linux box for quite a while, and CI will also be running each of those for 30 seconds as a part of the "fuzzing" job. That should catch some low-hanging fruits, if they ever appear.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation
Changelog is updated

Okay... it's time to bury this old branch of mine properly. As you might have guessed from the author's date on this commit and boilerplate comments, this changeset is quite old. Here are some fuzzers for Secure Message cryptosystem. They follow the general patterns established by Secure Cell but take into account specifics: - two modes: encryption and signature verification - two key types: EC and RSA Encryption is fuzzed by trying to encrypt and then decrypt a short test message, letting AFL mess with the keys and input, looking for crashes caused by most likely by invalid keys or subtle data dependency (Secure Message should not depend on plaintext content, but in case it does...) Decryption is fuzzed by decrypting a message encrypted by the reference implementation which is then mutated by AFL, looking for crashes that can be caused by message corruption. Test data has been generated by attached generation tools. AFL does not need too much data to work, so that's just "message" as a payload with some random keys. I have been exercising those tests on my Linux box for quite a while, and CI will also be running each of those for 30 seconds as a part of the "fuzzing" job. That should catch some low-hanging fruits, if they ever appear.

ilammy · 2021-01-24T07:04:18Z

This changeset is expected to fail the AFL fuzzing job until the fixes from #763 are merged.

tools/afl/generate/smessage_encrypt_decrypt.go

tools/afl/generate/smessage_sign_verify.go

Lagovas · 2021-01-25T00:44:24Z

tools/afl/src/smessage_sign_roundtrip.c

@@ -0,0 +1,163 @@
+/*


why don't enough roundtrip check and use separate decrypt binaries from test files?

why don't enough roundtrip check and use separate decrypt binaries from test files?

I'm sorry, I don't quite understood this. Could you please rephrase?

We have generate/smessage_encrypt_decrypt binary, which encrypts file and write to file. Plus src/smessage_encrypt_decrypt, which decrypts data from a file. And src/smessage_encrypt_roundtrip which read file, encrypts and then decrypts encrypted data. The same actions, but by one binary. Why we need first two binaries two and can't leave only smessage_encrypt_roundtrip?

Why we need first two binaries two and can't leave only smessage_encrypt_roundtrip?

Oh, now I understood your concerns! Okay...

There are two binaries in order to fuzz the encryption and decryption code paths separately.

The way AFL works is that it mutates only initial inputs. It does not mess with the control flow of an executing process (it does monitor it though, to see the effect of the changes it makes to the input). Therefore, we have two binaries that exercise the following flows:

mutated(plaintext, keys) => encrypt() => ciphertext => decrypt() => expect same plaintext

mutated(ciphertext, keys) => decrypt() => expect expected plaintext

Note that in the first case the ciphertext is not mutated by AFL. Therefore, decrypt() can only be fuzzed via that path only if the input it mutated in such a particular way that encryption succeeds but produces a ciphertext so broken that will trigger decrypt() to fail. Unlikely, but we look for that too. More likely, though, is for it to break from come corruption, which is exactly what the second flow tests.

Also note that AFL looks for crashes. If we want to look for an invalid result being returned, we have to make it into a crash. We do so by using abort(). Effectively, we're looking at the following flows:

The roundtrip binaries:

mutated(plaintext, keys) => 💥

So broken that we can't even parse the input data. Those are ignored.

mutated(plaintext, keys) => encrypt() => 💥

Invalid plaintext or key triggered a crash (not detected and reported an error).

mutated(plaintext, keys) => encrypt() => ciphertext => decrypt() => 💥

That rare case from above. Plaintext or keys were broken just the right way for encryption to actually succeed, but produce some weird ciphertext which causes decryption to crash.

mutated(plaintext, keys) => encrypt() => ciphertext => decrypt() => not plaintext => 💥

Even rarer, probably impossible case where decryption also succeeds, but somehow produces a different plaintext without Themis noticing the discrepancy.

The encrypt/decrypt binaries:

mutated(ciphertext, keys) => 💥

Again, so broken that we can't even parse the input data. Ignore.

mutated(ciphertext, keys) => decrypt() => 💥

Looking for crashes caused by encrypted data to be corrupted in a creative way.

mutated(ciphertext, keys) => decrypt() => not expected plaintext => 💥

Super-rare, impossible case where you can corrupt the data, have it successfully decrypted, but the modification is not noticed by integrity checks of Themis.

vixentael · 2021-01-25T18:06:37Z

tools/afl/src/smessage_encrypt_roundtrip.c

+                message_size);
+        abort();
+    }
+    if (memcmp(message_bytes, decrypted_bytes, message_size) != 0) {


haha, this line (and similar lines below) will trigger all SAST tools again :D
(which is totally fine, just funny)

haha, this line (and similar lines below) will trigger all SAST tools again :D

But why? memcpy() is perfectly fine for comparing bytes...

Well, maybe they could be triggered to suggest a constant-time comparison here, but I don't want to link the entire OpenSSL here just for that, which will not yield any new issues being noticed by AFL. For one, this binary is leaking memory and it's fine (ASAN detects it, but AFL tells it to shut up).

they could be triggered to suggest a constant-time comparison here

exactly, because SAST tools are usually quite primitive and they don't care if memory comparison is happen on sensitive data or not.

tools/afl/generate/smessage_encrypt_decrypt.go

Lagovas

lgtm

ilammy · 2021-01-28T11:40:13Z

Now that miscellaneous build insanity has been fixed on master and #763 with fixes landed there as well, the build with new fuzzers should go green.

ilammy added core Themis Core written in C, its packages tests Themis test suite labels Jan 24, 2021

ilammy requested review from vixentael, shadinua, iamnotacake and a team January 24, 2021 06:55

ilammy mentioned this pull request Jan 24, 2021

Fix buffer and integer overflows in Secure Message #763

Merged

4 tasks

Lagovas reviewed Jan 25, 2021

View reviewed changes

Improve error handling in generator tools

27344b5

vixentael reviewed Jan 25, 2021

View reviewed changes

iamnotacake reviewed Jan 25, 2021

View reviewed changes

tools/afl/generate/smessage_encrypt_decrypt.go Show resolved Hide resolved

Lagovas approved these changes Jan 26, 2021

View reviewed changes

vixentael approved these changes Jan 26, 2021

View reviewed changes

Merge branch 'master' into smessage-fuzzing

604d8f8

ilammy merged commit 60a0989 into cossacklabs:master Jan 28, 2021

ilammy deleted the smessage-fuzzing branch January 28, 2021 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzzing Secure Message #762

Fuzzing Secure Message #762

ilammy commented Jan 24, 2021 •

edited

Loading

ilammy commented Jan 24, 2021

Lagovas Jan 25, 2021

ilammy Jan 25, 2021

Lagovas Jan 25, 2021

ilammy Jan 26, 2021

vixentael Jan 25, 2021

ilammy Jan 26, 2021

vixentael Jan 26, 2021

Lagovas left a comment

ilammy commented Jan 28, 2021 •

edited

Loading

Fuzzing Secure Message #762

Fuzzing Secure Message #762

Conversation

ilammy commented Jan 24, 2021 • edited Loading

Checklist

ilammy commented Jan 24, 2021

Lagovas Jan 25, 2021

Choose a reason for hiding this comment

ilammy Jan 25, 2021

Choose a reason for hiding this comment

Lagovas Jan 25, 2021

Choose a reason for hiding this comment

ilammy Jan 26, 2021

Choose a reason for hiding this comment

vixentael Jan 25, 2021

Choose a reason for hiding this comment

ilammy Jan 26, 2021

Choose a reason for hiding this comment

vixentael Jan 26, 2021

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

ilammy commented Jan 28, 2021 • edited Loading

ilammy commented Jan 24, 2021 •

edited

Loading

ilammy commented Jan 28, 2021 •

edited

Loading