Opened 3 years ago

Closed 2 years ago

#21470 closed task (fixed)

Write unit tests for security regressions

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: test, 030-backport, 029-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When we fix a security vulnerability like the ones in [
https://trac.torproject.org/projects/tor/wiki/TROVE TROVE], we sometimes forget to include a unit test.

Some security vulnerabilities are easy to test, such as parsing errors. We should write tests that fail with the bug, but succeed when it is fixed.

Here's an initial list of security fixes we can write unit tests for:

Let's open a child ticket of this ticket for each one.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by nickm

Can we just add the relevant fuzzing failure cases to tor-fuzzing-corpora?

comment:2 in reply to:  1 Changed 3 years ago by teor

Replying to nickm:

Can we just add the relevant fuzzing failure cases to tor-fuzzing-corpora?

That works too, as long as we run it often enough (I guess Google will do that for us?)

comment:3 Changed 3 years ago by teor

We should also add a case for #21471 (and a v3 HS descriptor GET and POST) to http.

comment:4 Changed 3 years ago by kcc

The recommended way is to add the failing inputs to the corpus once the bug is fixed.
This way oss-fuzz will regularly run these inputs.

We also recommend to have your own CI system run the inputs (not necessary with fuzzing),
especially if you have pre-submit testing, to find the regressions earlier.
oss-fuzz does not have any SLA regarding the turnaround time, and we can typically detect a bug within 24 hours after submission. It's very unlikely that we will report a regression earlier than 3-4 hours after the commit.

Having a separate unit test that detects the same bug is redundant to some extent,
but still might be a good idea sometimes (especially if you do run unit tests and don't execute fuzz targets on their corpora before submit)

comment:5 Changed 3 years ago by nickm

I added oss-fuzz/408 to our corpus repository with commit abd71a897d0618edb295bcfa93dc36899fc2b82c.

I opened a ticket (#21491) to check our fuzzing corpora as part of our CI process.

comment:6 Changed 3 years ago by nickm

(Teor, do you happen to have test files for any of the other cases that I could add?)

comment:7 Changed 3 years ago by teor

For #20894, the test case is:
https://github.com/teor2345/tor/blob/fuzz-dir-sensitive-v2/src/test/fuzz/fuzz_dir_testcase/dir-header-read-beyond-buffer.case

Do you still have the #21018 test case? I think you found it with libfuzzer?
(Or is it already in the corpus?)

For #21450, I think a unit test might be the easiest option, as we want something that differs on i386 and x86_64.
(Are we fuzzing on i386, or only x86_64?)

comment:8 in reply to:  3 ; Changed 3 years ago by teor

Replying to teor:

We should also add a case for #21471 (and a v3 HS descriptor GET and POST) to http.

Oh, and #21471 will require a GET request for a v3 HS descriptor, and disabling the encrypted connection check in the function (or changing the mocked connection so it looks like it's encrypted).

comment:9 in reply to:  8 Changed 3 years ago by teor

Replying to teor:

Replying to teor:

We should also add a case for #21471 (and a v3 HS descriptor GET and POST) to http.

Oh, and #21471 will require a GET request for a v3 HS descriptor, and disabling the encrypted connection check in the function (or changing the mocked connection so it looks like it's encrypted).

dgoulet wrote a unit test for this, so it's not as essential.

comment:10 Changed 3 years ago by teor

Please see my branch test21470-029, which tests #21278, #21450, and #21507.

And we should add the http corpus file for #20894 at:
https://github.com/teor2345/tor/blob/fuzz-dir-sensitive-v2/src/test/fuzz/fuzz_dir_testcase/dir-header-read-beyond-buffer.case

Once these are merged, we can close this ticket as done.

I want to defer fuzzing for issues with v3 hidden services (#21471) - I've opened a ticket to implement v3 descriptor fuzzing: #21509. We can do this as part of testing hidden services in 0.3.0 or 0.3.1.

comment:11 Changed 3 years ago by teor

Status: newneeds_review

comment:12 Changed 3 years ago by nickm

Keywords: 030-backport 029-backport added
Milestone: Tor: 0.3.1.x-finalTor: 0.3.0.x-final

I've merged test21470-029 to master, because #21507 isn't backported yet. Once it's backported, we can consider backporting this to 0.2.9 or 0.3.0.

I've added explicit regression-tests for #21278 and #20894 to our fuzzing corpora.

Moving this ticket to 0.3.0 for backport consideration.

comment:13 Changed 2 years ago by nickm

Keywords: test 030-backport 029-backporttest, 030-backport, 029-backport

comment:14 Changed 2 years ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final
Resolution: fixed
Status: needs_reviewclosed

no backport.

Note: See TracTickets for help on using tickets.