Opened 3 years ago

Closed 3 years ago

#19561 closed defect (fixed)

Misleading prop250 log messages

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-sr
Cc: Actual Points:
Parent ID: Points: 0.3
Reviewer: Sponsor: SponsorR-must

Description

There are various instances where we combine sr_commit_get_rsa_fpr() with hex_str() in log messages. This won't work because sr_commit_get_rsa_fpr() actually calls hex_str() underneath, and you can't have repeated calls to hex_str() because it uses a stack buffer.

Examples:

  log_debug(LD_DIR, "SR: Inspecting commit from %s (voter: %s)?",
            sr_commit_get_rsa_fpr(commit),
            hex_str(voter_key, DIGEST_LEN));
    if (!commitments_are_the_same(commit, saved_commit)) {
      log_warn(LD_DIR, "SR: Commit from authority %s is different from "
                       "previous commit in our state (voter: %s)",
               sr_commit_get_rsa_fpr(commit),
               hex_str(voter_key, DIGEST_LEN));
      goto ignore;
    }
    if (verify_commit_and_reveal(commit) < 0) {
      log_warn(LD_BUG, "SR:a Commit from authority %s has an invalid "
                       "reveal value. (voter: %s)",
               sr_commit_get_rsa_fpr(commit),
               hex_str(voter_key, DIGEST_LEN));
      goto ignore;
    }

I found this issue while auditing log messages that did not make sense in a Chutney network.

In general, it's naughty to hide a delicate function like hex_str() behind other functions (like sr_commit_get_rsa_fpr()) without making it clear that this is the case.

Fortunately, from what I see, this bug only occurs in logging cases. But we should make sure we don't have this sort of issue from now on.

Child Tickets

Change History (6)

comment:1 Changed 3 years ago by asn

Possible fix: Have a const char * hex-encoded encoding of the RSA identity in sr_commit_t and return that when calling sr_commit_get_rsa_fpr(), instead of preparing it on the spot with hex_str().

Drawback: It's a non-trivial change.

Is there a more elegant trivial fix we could do here?

Last edited 3 years ago by asn (previous) (diff)

comment:2 Changed 3 years ago by dgoulet

Cc: dgoulet removed
Keywords: tor-sr added; tor-prop250 removed

I would be fine with this extra hex encoded identity digest in the commit because we use it quite often for logging and vote/state creation. We set it only once at the commit creation in commit_new(). Easy addition because we don't need to free it and it's immutable once the commit object has been created.

sr_commit_get_rsa_fpr() returns the const pointer and we win. I see that as a simple change that we should do now instead of once dirauth uses SR with 029 stable. We can deploy it in our test net also prior to deployment.

comment:3 Changed 3 years ago by dgoulet

Status: newneeds_review

Ok asn was in agreement with what I proposed. Here is the branch: bug19561_029_01.

comment:4 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Patch looks good to me!

BTW, is there a reason for sr_commit_get_rsa_fpr() to be in a .h file?

comment:5 in reply to:  4 Changed 3 years ago by dgoulet

Replying to asn:

BTW, is there a reason for sr_commit_get_rsa_fpr() to be in a .h file?

static inline which is also used in shared_random_state.c. Could be a function, or even a macro tbh, no strong opinion on it.

comment:6 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.