Opened 2 years ago

Closed 2 years ago

#22746 closed defect (implemented)

CID 1413651: No retval check in ed25519_donna_blind_public_key()

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: coverity
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor: SponsorR-can

Description

/src/ext/ed25519/donna/ed25519_tor.c: 307 in ed25519_donna_blind_public_key()
301       ed25519_donna_gettweak(tweak, param);
302       expand256_modm(t, tweak, 32);
303     
304       /* No "ge25519_unpack", negate the public key. */
305       memcpy(pkcopy, inp, 32);
306       pkcopy[31] ^= (1<<7);
>>>     CID 1413651:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "ge25519_unpack_negative_vartime" without checking return value (as is done elsewhere 4 out of 5 times).
307       ge25519_unpack_negative_vartime(&A, pkcopy);
308     
309       /* A' = [tweak] * A + [0] * basepoint. */
310       ge25519_double_scalarmult_vartime(&Aprime, &A, t, zero);
311       ge25519_pack(out, &Aprime);

Child Tickets

Change History (5)

comment:1 Changed 2 years ago by asn

Status: newneeds_review

Seems like the new code of #22006 inspired coverity to generate this warning.

Please check my branch bug22746 for a fix.

comment:2 Changed 2 years ago by nickm

Are these error paths tested already?

Does this need a changes file?

comment:3 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

No these new error paths are not tested. I think we need to provide a pubkey value that is not on the curve to the blinding function to make them fail. I'll try to do this.

And now that you mention it, I guess this needs a changes file as well, since the "bug" is on the blinding functions and not on the unreleased #22006 code (but I guess the retval checking of the #22006 code inspired coverity to find this bug).

comment:4 Changed 2 years ago by asn

Status: needs_revisionneeds_review

OK pushed two more commits on branch bug22746.
One commits adds unittests that trigger the error paths, the other commit adds a changes file.

The blinding functions will be used by prop224 client/service code which has not been merged yet, so the buggy code is not yet used in real life IIUC.

Placing this back in needs_review.

comment:5 Changed 2 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

looks good ; merged to master!

Note: See TracTickets for help on using tickets.