Opened 2 years ago

Closed 21 months ago

#24001 closed enhancement (implemented)

node_get_ed25519_id() should check if the microdesc ed25519 id is all zero

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy intro
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

Currently we only do this for the ri ed25519 id.
If that check is already done when we parse microdescs, we should add a comment.

Child Tickets

Attachments (2)

0001-Bug-24001-Comment-added-regarding-the-function.patch (665 bytes) - added by aruna1234 21 months ago.
0001-Conditional-check-added.patch (738 bytes) - added by aruna1234 21 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 2 years ago by dgoulet

Agree with this. It would help to have a stronger assumption on the returned key that it is in fact valid (non zero) all the time.

comment:2 Changed 22 months ago by teor

Keywords: easy intro added

comment:3 Changed 22 months ago by teor

Parent ID: #23975

Maybe we should do this as part of the consistency changes in #23975.

comment:4 in reply to:  description Changed 21 months ago by aruna1234

Replying to teor:

Currently we only do this for the ri ed25519 id.
If that check is already done when we parse microdescs, we should add a comment.

I think the check happens, only a comment is required.

comment:5 Changed 21 months ago by nickm

In C, doing an "if (ptr)" check on a pointer checks whether the pointer is NULL, not whether the memory that the pointer points to is zero.

comment:6 Changed 21 months ago by teor

Status: newneeds_revision

I think it would be a good idea to check both the ri and md ed25519 ids to see if the memory they point to is zero.

Can you copy the check from the ri ed25519 id to the md ed25519 id?

comment:7 in reply to:  5 Changed 21 months ago by aruna1234

Replying to nickm:

In C, doing an "if (ptr)" check on a pointer checks whether the pointer is NULL, not whether the memory that the pointer points to is zero.

If that's the case then I think this would work:
if(*(node->ri))

comment:8 Changed 21 months ago by teor

That checks if the first byte of the object is zero. We need to check if every byte is zero.
We have a function for that, I think it's called tor_mem_is_zero().

comment:9 in reply to:  8 Changed 21 months ago by aruna1234

Replying to teor:

That checks if the first byte of the object is zero. We need to check if every byte is zero.
We have a function for that, I think it's called tor_mem_is_zero().

I added this,
if(tor_mem_is_zero((char*)node->md, sizeof(node->md->ed25519_identity_pkey)))

however it is failing when I do a make check at src/test/test.

comment:10 in reply to:  6 Changed 21 months ago by aruna1234

Replying to teor:

I think it would be a good idea to check both the ri and md ed25519 ids to see if the memory they point to is zero.

Can you copy the check from the ri ed25519 id to the md ed25519 id?

also did this,

if (BUG(ed25519_public_key_is_zero(md_pk)))

md_pk = NULL;

this is passing all tests when I run a make check

comment:11 Changed 21 months ago by teor

Please attach a working patch that contains all your changes

Changed 21 months ago by aruna1234

comment:12 Changed 21 months ago by teor

Status: needs_revisionneeds_review
Type: defectenhancement

Please see my branch ticket24001 at https://github.com/teor2345

I moved and expanded the comment, and added a changes file.

comment:13 Changed 21 months ago by teor

Status: needs_reviewmerge_ready

comment:14 Changed 21 months ago by teor

Parent ID: #23975

(This is unrelated to #23975.)

comment:15 in reply to:  12 Changed 21 months ago by aruna1234

Replying to teor:

Please see my branch ticket24001 at https://github.com/teor2345

I moved and expanded the comment, and added a changes file.

okay!

comment:16 Changed 21 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged to master!

Note: See TracTickets for help on using tickets.