Opened 5 months ago

Closed 4 months ago

#21439 closed defect (implemented)

Add a configure option to disable safety features that make fuzzing harder

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-16 TorCoreTeam201702
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We've got quite a few places in our code where we use redundant safety features to prevent bugs from turning into really serious bugs. But many of those safety features interfere with fuzzing, by covering up any underlying bugs that fuzzing would otherwise detect.

For example, I'm thinking of:

  • The 4-byte sentinel word at the end of each buffer chunk
  • Various places in our code where we NUL-terminate stuff that doesn't actually (we hope!) need to be NUL-terminated.
  • The entire "memarea" fragmentation-resistant allocation strategy.
  • Probably some other stuff too

But in addition to hardening our code a little, these features all make some classes of memory bug less likely to get noticed by the sanitizers.

Now, you might argue that there's no need to have a way to fuzz without those safety features, if they actually do provide safety. But on the other hand, they're meant to provide redundant safety, and if they are ever actually needed, that's a bug in our code that we ought to fix.

Child Tickets

Change History (8)

comment:1 Changed 4 months ago by nickm

  • Owner set to nickm
  • Status changed from new to accepted

I checkpointed some work here in a branch called disable_memory_sentinels. Not for review or merge yet: it needs an option to turn it on and off.

comment:2 Changed 4 months ago by nickm

  • Status changed from accepted to needs_review

now ready for review.

comment:3 Changed 4 months ago by nickm

  • Keywords review-group-16 added

comment:4 Changed 4 months ago by ahf

  • Status changed from needs_review to needs_revision

Review comments:

  • In configure.ac: would it make sense to only allow memory-sentinels if either oss-fuzz or libfuzzer is enabled?
  • Minor nitpick: maybe remove the uncommented #define DEBUG_SENTINEL in buffers.c?

Other than that the code looks good to me.

comment:5 Changed 4 months ago by nickm

In configure.ac: would it make sense to only allow memory-sentinels if either oss-fuzz or libfuzzer is enabled?

I don't think so, because that doesn't include AFL_FUZZ.

Minor nitpick: maybe remove the uncommented #define DEBUG_SENTINEL in buffers.c?

Whoops. That change wasn't supposed to go in.

comment:6 Changed 4 months ago by nickm

  • Status changed from needs_revision to needs_review

I've added some fixups to disable_memory_sentinels . How does it look now?

comment:7 Changed 4 months ago by ahf

  • Status changed from needs_review to merge_ready

Looks good to me. I assume you squash the fixup commits together.

comment:8 Changed 4 months ago by nickm

  • Keywords TorCoreTeam201702 added
  • Resolution set to implemented
  • Status changed from merge_ready to closed

Squashed and merged!

Note: See TracTickets for help on using tickets.