Opened 5 weeks ago

Closed 3 days ago

#31682 closed defect (fixed)

CID 1453653: Integer handling (NEGATIVE_RETURNS) in build_establish_intro_dos_extension()

Reported by: teor Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop305, coverity 042-should
Cc: asn Actual Points: 0.1
Parent ID: #29999 Points: 0.1
Reviewer: asn Sponsor: Sponsor27-must

Description

trn_cell_extension_dos_encoded_len() returns ssize_t, but trn_cell_extension_field_setlen_field() takes size_t.
This looks like a bug on #30924, copying sponsor fields across.

/src/feature/hs/hs_cell.c: 532 in build_establish_intro_dos_extension()
528       /* Set the field with the encoded DoS extension. */
529       dos_ext_encoded_len = trn_cell_extension_dos_encoded_len(dos_ext);
530       /* Set length field and the field array size length. */
531       trn_cell_extension_field_set_field_len(field, dos_ext_encoded_len);
   CID 1453653:  Integer handling issues  (NEGATIVE_RETURNS)
   "dos_ext_encoded_len" is passed to a parameter that cannot be negative.
532       trn_cell_extension_field_setlen_field(field, dos_ext_encoded_len);
533       /* Encode the DoS extension into the cell extension field. */
534       field_array = trn_cell_extension_field_getarray_field(field);

Child Tickets

Change History (7)

comment:1 Changed 5 weeks ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:2 Changed 5 weeks ago by nickm

Keywords: 042-should added

comment:3 Changed 12 days ago by dgoulet

Actual Points: 0.1
Cc: dgoulet removed
Points: 0.1
Reviewer: asn
Status: acceptedneeds_review

PR: https://github.com/torproject/tor/pull/1388
Branch: ticket31682_042_01

comment:4 in reply to:  3 Changed 7 days ago by asn

Status: needs_reviewneeds_revision

Replying to dgoulet:

PR: https://github.com/torproject/tor/pull/1388
Branch: ticket31682_042_01

Hmm, not fully satisfied with the added:
tor_assert(ret > 0); in this branch.

I know it's safe, but trn_cell_extension_dos_encoded_len() explicitly returns -1 in case of a bad object, so I think it's not right to assert that the retval is gonna be positive. Also this might just cause another coverity warning in the future.

Perhaps we can turn build_establish_intro_dos_extension() into an int-returning function and do proper error checking on that function?

comment:5 Changed 6 days ago by dgoulet

Status: needs_revisionneeds_review

Fixup commit pushed!

comment:6 Changed 4 days ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:7 Changed 3 days ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged. I also fixed the comment on build_establish_intro_dos_extension: it said "this can't fail", but now it can.

Note: See TracTickets for help on using tickets.