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

fix: make senc parsing more robust #401

Merged
merged 1 commit into from
Jan 19, 2025
Merged

fix: make senc parsing more robust #401

merged 1 commit into from
Jan 19, 2025

Conversation

tobbee
Copy link
Collaborator

@tobbee tobbee commented Jan 17, 2025

Added checks that values are OK in initial decode of SencBox.

Fixes #400, and makes the mentioned fuzz test obsolete.

@tobbee tobbee force-pushed the fix-senc-robustness branch from c7552cc to 7bb380f Compare January 17, 2025 14:24
@eric
Copy link
Contributor

eric commented Jan 18, 2025

Thanks for doing this!

@eric
Copy link
Contributor

eric commented Jan 18, 2025

Doing more fuzzing with these fixes I found the Size() method:

mp4ff/mp4/senc.go

Lines 285 to 298 in 7bb380f

func (s *SencBox) Size() uint64 {
if s.readButNotParsed {
return boxHeaderSize + 8 + uint64(len(s.rawData)) // read 8 bytes after header
}
totalSize := boxHeaderSize + 8
perSampleIVSize := s.GetPerSampleIVSize()
for i := 0; i < int(s.SampleCount); i++ {
totalSize += perSampleIVSize
if s.Flags&UseSubSampleEncryption != 0 {
totalSize += 2 + 6*len(s.SubSamples[i])
}
}
return uint64(totalSize)
}

was using a lot of CPU and it wasn't going into the s.Flags&UseSubSampleEncryption != 0 branch, so I assume it was 0.

You can replicate it by making mp4/testdata/fuzz/FuzzDecodeBox/0881196294cc083f containing:

go test fuzz v1
[]byte("\x00\x00\x00\x10senc\x00000\xfd\xfd\xfd\xbc")

and run:

$ go test -v -cpuprofile=cpu.pprof -run=FuzzDecodeBox/0881196294cc083f ./mp4

once this branch has been rebased off of the latest master.

github.com/Eyevinn/mp4ff/mp4.(*SencBox).Size
mp4/senc.go

  Total:       1.25s      1.26s (flat, cum) 96.18%
    291            .          .           		return boxHeaderSize + 8 + uint64(len(s.rawData)) // read 8 bytes after header 
    292            .          .           	} 
    293            .          .           	totalSize := uint64(boxHeaderSize + 8) 
    294            .          .            
    295            .          .           	perSampleIVSize := uint64(s.GetPerSampleIVSize()) 
    296        1.09s      1.10s           	for i := uint32(0); i < s.SampleCount; i++ { 
    297            .          .           		totalSize += perSampleIVSize 
    298        160ms      160ms           		if s.Flags&UseSubSampleEncryption != 0 { 
    299            .          .           			totalSize += 2 + 6*uint64(len(s.SubSamples[i])) 
    300            .          .           		} 
    301            .          .           	} 
    302            .          .           	return totalSize 
    303            .          .           } 

@tobbee tobbee force-pushed the fix-senc-robustness branch from 7bb380f to ae02564 Compare January 19, 2025 10:17
@tobbee
Copy link
Collaborator Author

tobbee commented Jan 19, 2025

OK. Thanks for the input. The senc box is a terrible box to parse since it is not self-contained but depends on field-sizes defined in other boxes. Anyway, I now store the read box size and use it for all parsing, while calculating the size when encoding the box. That gets rid of the loop for malformed boxes.

mp4/senc.go Outdated
Comment on lines 140 to 146
if version > 0 {
return nil, fmt.Errorf("version %d not supported", version)
}

if version > 0 {
return nil, fmt.Errorf("version %d not supported", version)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you accidentally duplicated this condition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok. thanks for pointing that out.

@tobbee tobbee force-pushed the fix-senc-robustness branch from ae02564 to 672e51c Compare January 19, 2025 21:47
@eric
Copy link
Contributor

eric commented Jan 19, 2025

I've run the fuzzer on this for 40 minutes and not found any additional issues with this box.

@tobbee tobbee force-pushed the fix-senc-robustness branch from 672e51c to 3262da0 Compare January 19, 2025 21:53
@tobbee tobbee merged commit 38ff9db into master Jan 19, 2025
9 checks passed
@tobbee tobbee deleted the fix-senc-robustness branch January 19, 2025 22:03
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.

SencBox.Size() can panic with non-zero SampleCount and empty rawData
2 participants