Page MenuHomeFreeBSD

Fix wrong sizeof() argument and descriptor leak reported by Coverity
ClosedPublic

Authored by aniketp on Jun 24 2018, 9:10 PM.

Details

Summary

This revision fixes the resource leak and possible buffer overflow due to incorrect
sizeof(buf) argument passed to extattr_set_{file/fd/link}. Approach is to set the
extended attribute authorname to NULL and pass the size_t nbytes argument as 0.

CID List:

CID 1393489
CID 1393501
CID 1393509
CID 1393510
CID 1393514
CID 1393515
CID 1393516
CID 1393517
CID 1393518
CID 1393519

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aniketp created this revision.Jun 24 2018, 9:10 PM
asomers requested changes to this revision.Jun 25 2018, 12:39 AM
asomers added inline comments.
tests/sys/audit/file-attribute-access.c
818 ↗(On Diff #44399)

Technically this would fix the problem. But this technique obviously wouldn't work if you actually cared about the extended attribute. As an educational exercise, try fixing it the right way. Hint: you could define buff as an array rather than a pointer.

This revision now requires changes to proceed.Jun 25 2018, 12:39 AM
aniketp updated this revision to Diff 44416.EditedJun 25 2018, 11:04 AM

Declare buff as an array instead of a pointer, to be able to correctly pass the value to nbytes argument for extattr_set_*

[Much better than the earlier fix, Thanks!]

asomers requested changes to this revision.Jun 26 2018, 8:37 PM

This is still incorrect. sizeof(buff) returns 80. That means that extattr_set_file is going to set the attribute's value to ezio\0 followed by 75 bytes of stack garbage. You might try leaving buff's length unspecified, by declaring it like static char buff[] = "ezio";. Whatever you do, ensure that the nbytes argument is either 4 (if you don't desire NULL termination) or 5 (if you do).

This revision now requires changes to proceed.Jun 26 2018, 8:37 PM
aniketp updated this revision to Diff 44506.Jun 27 2018, 4:30 AM

Remove 80 bytes hardcoded initialization from buff array

asomers accepted this revision.Jun 27 2018, 2:56 PM
This revision is now accepted and ready to land.Jun 27 2018, 2:56 PM
This revision was automatically updated to reflect the committed changes.