Opened 6 weeks ago

Last modified 5 weeks ago

#23233 needs_review defect

Unexpected BUG violation in hsv3 decriptor decoding

Reported by: haxxpop Owned by:
Priority: High Milestone: Tor: 0.3.0.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

Description

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 6 weeks ago.

Download all attachments as: .zip

Change History (10)

Changed 6 weeks ago by haxxpop

Attachment: hsdesc_sample added

comment:1 Changed 6 weeks ago by nickm

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

comment:2 Changed 5 weeks 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 5 weeks 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 5 weeks ago by dgoulet

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

ACK.

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 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks ago by nickm

ok, merged to 0.3.1.

comment:9 Changed 5 weeks 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.)

Note: See TracTickets for help on using tickets.