777600 - (fuzzing-msgmgr) Fuzzing: Message Manager Protocols

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this.

Comment 2

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't think we would hold the release if we haven't fuzzed. But we should start fuzzing asap since otherwise there are bound to be exploits in there.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

We are picking this up as part of the renewed sandbox security program.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

How is this progressing cdiehl?

Comment 6

[Christoph Diehl [:posidron]]

I am going to upload in a few days a patch for a first review.
I want to make use of the new "--enable-fuzzing" compile time flag and got hold up a bit because of https://bugzilla.mozilla.org/show_bug.cgi?id=1320387

Comment 7

[Christoph Diehl [:posidron]]

Attachment: hermes_v0.9.diff

This is the current patch. This is not up for review yet.

Comment 8

[Christoph Diehl [:posidron]]

Attachment: hermes_v1.0.diff

Things which changed:

* added simple support for double, float, integer boolean and string types
* support objects inside of JSON messages (recursion)
* structural changes of the mutation function

Comment 9

[Dave Huseby [:huseby]]

Comment on attachment 8817479 [details] [diff] [review]
hermes_v1.0.diff

Review of attachment 8817479 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good so far.  Just a couple of nits.

::: dom/base/Hermes.cpp
@@ +29,5 @@
> +const unsigned int Hermes::sDefaultProbability = Hermes::DefaultProbability();
> +const bool Hermes::sIsLoggingEnabled = Hermes::Logging();
> +
> +Hermes::Hermes()
> +  : mEnabled(!!PR_GetEnv("ENABLE_HERMES_FUZZER"))

The !! is not necessary.  Pointers can be implicitly converted to bool in C++.  Null pointers are false, non-null pointers are true.

@@ +64,5 @@
> +
> +bool
> +Hermes::Logging(void)
> +{
> +  return !!PR_GetEnv("HERMES_ENABLE_LOGGING");

Here as well. No need for !!.  The explicit way to convert to a bool is to use static_cast<bool>(PR_GetEnv("HERMES_ENABLE_LOGGING"))

::: dom/base/nsFrameMessageManager.cpp
@@ +749,5 @@
>    }
>  
>    return DispatchAsyncMessageInternal(aCx, aMessageName, data, objects,
>                                        aPrincipal);
> +

please remove.

Comment 10

[Christoph Diehl [:posidron]]

Attachment: hermes_v1.1.diff

Comment 11

[Dave Huseby [:huseby]]

Comment on attachment 8819226 [details] [diff] [review]
hermes_v1.1.diff

