Page MenuHomeFreeBSD
Differential D16000

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

Authored by aniketp on Jun 24 2018, 9:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 29, 9:23 AM
Unknown Object (File)
Fri, Mar 28, 9:02 PM
Unknown Object (File)
Thu, Mar 6, 8:40 PM
Unknown Object (File)
Feb 23 2025, 2:24 PM
Unknown Object (File)
Feb 20 2025, 1:44 AM
Unknown Object (File)
Feb 17 2025, 2:29 AM
Unknown Object (File)
Feb 3 2025, 8:46 AM
Unknown Object (File)
Feb 3 2025, 6:31 AM
View All 80 Files
Subscribers
imp

Details

Reviewers
asomers
rwatson
gnn
Commits
rS335703: audit(4): fix Coverity issues
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

aniketp created this revision.Jun 24 2018, 9:10 PM
Harbormaster completed remote builds in B17599: Diff 44399.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!]

Harbormaster completed remote builds in B17606: Diff 44416.Jun 25 2018, 11:04 AM
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

Harbormaster completed remote builds in B17654: Diff 44506.Jun 27 2018, 4:30 AM
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
Closed by commit rS335703: audit(4): fix Coverity issues (authored by asomers). · Explain WhyJun 27 2018, 3:28 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptJun 27 2018, 3:28 PM

Revision Contents
Changeset List

PathSize
head/
tests/
sys/
audit/
2 lines
2 lines
2 lines

Diff 44530

View Options

head/tests/sys/audit/administrative.c

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines
View Options

head/tests/sys/audit/file-attribute-access.c

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines
View Options

head/tests/sys/audit/file-attribute-modify.c

Show First 20 Lines • Show All 1,629 Lines • ▼ Show 20 Lines#endif
* fall back to non RFC1323 RTT calculation. Normalize * fall back to non RFC1323 RTT calculation. Normalize
* timestamp if syncookies were used when this connection * timestamp if syncookies were used when this connection
* was established. * was established.
*/ */
if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) { if ((to.to_flags & TOF_TS) && (to.to_tsecr != 0)) {
to.to_tsecr -= tp->ts_offset; to.to_tsecr -= tp->ts_offset;
if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) { if (TSTMP_GT(to.to_tsecr, tcp_ts_getticks())) {
to.to_tsecr = 0; to.to_tsecr = 0;
} else if (tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) {
cc_cong_signal(tp, th, CC_RTO_ERR);
} }
} }
/* /*
* Process options only when we get SYN/ACK back. The SYN case * Process options only when we get SYN/ACK back. The SYN case
* for incoming connections is handled in tcp_syncache. * for incoming connections is handled in tcp_syncache.
* According to RFC1323 the window field in a SYN (i.e., a <SYN> * According to RFC1323 the window field in a SYN (i.e., a <SYN>
* or <SYN,ACK>) segment itself is never scaled. * or <SYN,ACK>) segment itself is never scaled.
* XXX this is traditional behavior, may need to be cleaned up. * XXX this is traditional behavior, may need to be cleaned up.
▲ Show 20 Lines • Show All 136 Lines • ▼ Show 20 Lines if (tlen == 0) {
(to.to_flags & TOF_SACK) == 0 && (to.to_flags & TOF_SACK) == 0 &&
TAILQ_EMPTY(&tp->snd_holes)) { TAILQ_EMPTY(&tp->snd_holes)) {
/* /*
* This is a pure ack for outstanding data. * This is a pure ack for outstanding data.
*/ */
TCPSTAT_INC(tcps_predack); TCPSTAT_INC(tcps_predack);
/* /*
* "bad retransmit" recovery without timestamps. * "bad retransmit" recovery.
*/ */
if ((to.to_flags & TOF_TS) == 0 && if (tp->t_rxtshift == 1 &&
tp->t_rxtshift == 1 &&
tp->t_flags & TF_PREVVALID && tp->t_flags & TF_PREVVALID &&
tp->t_badrxtwin != 0 && tp->t_badrxtwin != 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin)) { (((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr != 0 &&
TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
((to.to_flags & TOF_TS) == 0 &&
TSTMP_LT(ticks, tp->t_badrxtwin))))
cc_cong_signal(tp, th, CC_RTO_ERR); cc_cong_signal(tp, th, CC_RTO_ERR);
}
/* /*
* Recalculate the transmit timer / rtt. * Recalculate the transmit timer / rtt.
* *
* Some boxes send broken timestamp replies * Some boxes send broken timestamp replies
rscheffUnsubmitted
Done
Inline Actions

Any specific reason to write the or statement as two separate if clauses?

I believe in other parts we do it ((true & x & y & z) | (false & a & b &c )) - as multiline checks;

I don't like them both get executed "always", so at the very least, an "else" just before the 2nd if would be good.

The logic looks ok to me.

rscheff: Any specific reason to write the or statement as two separate if clauses? I believe in other…
glebiusUnsubmitted
Done
Inline Actions

I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as one wants TOF_TS and other !TOF_TS, so cc_cong_signal() can be executed only once. My first reading was that it is intentionally written this way cause we may want two calls into cc_cong_singal(), so it was a misleading reading.

Oh, the upper level embracing if can also be merged into one big clause:

if ((tp->t_rxtshift == 1 &&
    tp->t_flags & TF_PREVVALID &&
    tp->t_badrxtwin != 0) &&
    (((to.to_flags & TOF_TS) != 0 &&
    (to.to_tsecr != 0) &&
    TSTMP_LT(to.to_tsecr, tp->t_badrxtwin)) ||
    ((to.to_flags & TOF_TS) == 0 &&
    TSTMP_LT(ticks, tp->t_badrxtwin))))
        cc_cong_signal(tp, th, CC_RTO_ERR);

This will also fix lines longer than 79 chars. IMHO, we may slightly violate style(9) and add single space indentation to highlight our three main large blocks (a && (b || c)).

glebius: I agree with Richard, better one complex if clause. The two clauses are mutually exclusive as…
tuexenAuthorUnsubmitted
Done
Inline Actions

Now using one and two spaces of indentation to make the expression more readable.

tuexen: Now using one and two spaces of indentation to make the expression more readable.
tuexenAuthorUnsubmitted
Done
Inline Actions

It was a try to break things up for improved readability by humans.

tuexen: It was a try to break things up for improved readability by humans.
* during the SYN+ACK phase, ignore * during the SYN+ACK phase, ignore
* timestamps of 0 or we could calculate a * timestamps of 0 or we could calculate a
* huge RTT and blow up the retransmit timer. * huge RTT and blow up the retransmit timer.
*/ */
if ((to.to_flags & TOF_TS) != 0 && if ((to.to_flags & TOF_TS) != 0 &&
to.to_tsecr) { to.to_tsecr) {
uint32_t t; uint32_t t;
▲ Show 20 Lines • Show All 2,424 Lines • Show Last 20 Lines