Page MenuHomeFreeBSD

cd9660: Add various length checks when parsing RRIP extensions
Needs ReviewPublic

Authored by jhb on Wed, May 20, 7:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 5, 3:30 PM
Unknown Object (File)
Thu, Jun 4, 3:33 AM
Unknown Object (File)
Fri, May 29, 2:58 PM
Unknown Object (File)
Fri, May 29, 9:57 AM
Unknown Object (File)
Thu, May 28, 7:35 PM
Unknown Object (File)
Thu, May 28, 7:34 PM
Unknown Object (File)
Wed, May 27, 1:57 AM
Unknown Object (File)
Tue, May 26, 6:15 PM
Subscribers

Details

Reviewers
emaste
des
Summary

Pass the length of a RockRidge attribute to the handler functions and
validate that length in each handler. If a parsing error is detected,
abort the entire parsing pass.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 73803
Build 70686: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Wed, May 20, 7:09 PM

While the previous patch in this stack fixes a reproducer from Robert, this followup patch is intended as additional robustness. With both of these I was able to still mount a FreeBSD install CD and all of the attributes reported by find /mnt -ls were the same before and after the patches.

des added inline comments.
sys/fs/cd9660/cd9660_rrip.c
146–155

why is this not the loop initialization?

sys/fs/cd9660/cd9660_rrip.c
516–519

Ought to have the check before p->len_id above

sys/fs/cd9660/cd9660_rrip.c
146–155

Because it isn't the thing being iterated (pcomp) which would be the normal thing to set in the initializer. This is more like a TAILQ_FOREACH_SAFE(). The assignment here is more to pacify the compiler, but it isn't needed as the loop body always initializes pcompn before it can be used in the last clause. I haven't actually tried building without this assignment, perhaps modern compilers are smart enough to not warn?

516–519

Oops, yes.

modulo fix for the issue @emaste raised

This revision is now accepted and ready to land.Fri, Jun 5, 4:20 PM
This revision now requires review to proceed.Wed, Jun 10, 2:12 PM