Opened 2 years ago

Closed 21 months ago

#23233 closed defect (fixed)

Unexpected BUG violation in hsv3 decriptor decoding

Reported by: haxxpop Owned by:
Priority: High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, easy, tor-hs, 031-backport
Cc: asn, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR-must


As you can see in hs_descriptor.c

  /* Find the start of signature. */
  sig_start = tor_memstr(encoded_desc, encoded_len, "\n" str_signature);
  /* Getting here means the token parsing worked for the signature so if we
   * can't find the start of the signature, we have a code flow issue. */
  if (BUG(!sig_start)) {
    goto err;

str_signature is "signature", so, if you send the "\n signature" (like in the attachment) instead of "\nsignature" tor_memstr will return null and violate BUG(!sig_start)

I suggest that we should just remove BUG and let it be if (!sig_start) {

Child Tickets

Attachments (1)

hsdesc_sample (13.8 KB) - added by haxxpop 2 years ago.

Download all attachments as: .zip

Change History (11)

Changed 2 years ago by haxxpop

Attachment: hsdesc_sample added

comment:1 Changed 2 years ago by nickm

Milestone: Tor: 0.3.2.x-final
Priority: MediumHigh
Sponsor: SponsorR-must

comment:2 Changed 2 years ago by asn

Status: newneeds_review

Please see branch bug23233 in my repo for the fix here.
It also adds a unittest for this specific bug.

comment:3 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

Looks okay to me, but: this if (BUG(!sig_start)) code is also present in 0.3.1. Should this be backported? If so it needs a changes file.

comment:4 Changed 2 years ago by dgoulet

Cc: dgoulet removed
Keywords: tor-hs 031-backport added


For the backport, I think we should do it because 031 is in -alpha and avoiding stacktrace in lots of log files because someone is having fun uploading malformed descriptors...

comment:5 Changed 2 years ago by asn

Status: needs_revisionneeds_review

Oops forgot the changes file! I pushed one now on the same branch.

Also, this bug is indeed present even in 0.3.0 IIRC. I wonder if we should backport this warning. I'd go for "no" since it's not a security issue, just a spammable warning. If you think otherwise, let me know and I will happily create 0.3.1 and 0.3.0 branches.

comment:6 Changed 2 years ago by dgoulet

Didn't realize it's in 030... I vote for 031 backport but not 030 which is stable already.

comment:7 Changed 2 years ago by asn

OK. Also pushed bug23233_031 which is based on maint-0.3.1!
Feel free to use it if you think backporting to 0.3.1 is good.

comment:8 Changed 2 years ago by nickm

ok, merged to 0.3.1.

comment:9 Changed 2 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.0.x-final

(We'll need to backport to 0.3.0 as well if it turns out that these warnings happen in practice: otherwise people will freak out.)

comment:10 Changed 21 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final
Resolution: fixed
Status: needs_reviewclosed

Not backporting, since people don't seem to be seeing these warnings.  We can reopen if they do.

Note: See TracTickets for help on using tickets.