Page MenuHomeFreeBSD

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

Authored by aniketp on Jun 24 2018, 9:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 10:10 PM
Unknown Object (File)
Mon, Mar 11, 2:56 PM
Unknown Object (File)
Mon, Mar 11, 2:56 PM
Unknown Object (File)
Dec 20 2023, 12:13 AM
Unknown Object (File)
Dec 10 2023, 8:52 PM
Unknown Object (File)
Nov 4 2023, 4:36 PM
Unknown Object (File)
Sep 18 2023, 4:21 AM
Unknown Object (File)
Sep 18 2023, 4:19 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

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

Remove 80 bytes hardcoded initialization from buff array

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.