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

Use go-fuzz for improved testing #81

Closed
Metalnem opened this Issue Jul 20, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@Metalnem

Metalnem commented Jul 20, 2017

Using go-fuzz for around 30 minutes with just three small random PDF files, I was able to find multiple bugs in the PDF reader. You should probably fuzz it more and with improved corpus. For more details on fuzzing in Go, see this. My Fuzz function was looking like this:

func Fuzz(data []byte) int {
	b := bytes.NewReader(data)

	if _, err := model.NewPdfReader(b); err != nil {
		return 0
	}

	return 1
}
@gunnsth

This comment has been minimized.

Contributor

gunnsth commented Jul 20, 2017

We will definitely look into this. We already have a pretty big test corpus and handle a lot of all kinds of files, although most of our focus has been on files that are incompatible with the PDF standard but can still be opened by PDF readers as they are very forgiving. This will be a good chance to take our tests further and make more robust.

Many thanks. I had not heard of go-fuzz before, but it looks perfect for this.

@gunnsth

This comment has been minimized.

Contributor

gunnsth commented Jul 29, 2017

So far, beyond the 4 issues found and reported by @Metalnem initially:

#77
panic: interface conversion: pdf.PdfObject is nil, not *pdf.PdfObjectInteger bug
Fixed by commit 1769c68

#78
panic: runtime error: makeslice: len out of range
Fixed by commit 68bca16

#79
panic: runtime error: invalid memory address or nil pointer dereference bug
Fixed by 5f506b7

#80
runtime: goroutine stack exceeds 1000000000-byte limit bug
Fixed by commit 6e30b10

We have fuzzed, found and fixed additional 8 bugs (so total of 12 reported & fixed):

4698564
Fix fuzz-detected problem where stream length referring to stream itself (with a test case to verify)

b2bb6d9
Fixed problem found by fuzzing when Encrypt pointing to invalid reference

99872f2
Type check for xref Prev entries added.

124bd10
Check bounds on XRef stream index list (found by fuzzing)

3639b5c
Add check for ID array length

7f83a4e
Add bounds check in crypt, parser. Avoid division by zero. (3 fixes)

We have created a larger and more optimized test corpus and are in the process of running it on a pretty powerful machine for a few days. We will create an issue for each of the bugs found and refer to it here.

Our goal with this work is to make unidoc as stable as possible. Bugs like these can potentially be used for denial of service attacks. Fortunately, they cannot be used for remote code execution attacks.

@gunnsth gunnsth closed this Jun 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment