Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow adding additional ObjectGroups to environment using Profile #419

Conversation

0xedward
Copy link
Contributor

@0xedward 0xedward commented Feb 22, 2024

As far as I'm aware, currently the only way to add additional ObjectGroups to the JS environment requires maintaining a separate fork of Fuzzilli. I thought allowing users of Fuzzilli to supply additional ObjectGroups in their Profiles would remove the need to maintain a separate fork to have a different JS environment from the main branch of Fuzzilli.

An example of a fork that could've benefited from this change is fuzzilli4wasm

Example use case

For Hermes, we want to fuzz the TextEncoder web API (facebook/hermes@3863a36, facebook/hermes@7f9d9d5, facebook/hermes@14790c9):

// HermesProfile.swift
import Fuzzilli

let TextEncoderConstructor = ILType.object(ofGroup: "TextEncoder", withProperties: ["encoding"], withMethods: ["encodeInto", "encode"])

let hermesProfile = Profile(

    // ...

    additionalBuiltins: [
        "TextEncoder"           : TextEncoderConstructor,
    ],

    additionalObjectGroups: [
        ObjectGroup(
            name: "TextEncoder",
            instanceType: TextEncoderConstructor,
            properties: [
                "encoding"      : .jsString,
            ],
            methods: [
                "encodeInto"    : [.string] => .jsTypedArray("Uint8Array"),
                "encode"        : [.string, .iterable] => .object(), // Just an example, but .iterable should be a Uint8Array instead
            ]),
    ],

    // ...
)

Test plan

  1. Updated Fuzzilli.JavaScriptEnvironment.groups and Fuzzilli.JavaScriptEnvironment.builtinTypes access control modifiers from private to public
  2. Added print statements to print environment.groups and environment.builtinTypes in FuzzilliCli/main.swift
  3. Ran Fuzzilli - swift run FuzzilliCli --profile=hermes ../fuzzilli_build/bin/fuzzilli --storagePath=./corpus --logLevel=verbose
  4. Checked that TextEncoder is in the output for environment.groups and environment.builtinTypes

Comment on lines +443 to +446
logger.verbose("Loaded additional builtins from profile: \(profile.additionalBuiltins.map { $0.key })")
}
if !profile.additionalObjectGroups.isEmpty {
logger.verbose("Loaded additional ObjectGroups from profile: \(profile.additionalObjectGroups.map { $0.name })")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.verbose("Loaded additional builtins from profile: \(profile.additionalBuiltins.map { $0.key })")
}
if !profile.additionalObjectGroups.isEmpty {
logger.verbose("Loaded additional ObjectGroups from profile: \(profile.additionalObjectGroups.map { $0.name })")
logger.info("Loaded additional builtins from profile: \(profile.additionalBuiltins.map { $0.key })")
}
if !profile.additionalObjectGroups.isEmpty {
logger.info("Loaded additional ObjectGroups from profile: \(profile.additionalObjectGroups.map { $0.name })")

What are your thoughts on using logger.info instead logger.verbose here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this logging is necessary as we will also print all registered builtins in the initialize function in the JavaScriptEnvironment. So if you want to keep this for visibility then we should use .verbose, otherwise I think we could also drop this, your choice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I noticed that initialize in JavaScriptEnvironment prints all the registered builtins and objectGroups. Since we fuzz using quite a few Fuzzilli profiles, I think we'd find it helpful to have which of the objectGroups and builtins were loaded from the profile without having to search through the full list of objectGroups and builtins in our logs

Copy link
Collaborator

@carl-smith carl-smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Comment on lines +443 to +446
logger.verbose("Loaded additional builtins from profile: \(profile.additionalBuiltins.map { $0.key })")
}
if !profile.additionalObjectGroups.isEmpty {
logger.verbose("Loaded additional ObjectGroups from profile: \(profile.additionalObjectGroups.map { $0.name })")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this logging is necessary as we will also print all registered builtins in the initialize function in the JavaScriptEnvironment. So if you want to keep this for visibility then we should use .verbose, otherwise I think we could also drop this, your choice :)

@0xedward
Copy link
Contributor Author

0xedward commented Mar 5, 2024

Thanks for the review, Carl! :)

@carl-smith carl-smith merged commit d4bf44f into googleprojectzero:main Mar 5, 2024
3 checks passed
@0xedward 0xedward deleted the expose-config-for-additionalObjectGroups branch March 5, 2024 13:29
facebook-github-bot pushed a commit to facebook/hermes that referenced this pull request Apr 20, 2024
Summary:
For easier maintenance when pulling in future changes from upstream

Related PR: googleprojectzero/fuzzilli#419

Reviewed By: werew

Differential Revision: D54571292

fbshipit-source-id: 763b8ae1ae11aa458bd0b432f67640e0c1ab7668
facebook-github-bot pushed a commit to facebook/hermes that referenced this pull request Jun 24, 2024
Summary:
Original Author: [email protected]
Original Git: 5e22aeb
Original Reviewed By: werew
Original Revision: D54571292

For easier maintenance when pulling in future changes from upstream

Related PR: googleprojectzero/fuzzilli#419

Reviewed By: neildhar

Differential Revision: D58849769

fbshipit-source-id: 02ea7874d8cbdafc07a54499ca731f4620d58dc1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants