Fuzz testing #1712

hulkoba · 2023-12-14T15:13:08Z

PR description

This PR is not meant as a comprehensive Fuzz suite, just the scaffolding with a few API entries that should be easy to extend.

Relates to #1689

Adds fuzz test for

createCleartextMessage
createMessage({binary})
createMessage({text})
generateKey()
readKey({ armoredKey })
readKey({ binaryKey })
readMessage({ binaryMessage })
readMessage({ armoredMessage })

Running a fuzz test

The test/fuzz directory contains fuzz targets like for example createMessageBinary.

You can run one fuzz target:

TARGET=createMessageBinary npm run fuzz

Notice, that TARGET is the name of your fuzz target module.

(Don't) running with coverage

Jazzer.js, unfortunately, does not support coverage for esm tests, yet.

We've implemented the tests also with coverage, turned everything into cjs, but removed it because it is not super useful.
Because the imported modules were not read correctly, the result always shows, that the fuzzing function is 100% covered but not the function we are trying to fuzz.

We decided to use esm-tests and wait until jazzer.js adds coverage support for esm.
Then it can be adapted easily, by adding the --coverage option to the jazzer command.

because it is not super useul

since coverage does not work as expected at all, we can use esm. So if jazzer at some point, add esm support for esm, we can easily add it

larabr

Thanks for setting this up! 🙌
I left some comments, most of them are minor, but I'm especially interested in the CI management part 😄

larabr · 2023-12-15T10:18:24Z

test/fuzz/readKeyBinary.js

+ * @param { Buffer } inputData
+ */
+export function fuzz (inputData) {
+  const binaryKey = new Uint8Array(`-----BEGIN PGP PRIVATE KEY BLOCK-----\n ${inputData.toString('base64')} -----END PGP PRIVATE KEY BLOCK-----`);


Not sure if this test was finalised or missed by mistake?
I don't think this line does what would be expected: it just takes the string as "number" and uses it as size of the array 😄

BTW, a OpenPGP binary input does not correspond to an armored one in Uint8Array format. I think we could just pass the fuzzed/random input as binaryKey.

larabr · 2023-12-15T10:19:01Z

test/fuzz/readMessageText.js

@@ -0,0 +1,27 @@
+import { FuzzedDataProvider } from '@jazzer.js/core';


Minor, but for consistency, this file should be name readMessage**Armored**.js 🙂

larabr · 2023-12-15T10:23:22Z

test/fuzz/createCleartextMessage.js

@@ -0,0 +1,15 @@
+import { FuzzedDataProvider } from '@jazzer.js/core';
+
+import openpgp from '../initOpenpgp.js';


Minor, but that entrypoint is just for mocha tests, I think it's best to use import * as openpgp from 'openpgp'; (or just import { <method> } from 'openpgp') in all these fuzz tests instead, to ensure we test the released bundle as-is 🙂

larabr · 2023-12-15T10:25:38Z

test/fuzz/readMessageBinary.js

@@ -0,0 +1,22 @@
+import openpgp from '../initOpenpgp.js';
+
+const ignored = ['This message / key probably does not conform to a valid OpenPGP format'];


Very minor, but I'd consider renaming ignored to expected, which as far as I understand it's the purpose it serves..? 🙂

larabr · 2023-12-15T10:34:48Z

test/fuzz/reports/.gitkeep

I assume this is needed as the fuzz lib expects it to exist?

Is there a way to have the reports output to stdout rather than creating files? Since we are not going to commit these reports (so this folder will always be empty), and we'd like to run some fuzz tests in the CI, I think that might be preferable.

Alternatively, if the folder is needed, I'd rather have the npm run fuzz command create it before running the fuzzer.

Exactly, this directory needs to exist. And there is no way to print the output to stout. But the naming was confusing. These are no reports, more 'artifacts'. I renamed it, to avoid confusion. The directory will be created before the fuzzer runs.

larabr · 2023-12-15T10:39:01Z

test/fuzz/createMessageBinary.js

This is more of a comment/question for @twiss , but I'd be for skipping fuzzing these create* methods, as there is no parsing nor input processing being done, and I think some internal crypto functions (decryption in particular) would be a better use of "fuzzing time" ..? 😄

larabr · 2023-12-15T10:49:05Z

test/fuzz/generateKey.js

+  const asciiString = data.consumeString(MAX_COMMENT_LENGTH);
+  const utf8String = data.consumeString(MAX_NAME_LENGTH, 'utf-8');
+
+  return openpgp.generateKey({ userIDs: [


It seems to me this is testing the UserID parsing part more than the key generation itself (which happens to be a not-so-cheap function to run)?
For the same purpose, I'd directly fuzz UserID.fromObject().

I'm not sure we need to fuzz generateKey in general, for similar reasons as the create* methods (cc @twiss).

I've added a fuzz test for UserIDPacket.fromObject()

larabr · 2023-12-15T10:52:38Z

package.json

@@ -47,6 +47,7 @@
    "prepare": "npm run build",
    "test": "mocha --timeout 120000 test/unittests.js",
    "test-type-definitions": "node --loader ts-node/esm test/typescript/definitions.ts",
+    "fuzz": "jazzer test/fuzz/$TARGET -- -artifact_prefix=test/fuzz/reports/",


With CI testing in mind, is there a way to have this run on all the possible targets automatically?

Unfortunately not.
The jest integration allows to put several fuzz tests into one describe block https://github.com/CodeIntelligenceTesting/jazzer.js/blob/main/docs/jest-integration.md#executing-jest-fuzz-tests.

For the fuzz targets, there is theoretically the --includes option, but it does not work with several fuzz targets.

To fuzz-test several functions, we could have one javascript file with all fuzz tests inside. This way is hard to debug when an error occurs. Especially when we have tests for one function with different parameter types.
Another way can be found in this example: https://github.com/CodeIntelligenceTesting/jazzer.js/tree/main/examples/bug-detectors
Each fuzz test has its package.json and on the root, a run-all shell script is executed, which runs each npm run test command for each package.json

The way this works today, we envision a long-running fuzz-testing run as a pre-release procedure, rather than fuzzing on each PR, which takes a long time, or if shortened, is not very useful.

Thanks for the explanation 🙂 The script approach sounds good, but I'd be more comfortable running the fuzzing CI on every commits to main (incl. PR merging), where we can allow it to run for longer compared to PR commits.
I wouldn't want to find out issues as I am about to release, as that might add a considerable delay to something else we have planned on top of it.

cc @twiss , for another opinion.

IIUC, properly fuzzing everything would take many hours, so running it on every commit to main might not really be feasible.

Also - AIUI fuzzing is normally not primarily to find regressions, but rather to find unknown issues. Obviously the latter can lead to the former, but e.g. if we find an issue using fuzzing, we can add a test case for it to make sure it doesn't regress (once fixed).

So I think running it occasionally, e.g. after large changes have been made, is fine 😊

larabr · 2023-12-15T10:57:52Z

test/fuzz/readMessageBinary.js

+ * @param { Buffer } inputData
+ */
+export function fuzz (inputData) {
+  const binaryMessage = new Uint8Array(`-----BEGIN PGP MESSAGE-----\n ${inputData.toString('base64')} -----END PGP MESSAGE-----`);


See https://github.com/openpgpjs/openpgpjs/pull/1712/files#r1427803383

larabr · 2023-12-15T11:00:55Z

test/fuzz/readKeyArmored.js

+  const fuzzedText = data.consumeString(MAX_MESSAGE_LENGTH, 'utf-8');
+  const armoredKey = `-----BEGIN PGP PRIVATE KEY BLOCK-----\n ${fuzzedText} -----END PGP PRIVATE KEY BLOCK-----`;
+
+  return openpgp.readKey({ armoredKey })


Most (if not all) of the functions being tested in this PR are actually async. I just want to make sure that the fuzzing lib is awaiting them..? Since it's not being done explicitly in this code (nor is the function declared async) 🙂

It does.
Whenever jazzer.js detects that the fuzz target returns a promise it will await the completion before it calls the fuzz target again.
This is explained here https://youtu.be/KyIhxEiNnfc?t=1571

hulkoba · 2023-12-18T11:01:20Z

(Put this already as a comment, so you see the PR description edit)
This PR is not meant as a comprehensive Fuzz suite, just the scaffolding with a few API entries that should be easy to extend.

hulkoba · 2023-12-18T12:03:19Z

test/fuzz/readMessageBinary.js

+  const bufferedMessage = new Uint8Array(Buffer.from(binaryMessage, 'utf8'));
+  return readMessage({ binaryMessage: bufferedMessage })
+    .catch(error => {
+      if (error.message && !ignoredError(error)) {


Question: This ignored error occurs each time because of this check:
(peekedBytes[0] & 0x80) === 0)
here https://github.com/openpgpjs/openpgpjs/blob/v6/src/packet/packet.js#L124

How can I avoid this?

This error means that the bytes are not a valid OpenPGP message, and should be rejected. So - for many byte arrays this error will be correct, and should indeed be ignored. In fact, I think there will be many other error messages that are (usually/hopefully) expected and correct, e.g. for malformed packets, etc.

I'm not super sure what's the easiest way to detect whether an error is "expected" or not; certainly things like TypeErrors will be unexpected, but those are also probably unlikely to happen purely based on the message contents (but perhaps not impossible).

Another option could be to check whether the message is "supposed" to be valid, e.g. with another OpenPGP implementation, though that would probably be a lot of work, and you might then run into many minor differences in parsing that may or may not be important (but might still be interesting).

hulkoba added 7 commits December 12, 2023 13:28

chore: install jazzerjs

aeb467a

test(fuzz): add fuzz tests

eab3b8f

docs(fuzz): add readme

1f3b58d

docs(fuzz): add sync explaination

c10da44

chore(fuzz): remove coverage

b29c6fc

because it is not super useul

test(fuzz): turn everything into esm

4c0a324

since coverage does not work as expected at all, we can use esm. So if jazzer at some point, add esm support for esm, we can easily add it

chore(fuzz): change max message length

e43f924

larabr reviewed Dec 15, 2023

View reviewed changes

hulkoba force-pushed the fuzz-testing branch 2 times, most recently from 7e80bb1 to 99a506a Compare December 18, 2023 08:03

chore: create artifacts dir on run

eadc867

hulkoba force-pushed the fuzz-testing branch from 99a506a to eadc867 Compare December 18, 2023 08:07

hulkoba added 4 commits December 18, 2023 09:11

refactor(fuzz): readMessageText -> readMessageArmored

e7b270b

chore(fuzz): import specific function

2f05bd5

refactor(fuzz): rename errors: inored -> expected

227c12c

feat(fuzz): userId.fromObject

08cda81

hulkoba force-pushed the fuzz-testing branch from 9c6c094 to 08cda81 Compare December 18, 2023 09:45

hulkoba force-pushed the fuzz-testing branch from eeba908 to afc0432 Compare December 18, 2023 11:50

hulkoba commented Dec 18, 2023

View reviewed changes

fix(fuzz): pass buffered to readBinary funcs

02adea3

hulkoba force-pushed the fuzz-testing branch from afc0432 to 02adea3 Compare December 18, 2023 14:47

larabr force-pushed the v6 branch 2 times, most recently from b340608 to b41298a Compare March 22, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzz testing #1712

Fuzz testing #1712

hulkoba commented Dec 14, 2023 •

edited

larabr left a comment

larabr Dec 15, 2023

larabr Dec 15, 2023

larabr Dec 15, 2023

larabr Dec 15, 2023

larabr Dec 15, 2023

hulkoba Dec 18, 2023

larabr Dec 15, 2023

larabr Dec 15, 2023 •

edited

hulkoba Dec 18, 2023

larabr Dec 15, 2023

hulkoba Dec 18, 2023

larabr Dec 18, 2023

twiss Dec 18, 2023

larabr Dec 15, 2023

larabr Dec 15, 2023

hulkoba Dec 18, 2023 •

edited

hulkoba commented Dec 18, 2023

hulkoba Dec 18, 2023

twiss Dec 18, 2023

		@@ -0,0 +1,27 @@
		import { FuzzedDataProvider } from '@jazzer.js/core';

		@@ -0,0 +1,15 @@
		import { FuzzedDataProvider } from '@jazzer.js/core';

		import openpgp from '../initOpenpgp.js';

		@@ -0,0 +1,22 @@
		import openpgp from '../initOpenpgp.js';

		const ignored = ['This message / key probably does not conform to a valid OpenPGP format'];

Fuzz testing #1712

Are you sure you want to change the base?

Fuzz testing #1712

Conversation

hulkoba commented Dec 14, 2023 • edited

PR description

Running a fuzz test

(Don't) running with coverage

larabr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larabr Dec 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkoba Dec 18, 2023 • edited

Choose a reason for hiding this comment

hulkoba commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkoba commented Dec 14, 2023 •

edited

larabr Dec 15, 2023 •

edited

hulkoba Dec 18, 2023 •

edited