Opened 3 years ago

Closed 3 years ago

#19066 closed defect (fixed)

Wrong length used in networkstatus_parse_detached_signatures

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-2 TorCoreTeam201505
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:


While fixing #14013, nikkolasg realized thatif we did in fact use != DIGEST256_LEN it caused a failure in the test:

    // XXX Should it not be always DIGEST256_LEN ? Running the tests with
    // the condition ` != DIGEST256_LEN` fails.
    if (base16_decode(digests->d[alg], DIGEST256_LEN,
                      hexdigest, strlen(hexdigest)) < 0) {

Turns out that alg here is actually sha1 thus of size DIGEST_LEN. The base16 decode is safe with a larger length but this check (just above in the code) could resolved in unwanted behavior:

    if (!tor_mem_is_zero(digests->d[alg], DIGEST256_LEN)) {

tor_mem_is_zero does make sure that the full length is zeroes thus here looking for 12 extra bytes out of bound to be 0... (DIGEST_LEN vs DIGEST256_LEN). The length we used should be set according to the algorithm in alg

Patch coming soon.

Child Tickets

Change History (4)

comment:1 Changed 3 years ago by dgoulet

Status: newneeds_review

See branch for a potential fix. Test passes.


comment:2 Changed 3 years ago by nickm

Keywords: review-group-2 added

Create a review-group-2 from (most of the) tickets in 0.2.8 or 0.2.9 or 029-nickm-says-yes listed as needs_review,

comment:3 Changed 3 years ago by nickm

Reviewer: nickm

comment:4 Changed 3 years ago by nickm

Keywords: TorCoreTeam201505 added
Resolution: fixed
Status: needs_reviewclosed

works for me. merged! Also, in b53a2059c44959 I simplified the code by using a previously unexposed function from crypto.c. Tests still pass but somebody should make sure I didn't mess it up.

Note: See TracTickets for help on using tickets.