Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22940 closed defect (fixed)

prop224: HS revision counter should persist after service reboot

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs prop224
Cc: Actual Points:
Parent ID: #20657 Points: 1
Reviewer: dgoulet Sponsor: SponsorR-can

Description (last modified by asn)

We currently have the following logic in the HSDir-side when accepting an HS descriptor:

    if (cache_entry->plaintext_data->revision_counter >=
        desc->plaintext_data->revision_counter) {
      log_info(LD_REND, "Descriptor revision counter in our cache is "
                        "greater or equal than the one we received. "
      goto err;

Unfortunately, while HSes keep track of the revision counter in memory, they never save it on disk, so if the service reboots the Hs will lose track of the rev counter and publish descriptors with small counters that will get rejected by the HSDir.

We have brainstormed the following solutions for this:
a) Save the latest rev counter in the state file in a form like this:
HidServRevCounter <service_pubkey> <rev_counter> <time_period_num>
b) Set the rev counter to a value like rounddown(time(NULL)) so that the HS always generates correct rev counters even without saving them on state.

However, wrt (b), since the quoted check above rejects equal rev counters the HS will have trouble uploading descriptors in the same hour. We could backport a fix that changes the check to 0.3.0 and then do this idea but that's kind of a PITA.

We are currently aiming for (a)

Child Tickets

Attachments (1)

ope_example.c (2.3 KB) - added by nickm 2 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 2 years ago by asn

Description: modified (diff)

comment:2 Changed 2 years ago by nickm

If you decide to do B, it would be a good application for an order-preserving encryption, which is kinda fun. I'll write up a short example.

Changed 2 years ago by nickm

Attachment: ope_example.c added

comment:3 Changed 2 years ago by nickm

Attached is a very silly order-preserving encryption example based on the scheme I've seen attributed to Bebek 02.

comment:4 in reply to:  3 Changed 2 years ago by asn

Replying to nickm:

Attached is a very silly order-preserving encryption example based on the scheme I've seen attributed to Bebek 02.

What's the bebek 02 paper btw? Is it Anti-Tamper Databases: Inference control techniques or another one?

comment:5 Changed 2 years ago by nickm

19:47 <+nickm> asn: Anti-tamper database research: Inference control techniques.
 Technical Report EECS 433 Final Report, Case Western Reserve University, Novemb
er 2002
19:47 <+nickm> I yoinked the scheme from

comment:6 Changed 2 years ago by asn

Status: newneeds_review

Hey David,

you can find the first version of this work on my bug22940 branch.

It does the revision counter state file business, and also fixes the over-rotation problem. There are no unittests yet, so it's not really production ready but definitely ready to be tested. I also need to update the state file docs to document the new entries.

comment:7 Changed 2 years ago by asn

Please find even better code at bug22940_v2.

It fixes a bad bug on the previous branch, fixes a broken unittest, and introduces unittests for the rev counter state file business.

Let me know if you need anything else here!

comment:8 Changed 2 years ago by asn

(FWIW, Nick's idea on comment:4 was not implemented, since apart from being pretty cutting edge, it also requires us to backport order-preserving encryption patches to 030 so that HSDirs can understand the encrypted counters. Let's re-consider this if we ever find the state-saving solution to be bad.)

comment:9 Changed 2 years ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision
  • Small fix for:
    +  log_info(LD_GENERAL, "Found rev counter for %s: %u",
    +           b64_key_str, (unsigned) rev_counter);
    +  log_info(LD_GENERAL, "[!] Adding rev counter %d for %s!",
    +           (int) rev_counter, blinded_pubkey_b64);

I would go for using PRIu64 instead of casting this and "possibly" reaching an overflow. I know unlikely but...

The rest looks good! Solid code!

comment:10 Changed 2 years ago by dgoulet

Resolution: fixed
Status: needs_revisionclosed

I fixed the above and merged the commits in #20657! woot

comment:11 Changed 2 years ago by nickm

AFAICT, the order-preserving backport wouldn't require any change in the HSDirs, since the hsdirs only require that the numbers be monotonically increasing 64-bit integers, and that's what you get from OPI, I believe. But I agree it sure isn't necessary for 0.3.2.

Note: See TracTickets for help on using tickets.