Review of attachment 8819226 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Hermes.cpp
@@ +36,5 @@
> +
> +  const char* userSeed = PR_GetEnv("HERMES_SEED");
> +  unsigned long randomSeed = static_cast<unsigned long>(PR_IntervalNow());
> +  if (userSeed) {
> +    long n = std::strtol(userSeed, nullptr, 10);

I would change this to:

const unsigned long n = std::strtoul(userSeed, nullptr, 10);
if (n != 0) {
  randomSeed = n;
}

@@ +53,5 @@
> +{
> +  // Defines the likelihood of fuzzing a message.
> +  const char* probability = PR_GetEnv("HERMES_PROBABILITY");
> +  if (probability) {
> +    long n = std::strtol(probability, nullptr, 10);

You'll could run into range issues here.  You're converting a string to an 8-byte long value, but you're returning a 4-byte unsigned int.  You should probably use std::atoi() to get an int, or change the return type to be a long.

Comment 12

[Christoph Diehl [:posidron]]

Attachment: hermes_v1.2.diff

Bill, could you help find a person who has time to review this patch, so that we can land it?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8822512 [details] [diff] [review]
hermes_v1.2.diff

Review of attachment 8822512 [details] [diff] [review]:
-----------------------------------------------------------------

I can do the review. I'm afraid it needs a lot of work though.

::: dom/base/Hermes.cpp
@@ +15,5 @@
> +#include "mozilla/dom/ipc/StructuredCloneData.h"
> +#include "nsFrameMessageManager.h"
> +#include "nsXULAppAPI.h"
> +#include <cmath>
> +#include <climits>

#include files should be alphabetical.

@@ +68,5 @@
> +  return static_cast<bool>(PR_GetEnv("HERMES_ENABLE_LOGGING"));
> +}
> +
> +unsigned int
> +Hermes::Random(unsigned int aMax)

It looks like a lot of this code is copied from Faulty.cpp/h. We shouldn't do that. Please find a way to share the code. (Faulty has many of the same problems as this code, so please try to fix that as well.)

@@ +238,5 @@
> +}
> +
> +bool
> +Hermes::MutateJSON(JSContext* aCx, const nsAString& aMessageName,
> +                   const JS::Value& aJSON, StructuredCloneData& fuzzed) {

The brace should be on its own line.

@@ +250,5 @@
> +                NS_ConvertUTF16toUTF8(JSON).get());
> +
> +  // Parse JSON
> +  JS::Rooted<JS::Value> mutation(aCx, JS::NullValue());
> +  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const char16_t*>(JSON.get()),

This is going to break a lot of stuff even if you don't mutate anything. We typically don't use JSON to serialize JS data. We start by using structured cloning. If that fails we use JSON [1]. So you'll need to handle both cases here. I would suggest that you try to copy the data using structured clone. If that fails, don't bother trying to mutate. That should only affect a small number of messages, and you come back to those late.

I think that :baku can help you with the APIs for copying the data using structured clone.

[1] http://searchfox.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#499

@@ +254,5 @@
> +  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const char16_t*>(JSON.get()),
> +                              JSON.Length(), &mutation), false);
> +
> +  // Mutate
> +  if(mutation.isObject()) {

Would be nice to fuzz other types of values. I think it would be nicer to have a MutateValue method split out from a MutateObject method.

::: dom/base/Hermes.h
@@ +6,5 @@
> +
> +#ifndef Hermes_h
> +#define Hermes_h
> +
> +#include "nsFrameMessageManager.h"

This #include doesn't seem to be used.

@@ +7,5 @@
> +#ifndef Hermes_h
> +#define Hermes_h
> +
> +#include "nsFrameMessageManager.h"
> +#include "base/singleton.h"

This singleton.h thing is Chromium code that we would like to remove eventually, so please don't use it. It seems like everything in Hermes could be made static, which would make this a lot easier. For checking if it's enabled, you could use a method that checks a static variable.

@@ +8,5 @@
> +#define Hermes_h
> +
> +#include "nsFrameMessageManager.h"
> +#include "base/singleton.h"
> +#include "mozilla/dom/ipc/StructuredCloneData.h"

You don't need this #include. You just need to forward-declare StructuredCloneData. You probably will need to include "js/Rooted.h" and "jspubtd.h".

@@ +12,5 @@
> +#include "mozilla/dom/ipc/StructuredCloneData.h"
> +
> +
> +#define HERMES_DEFAULT_PROBABILITY 2
> +#define HERMES_LOG(fmt, args...) \

Doesn't seem like you need to put this in the header file.

@@ +23,5 @@
> +
> +class Hermes
> +{
> +  public:
> +    const bool mEnabled;

Please add an IsEnabled method and make mEnabled private (or eliminate it and use a static variable inside IsEnabled).

@@ +36,5 @@
> +    bool GetChance(unsigned int aProbability);
> +    nsAutoString ReadJSON(JSContext* aCx, const JS::Value& aJSON);
> +    bool MutateJSON(JSContext* aCx, const nsAString& aMessageName,
> +                    const JS::Value& aJSON, ipc::StructuredCloneData& fuzzed);
> +    void Mutate(JSContext* aCx, JS::Rooted<JS::Value>& mutation);

It seems like almost everything here except IsEnabled and MutateJSON could be private.

::: dom/base/moz.build
@@ +37,5 @@
>  XPIDL_MODULE = 'dom'
>  EXPORTS += [
>      'AutocompleteFieldList.h',
>      'Crypto.h',
> +    'Hermes.h',

Please don't call this Hermes. No one will know what it is. Please use a name like MessageManagerFuzzing.

::: dom/base/nsFrameMessageManager.cpp
@@ +723,5 @@
> +    StructuredCloneData fuzzed;
> +    Singleton<mozilla::dom::Hermes>::get()->MutateJSON(aCx, aMessageName,
> +                                                       aJSON, fuzzed);
> +
> +    if (!AllowMessage(fuzzed.DataLength(), aMessageName)) {

It seems like you could avoid this code duplication.

@@ +732,5 @@
> +    if (aArgc >= 3 && aObjects.isObject()) {
> +      objects = &aObjects.toObject();
> +    }
> +
> +    return DispatchAsyncMessageInternal(aCx, aMessageName, fuzzed, objects,

Why are you only fuzzing async messages? It seems like doing sync as well would be just a few extra lines of changes.

Comment 14

[Dave Huseby [:huseby]]

Comment on attachment 8822512 [details] [diff] [review]
hermes_v1.2.diff

Review of attachment 8822512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Hermes.cpp
@@ +20,5 @@
> +
> +using namespace mozilla;
> +using namespace mozilla::ipc;
> +using namespace mozilla::dom;
> +using namespace mozilla::dom::ipc;

If you are only using a few specific classes from these namespaces, you should consider adding just a forward declaration like so:

namespace mozilla {
    class TheOneClassINeed;
}

or even

namespace mozilla {
    namespace ipc {
        class TheOtherClassINeed;
    }
}

doing it this way might allow you to reduce the number of #include'd files at the top and speed up compilation.

@@ +48,5 @@
> +  HERMES_LOG("Random seed = %lu", randomSeed);
> +}
> +
> +unsigned int
> +Hermes::DefaultProbability(void)

This function does not mutate the internal state of the Hermes instance so this function can be declared const and don't use void in the params list.

unsigned int
Hermes::DefaultProbability() const

@@ +62,5 @@
> +  return HERMES_DEFAULT_PROBABILITY;
> +}
> +
> +bool
> +Hermes::Logging(void)

again

bool
Hermes::Logging() const

@@ +68,5 @@
> +  return static_cast<bool>(PR_GetEnv("HERMES_ENABLE_LOGGING"));
> +}
> +
> +unsigned int
> +Hermes::Random(unsigned int aMax)

I agree. Maybe create some utility classes that do the random number/probability stuff that is used by both.

here too

unsigned int
Hermes::Random(unsigned int aMax) const

@@ +75,5 @@
> +  return static_cast<unsigned int>(random() % aMax);
> +}
> +
> +bool
> +Hermes::GetChance(unsigned int aProbability)

again

bool
Hermes::GetChance(unsigned int aProbability) const

@@ +94,5 @@
> +/**
> + * RandomNumericValue generates negative and positive integrals.
> + */
> +template <typename T>
> +T RandomIntegral()

Maybe find where we define mozilla::IsArithmetic<T> and possibly make this mozilla::RandomIntegral<T>.  This function seems like a very generic, utility function that could be used by lots of different code.  We may even already have a function that does this somewhere.

@@ +113,5 @@
> + */
> +template <typename T>
> +T RandomNumericLimit() {
> +  static_assert(mozilla::IsArithmetic<T>::value == true,
> +                "T must be an arithmetic type");

same here, maybe good for adding to our numeric or math library?

@@ +122,5 @@
> +/**
> + * RandomIntegerRange returns a random integral within a user defined range.
> + */
> +template <typename T>
> +T RandomIntegerRange(T min, T max)

same here.

@@ +135,5 @@
> + * RandomFloatingPointRange returns a random floating-point number within a
> + * user defined range.
> + */
> +template <typename T>
> +T RandomFloatingPointRange(T min, T max)

this too.

@@ +148,5 @@
> +/**
> + * RandomFloatingPoint returns a random floating-point number.
> + */
> +template <typename T>
> +T RandomFloatingPoint()

and this.

@@ +167,5 @@
> +  return JSON;
> +}
> +
> +void
> +Hermes::Mutate(JSContext* aCx, JS::Rooted<JS::Value>& mutation) {

This can be const as well.

void
Hermes::Mutate(JSContext* aCx, JS::Rooted<JS::Value>& mutation) const

@@ +238,5 @@
> +}
> +
> +bool
> +Hermes::MutateJSON(JSContext* aCx, const nsAString& aMessageName,
> +                   const JS::Value& aJSON, StructuredCloneData& fuzzed) {

const too

::: dom/base/Hermes.h
@@ +7,5 @@
> +#ifndef Hermes_h
> +#define Hermes_h
> +
> +#include "nsFrameMessageManager.h"
> +#include "base/singleton.h"

You can get around the use of the singleton.h file by doing the following:

class Hermes
{
public:
    static Hermes * Instance() {
        if (!sInstance) {
            sInstance = new Hermes();
        }
        return sInstance;
    }

private:
    static Hermes * sInstance;
};

// initialize the instance
Hermes::sInstance = nullptr;


Then in your code you can always get the global instance of Hermes by doing:

Hermes * pHermes = Hermes::Instance();

There will only ever be a single instance of Hermes that will be constructed the first time it is referenced.  You probably want to use a smart pointer for the static member variable to ensure that the Hermes instance gets destructed when the process gets destroyed.

@@ +36,5 @@
> +    bool GetChance(unsigned int aProbability);
> +    nsAutoString ReadJSON(JSContext* aCx, const JS::Value& aJSON);
> +    bool MutateJSON(JSContext* aCx, const nsAString& aMessageName,
> +                    const JS::Value& aJSON, ipc::StructuredCloneData& fuzzed);
> +    void Mutate(JSContext* aCx, JS::Rooted<JS::Value>& mutation);

make all of the non-static member functions const since none of them mutate the internal state of Hermes.

::: dom/base/moz.build
@@ +37,5 @@
>  XPIDL_MODULE = 'dom'
>  EXPORTS += [
>      'AutocompleteFieldList.h',
>      'Crypto.h',
> +    'Hermes.h',

+1

::: dom/base/nsFrameMessageManager.cpp
@@ +718,4 @@
>    if (aArgc >= 2 && !GetParamsForMessage(aCx, aJSON, aTransfers, data)) {
>      return NS_ERROR_DOM_DATA_CLONE_ERR;
>    }
> +#ifdef FUZZING

Hermes const * const hermes = Hermes::Instance();
  if (hermes) {

Comment 15

[Christoph Diehl [:posidron]]

Attachment: hermes_v1.3.diff

Comment 16

[Christoph Diehl [:posidron]]

Attachment: hermes_v1.3.diff

Comment 17

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.3.diff

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8835267 [details] [diff] [review]
hermes.v1.3.diff

Review of attachment 8835267 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so I haven't looked at all the function bodies within MessageManagerFuzzer.cpp and FuzzingTraits.cpp. I think, first we need to fix some design and encapsulation issues with the current code. Also we need to address the question of what should happen at compile time and what needs to happen at run time.

::: dom/base/MessageManagerFuzzer.cpp
@@ +12,5 @@
> +#include "nsDebug.h"
> +#include "nsError.h"
> +#include "nsFrameMessageManager.h"
> +#include "nsJSUtils.h"
> +#include "nsXULAppAPI.h"

nit: some them in alphabetical order.

@@ +23,5 @@
> +#define HERMES_DEFAULT_MUTATION_PROBABILITY 2
> +#define HERMES_LOG(fmt, args...) \
> +  if (Hermes::IsLoggingEnabled()) { \
> +    printf_stderr("[HERMES] " fmt "\n", ## args); \
> +  }

nit: does that need to be a MACRO? If yes, then please align \ at the end of the longest line.

@@ +46,5 @@
> +Hermes::IsLoggingEnabled() {
> +  static bool sInitialized = false;
> +  static bool sIsLoggingEnabled = false;
> +  if (!sInitialized) {
> +      sIsLoggingEnabled = !!PR_GetEnv("HERMES_ENABLE_LOGGING");

where is that HERMES_ENABLE_LOGGING is coming from?
Either you make the logging dependend on an ifdef or you make it depenend on a pref, in which case you could convert the logging macro from the top into a function.

@@ +53,5 @@
> +  return sIsLoggingEnabled;
> +}
> +
> +Hermes::Hermes()
> +  : mHermesEnabled(IsValidToEnable())

I think you can drop the member variable mHermesEnabled completely and merge the function isValidToEnable and Enable() into one. You can then call this function from e.g. nsFrameMessageManager like you do at the moment OR you keep the member variable and handle the case whether is enabled internally within the object.

@@ +57,5 @@
> +  : mHermesEnabled(IsValidToEnable())
> +{
> +  HERMES_LOG("Initializing environment for Hermes.");
> +
> +  const char* userSeed = PR_GetEnv("HERMES_SEED");

where is HERMES_SEED coming from? is that a pref you are going to place in init/all.js? If so, then you should probably use GetBoolVarCache in the constructor.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

As discussed in Person, what you could do to simplify. Only have on public interface to your class, which could be Hermes::TryMutateJSON(...); within that static method you can then check if the fuzzer is enabled, if not, return right away, if it is enbabled, do whatever you need to do within that method, e.g. mutate the JSON.

I think there is no need for a singleton pattern and I also don't think there is any reason the Hermes object needs to hold an member variables, right?

Hope that helps to get you started, let's discuss more in person after that change.

Comment 20

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.4.diff

New patch attached with most of the review comments from above fixed. Please lets get this landed first (if there are not critical mistakes) so we can already setup automation for this and make further changes afterwards.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8836491 [details] [diff] [review]
hermes.v1.4.diff

Review of attachment 8836491 [details] [diff] [review]:
-----------------------------------------------------------------

This is better, but it doesn't fix the most important problem: not all data sent through the message manager can be converted to JSON. You're assuming it can be.

::: .hgignore
@@ +134,5 @@
>  
>  # tup database
>  ^\.tup
> +
> +ff-x86_64-apple-darwin16.4.0-asan-release/

Please don't add things here.

::: tools/fuzzing/common/FuzzingTraits.cpp
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace fuzzing {
> +
> +unsigned int

Please add /* static */ to the type of this method. Same for the one below.

::: tools/fuzzing/common/FuzzingTraits.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef FUZZING_COMMON_h___

This should be called mozilla_fuzzing_FuzzingTraits_h.

@@ +16,5 @@
> +namespace mozilla {
> +namespace fuzzing {
> +
> +/**
> + * RandomNumericValue generates negative and positive integrals.

I think you mean to say "integer" rather than "integral". "Integral" as a noun means the opposite of a derivative.

@@ +23,5 @@
> +T RandomIntegral()
> +{
> +  static_assert(mozilla::IsIntegral<T>::value == true,
> +                "T must be an integral type");
> +  double r = static_cast<double>(random() % ((sizeof(T) * CHAR_BIT) + 1));

Despite casting this to a double, you're still going to get an integer value out.

@@ +24,5 @@
> +{
> +  static_assert(mozilla::IsIntegral<T>::value == true,
> +                "T must be an integral type");
> +  double r = static_cast<double>(random() % ((sizeof(T) * CHAR_BIT) + 1));
> +  T x = static_cast<T>(pow(2.0, r)) - 1;

So this function only generates powers of two. Is that the intention? If so, it should be made clear in the name and the comment.

@@ +36,5 @@
> + * RandomNumericLimit returns either the min or max limit of an arithmetic
> + * data type.
> + */
> +template <typename T>
> +T RandomNumericLimit() {

Brace should go on the next line.

@@ +44,5 @@
> +                           : std::numeric_limits<T>::max();
> +}
> +
> +/**
> + * RandomIntegerRange returns a random integral within a user defined range.

Please say that the returned integer is in the range [min, max).

@@ +60,5 @@
> + * RandomFloatingPointRange returns a random floating-point number within a
> + * user defined range.
> + */
> +template <typename T>
> +T RandomFloatingPointRange(T min, T max)

This one is weird because it actually can return |max|, unlike RandomIntegerRange. I'm not sure what even makes sense here since there are no callers. Maybe just delete it.

@@ +73,5 @@
> +/**
> + * RandomFloatingPoint returns a random floating-point number.
> + */
> +template <typename T>
> +T RandomFloatingPoint()

I think this doesn't return the full range of floating point numbers, but that's probably fine.

@@ +84,5 @@
> +  return x * RandomFloatingPointRange<T>(-1.0, 1.0);
> +}
> +
> +class FuzzingTraits {
> +    public:

public: should not be indented. The methods below should be indented only two spaces.

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
@@ +24,5 @@
> +#include "nsLocalFile.h"
> +#include "nsTArray.h"
> +
> +#include <climits>
> +#include <cmath>

Are these used?

@@ +96,5 @@
> +  }
> +  // If something goes wrong with importing the file we return an empty string.
> +  if (valuesInFile.Length() == 0) {
> +    nsCString emptyStr;
> +    return emptyStr;

return nsCString();

@@ +209,5 @@
> +                   NS_ConvertUTF16toUTF8(JSON).get());
> +
> +  // Parse JSON
> +  JS::Rooted<JS::Value> mutation(aCx, JS::NullValue());
> +  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const char16_t*>(JSON.get()),

As I said in the previous review, the data sent over the message manager often cannot be converted to JSON correctly. So this code will break Firefox even if it doesn't mutate anything.

@@ +213,5 @@
> +  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const char16_t*>(JSON.get()),
> +                              JSON.Length(), &mutation), false);
> +
> +  // Mutate
> +  if (mutation.isObject()) {

I also described how you can fuzz more than just objects without much extra work. I don't see any reason not to do this.

@@ +257,5 @@
> +}
> +
> +/*static*/
> +bool
> +MessageManagerFuzzer::IsLoggingEnabled() {

Brace should be on its own line.

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MessageManagerFuzzer_h___

Should be mozilla_dom_MessageManagerFuzzer_h.

@@ +14,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +namespace ipc {
> +  class StructuredCloneData;

Shouldn't be indented.

@@ +19,5 @@
> +}
> +
> +class MessageManagerFuzzer
> +{
> +  public:

Shouldn't be indented.

@@ +20,5 @@
> +
> +class MessageManagerFuzzer
> +{
> +  public:
> +    static void TryMutateJSON(JSContext* aCx,

All these methods should be indented two spaces.

@@ +25,5 @@
> +                              const nsAString& aMessageName,
> +                              const JS::Value& aJSON,
> +                              ipc::StructuredCloneData& aData);
> +
> +  private:

Shouldn't be indented.

Comment 22

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.5.diff

(In reply to Bill McCloskey (:billm) from comment #21)

> > +T RandomIntegral()
> > +{
> > +  static_assert(mozilla::IsIntegral<T>::value == true,
> > +                "T must be an integral type");
> > +  double r = static_cast<double>(random() % ((sizeof(T) * CHAR_BIT) + 1));

> Despite casting this to a double, you're still going to get an integer value
> out.

pow() requires a double, therefore the cast.

> > +template <typename T>
> > +T RandomFloatingPointRange(T min, T max)

> This one is weird because it actually can return |max|, unlike
> RandomIntegerRange. I'm not sure what even makes sense here since there are
> no callers. Maybe just delete it.

The caller is in the function below that one: "RandomFloatingPoint()". 
This is mainly a dummy function for generating floats.

> > +  // Parse JSON
> > +  JS::Rooted<JS::Value> mutation(aCx, JS::NullValue());
> > +  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const char16_t*>(JSON.get()),

> As I said in the previous review, the data sent over the message manager
> often cannot be converted to JSON correctly. So this code will break Firefox
> even if it doesn't mutate anything.

I did not experience any breakage in my tests. This could always be added in a follow up patch since that approach would take more time to implement. I would like not to hold up this patch from landing cause it did find already some real bugs and it would make life much easier for starting to work on automation.

https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
http://searchfox.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#498

> > +  NS_ENSURE_TRUE(JS_ParseJSON(aCx, static_cast<const char16_t*>(JSON.get()),
> > +                              JSON.Length(), &mutation), false);
> > +
> > +  // Mutate
> > +  if (mutation.isObject()) {

> I also described how you can fuzz more than just objects without much extra
> work. I don't see any reason not to do this.

|mutation| is either null or a JSON object containing the individual types. If there is any other object in that JSON object, we recurse into it and fuzz possible datatypes inside that object. I do not know what you mean by 'more than just objects'.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I got confused when I was writing out my comments. I meant to say that RandomInteger is never used so it should be removed. I don't understand why you would want a function that returns powers of two.

> I did not experience any breakage in my tests. This could always be added in a follow up patch since
> that approach would take more time to implement. I would like not to hold up this patch from landing
> cause it did find already some real bugs and it would make life much easier for starting to work on
> automation.

I don't see any purpose in landing a fuzzing patch when we know that some of the "bugs" it finds will actually be bugs in the fuzzer.

This needs to be fixed. At this point I'd be willing to make the changes for you. I want to get this done as much as you do, but it needs to be done right.

> |mutation| is either null or a JSON object containing the individual types. If there is any other
> object in that JSON object, we recurse into it and fuzz possible datatypes inside that object. I do
> not know what you mean by 'more than just objects'.

|mutation| could also be a string, number, or boolean. I think it makes sense to fuzz those types as well.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8839600 [details] [diff] [review]
hermes.v1.5.diff

Review of attachment 8839600 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Bill McCloskey (:billm) from comment #21)
> This is better, but it doesn't fix the most important problem: not all data
> sent through the message manager can be converted to JSON. You're assuming
> it can be.

As discussed on IRC, we first need to address the comment by billm before we can move on. Clearing f? until then.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: hermes.v1.7.patch

Bill,

I helped cdiehl debugging some problems within his patch. We fixed a few minor issues and the good news is it works now (even though the patch still needs cleanup). Anyway, I think I found a potential problem outside of this patches' code. Probably you are remembering that the patch was hitting the assertion within StructureCloneData::Read() because mInitialized was false [0]. The root of this problem is within GetParamsForMessage() were we return 'true' in case of an error [1]. I suppose that should return 'false', no? If that is a problem I am happy to file a bug and get that fixed.

As of now I am using a workaround within the fuzzing patch like this:
+  if (data.DataLength() > 0) {
+    MessageManagerFuzzer::TryMutate(aCx, aMessageName, &data);
+  }
which only tries to mutate the data in case it was properly initialized. What do you think?

[0] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/StructuredCloneData.cpp#63
[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameMessageManager.cpp#510

Comment 26

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.8.diff

Updated and cleaned-up version.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8843979 [details] [diff] [review]
hermes.v1.8.diff

Review of attachment 8843979 [details] [diff] [review]:
-----------------------------------------------------------------

This is getting pretty close in my opinion. A few questions:
*) What is aRecusionCounter doing? It seems you are only using it for logging, right?
*) Would it make sense to abort in case something goes wrong in the setup? E.g. what if I forget to define any of the environment variables? Then the fuzzing is very limited in its functionality, right?

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
@@ +96,5 @@
> +
> +/* static */
> +void
> +MessageManagerFuzzer::MutateObject(JSContext* aCx,
> +                                   JS::HandleValue v,

nit: replace v with aValue

@@ +136,5 @@
> +/* static */
> +void
> +MessageManagerFuzzer::MutateValue(JSContext* aCx,
> +                                  JS::HandleValue v,
> +                                  JS::MutableHandleValue newV,

prefix argument names

@@ +140,5 @@
> +                                  JS::MutableHandleValue newV,
> +                                  unsigned short int aRecursionCounter)
> +{
> +  if (v.isInt32()) {
> +    if (FuzzingTraits::Sometimes(DefaultMutationProbability() * 2)) {

curious: why * 2?

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_MessageManagerFuzzer_h
> +#define mozilla_dom_MessageManagerFuzzer_h

nit, other h files use __ in the end, so please update to

#ifndef mozilla_dom_MessageManagerFuzzer_h__
#define mozilla_dom_MessageManagerFuzzer_h__

@@ +16,5 @@
> +
> +namespace ipc {
> +class StructuredCloneData;
> +}
> +

I think it would make sense to add a small description here. E.g. if I would want to use the fuzzer I would like to know what environment variables I have to set before I can run the fuzzer. Or also, what's the layout of the file defined within MESSAGEMANAGER_FUZZER_STRINGSFILE?

@@ +35,5 @@
> +    ipc::StructuredCloneData* aData);
> +  static void Mutate(JSContext* aCx, JS::Rooted<JS::Value>& aMutation);
> +  static void MutateObject(
> +    JSContext* aCx,
> +    JS::HandleValue value,

nit: aValue

@@ +39,5 @@
> +    JS::HandleValue value,
> +    unsigned short int aRecursionCounter);
> +  static void MutateValue(
> +    JSContext* aCx,
> +    JS::HandleValue value,

nit: aValue

@@ +40,5 @@
> +    unsigned short int aRecursionCounter);
> +  static void MutateValue(
> +    JSContext* aCx,
> +    JS::HandleValue value,
> +    JS::MutableHandleValue newV,

this is the outgoing value, right? please rename to aOutValue; (also in the cpp files please)

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)
> Created attachment 8843896 [details] [diff] [review]
> hermes.v1.7.patch
> 
> Bill,
> 
> I helped cdiehl debugging some problems within his patch. We fixed a few
> minor issues and the good news is it works now (even though the patch still
> needs cleanup). Anyway, I think I found a potential problem outside of this
> patches' code. Probably you are remembering that the patch was hitting the
> assertion within StructureCloneData::Read() because mInitialized was false
> [0]. The root of this problem is within GetParamsForMessage() were we return
> 'true' in case of an error [1]. I suppose that should return 'false', no? If
> that is a problem I am happy to file a bug and get that fixed.
> 
> As of now I am using a workaround within the fuzzing patch like this:
> +  if (data.DataLength() > 0) {
> +    MessageManagerFuzzer::TryMutate(aCx, aMessageName, &data);
> +  }
> which only tries to mutate the data in case it was properly initialized.
> What do you think?
> 
> [0]
> https://dxr.mozilla.org/mozilla-central/source/dom/ipc/StructuredCloneData.
> cpp#63
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/base/
> nsFrameMessageManager.cpp#510

The return in [1] is actually the success path. It's checking !rv.Failed(). If we return true from GetParamsForMessage, it seems like we've always successfully written to aData. So I don't understand why mInitialized would ever be false.

Comment 29

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.9.diff

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)

> This is getting pretty close in my opinion. A few questions:
> *) What is aRecusionCounter doing? It seems you are only using it for
> logging, right?

For now, yes. There might be a future use target for setting a max recursion limit but the problem didn't occur yet in MessageManager fuzzing.

> *) Would it make sense to abort in case something goes wrong in the setup?
> E.g. what if I forget to define any of the environment variables? Then the
> fuzzing is very limited in its functionality, right?

This is handled with default values. The only required variable is MESSAGEMANAGER_FUZZER_ENABLE. If this variable is not provided, the fuzzer is not enabled and nothing will happen.

> curious: why * 2?

This is almost arbitrary but history has proven that choosing MAX_ values lead more often to OOM situations rather memory corruptions caused by possible integer overflows in upcoming add/sub operations. We want to test it, just not as often as common values. 

MESSAGEMANAGER_FUZZER_DEFAULT_MUTATION_PROBABILITY equals 2.
2 * 2 = 4 
It is a 25% chance that we test with MAX_ values.

> @@ +16,5 @@
> > +
> > +namespace ipc {
> > +class StructuredCloneData;
> > +}
> > +
> 
> I think it would make sense to add a small description here. E.g. if I would
> want to use the fuzzer I would like to know what environment variables I
> have to set before I can run the fuzzer. Or also, what's the layout of the
> file defined within MESSAGEMANAGER_FUZZER_STRINGSFILE?

This will be documented in a GitHub page hosted at github.com/MozillaSecurity. There will also be a sample "strings" file. I would like not to put descriptions, except brief comments into the source because descriptions, help texts are subject to change frequently. I added a list of exposed environment variables as comment in MessageManagerFuzzer.h though.


Fixed nits.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Bill McCloskey (:billm) from comment #28)
> The return in [1] is actually the success path. It's checking !rv.Failed().
> If we return true from GetParamsForMessage, it seems like we've always
> successfully written to aData. So I don't understand why mInitialized would
> ever be false.

Oh, indeed, it's checking for |!rv.Failed()|. Maybe cdiehl has time to look into that again and can figure out why mInitialized would be false here. Anyway, using |if (data.DataLength() > 0) {| causes the problem to disappear. If really desired I can look into that problem again. Sorry for the false bug-alarm.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8844309 [details] [diff] [review]
hermes.v1.9.diff

Review of attachment 8844309 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for answering my question adn addressing my nits within comment 29. Looks pretty good to me. Ultimately it's bills call. Especially if we need to dig deeper to find out why mInitialized is ever false when we try to fuzz structuredCloneData.

Comment 32

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.10.diff

* added output for the mutated message as a whole
* added missing aTransfer to the aData->Write() 
* replaced:
  aData = &mutatedStructuredCloneData;
  with
  aData->Copy(mutatedStructuredCloneData);
  because the first variant did not do the trick.

I am now able to reproduce the same bugs found while I was using the JSON mutation method.

Comment 33

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.11.diff

* added blacklist to ignore certain messages from being fuzzed.
  - set with MESSAGEMANAGER_FUZZER_BLACKLIST
* if no "herems.strings" file is provided then string types won't get fuzzed.
  - fuzzing strings produces stalls when running against certain test-suites like mochitest because it's fuzzing itself / communication with the driver for running the tests, e.g: overwriting the URL.
* output original and mutated strings in debug information.

Comment 34

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.12.diff

* print out the whole message content only if the message got mutated.

Ok, I think that's it for now.

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8845387 [details] [diff] [review]
hermes.v1.12.diff

Review of attachment 8845387 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/fuzzing/messagemanager/MessageManagerFuzzer.cpp
@@ +228,5 @@
> +  ErrorResult rv;
> +  JS::RootedValue t(aCx, aTransfer);
> +
> +  /* Read original StructuredCloneData. */
> +  JS::RootedValue scdContent(aCx, JS::NullValue());

No need to set these to NullValue. They get set to UndefinedValue automatically.

@@ +251,5 @@
> +    JS_ClearPendingException(aCx);
> +    return false;
> +  }
> +
> +  aData->Copy(mutatedStructuredCloneData);

Copy isn't really supposed to be used on existing data. You can probably get away with it, but it will fail if anyone tries to send things like MessagePorts over a channel. I filed bug 1346040 about this. I guess you can probably land without it though.

The pointer approach I suggested would avoid this problem. Why didn't it work?

@@ +308,5 @@
> +/* static */
> +bool
> +MessageManagerFuzzer::IsEnabled()
> +{
> +  return (!!PR_GetEnv("MESSAGEMANAGER_FUZZER_ENABLE") && XRE_IsParentProcess());

This seems backwards to me. It means you're only mutating messages sent from the parent to the child. Messages from the parent are trustworthy. Messages from the child are not. The condition should be XRE_IsContentProcess().

Also, you can drop the outer parens.

Comment 36

[Christoph Diehl [:posidron]]

(In reply to Bill McCloskey (:billm) from comment #35)
> @@ +251,5 @@
> > +    JS_ClearPendingException(aCx);
> > +    return false;
> > +  }
> > +
> > +  aData->Copy(mutatedStructuredCloneData);
> 
> Copy isn't really supposed to be used on existing data. You can probably get
> away with it, but it will fail if anyone tries to send things like
> MessagePorts over a channel. I filed bug 1346040 about this. I guess you can
> probably land without it though.
> 
> The pointer approach I suggested would avoid this problem. Why didn't it
> work?

Christoph can probably answer this better, he was helping in debugging it at that time cause there was also that issue with "mInitialized". Later on it turned out that |aData| was not keeping the assigned mutated data; based on the output it looked like we were fuzzing but actually we weren't. Then I went with the Copy() approach and I got results.  I added a reference to the source for bug 1346040.

Comment 37

[Christoph Diehl [:posidron]]

Attachment: hermes.v1.13.diff

This update addresses the last three issues in Comment 35. I am carrying over the review+ from Bill in Comment 35.


\o/ Thanks to everyone for their help!

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2620293368c
Add MessageManager Fuzzer. r=billm

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d2620293368c