Opened 9 months ago

Closed 9 months ago

#20717 closed defect (fixed)

Hashing api should return negative values for errors

Reported by: chelseakomlo Owned by: chelseakomlo
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-backport, review-group-13
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In crypto.c, the hashing API currently returns 1 for error, 0 on success. Specifically in: crypto_digest, crypto_digest256, and crypto_digest512.

This is often misinterpreted in calling functions. For example, crypto_pk_get_digest and crypto_pk_private_sign_digest both incorrectly assume negative error return values.

The fix for this should refactor to return -1 on error, and change calling functions that currently expect 1 on error, such as crypto_expand_key_material_TAP and crypto_pk_get_hashed_fingerprint.

Child Tickets

Change History (7)

comment:1 Changed 9 months ago by chelseakomlo

I have a patch for this, will submit shortly.

comment:2 Changed 9 months ago by chelseakomlo

I have a patch for this here: git@github.com:chelseakomlo/tor_patches.git, branch 20717_hashing_api_bug

My concern about this fix is that there are no tests to validate this behavior. I wasn't sure how to write test cases that would either 1) force the error case to happen, or 2) mock the hashing functions that are wrapped.

Any suggestions would be great- if you want to go ahead and merge this code, that is fine too, we can think about longer-term options to make sure we don't see these kinds of bugs in the future.

comment:3 Changed 9 months ago by chelseakomlo

Status: newneeds_review

comment:4 Changed 9 months ago by nickm

Keywords: review-group-13 added

comment:5 Changed 9 months ago by nickm

Owner: set to chelseakomlo
Status: needs_reviewassigned

setting owner.

comment:6 Changed 9 months ago by nickm

Status: assignedneeds_review

comment:7 Changed 9 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged it!
(plus daeb633825920ca99830c75a79d9a7d4ed211a13 for a whitespace fix and 41adfd6fa38bf94bc7d71174dbaf7f32a41a64ec where I found a couple more calls.)

Note: See TracTickets for help on using tickets.