Opened 2 years ago

Closed 2 years ago

#22103 closed defect (fixed)

confparse.c checks pointer instead of value (!ok)

Reported by: nullius Owned by: catalyst
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Description

In src/or/confparse.c, functions conf_parse_msec_interval() and conf_parse_interval() incorrectly check a pointer instead of the pointed-to value. Patch attached.

Impact

When config_parse_units() hits an error, these functions may return 0 as a valid value instead of -1 as an error.

Security evaluation

Far worse could be done by any attacker with sufficient access to feed malicious data to these functions. Thus, I don’t see how it could be exploited as a practical matter.

note[0]

From my ~/tor/BUGS.txt with mtime 2014-03-19T03:07:45Z. So sorry I did not report it sooner. I could have been rich and famous!

note[1]

Use of the variable ok is inconsistent in confparse.c. In config_assign_value(), ok is an int. Elsewhere, pointer to int. That’s not ok! Also, there is a confusing tor_assert(ok); to check for non-NULL pointer; KNF style would prescribe the check to be explicit tor_assert(ok != NULL);, for a reason. (The actual bug concerns a Boolean check, so if (!*ok) is stylistically sane.) I could open a separate bug and/or do some minor refactoring, if committers were to express an interest in making ok more ok.

Child Tickets

Attachments (1)

confparse.c.patch (658 bytes) - added by nullius 2 years ago.
Patch for src/or/confparse.c [c2947db]

Download all attachments as: .zip

Change History (18)

Changed 2 years ago by nullius

Attachment: confparse.c.patch added

Patch for src/or/confparse.c [c2947db]

comment:1 Changed 2 years ago by catalyst

Owner: set to catalyst
Status: newassigned

comment:2 Changed 2 years ago by dgoulet

Component: Core TorCore Tor/Tor
Milestone: Tor: 0.3.1.x-final

comment:3 Changed 2 years ago by catalyst

Do we want to fix this for 0.2.9.x? This is a quite old bug, dating back to code in config.c (8acaf8e1872f711898e850687ccf55a196dc1312) that later got moved to confparse.c.

comment:4 Changed 2 years ago by dgoulet

+1 for backport up to 029.

Let me take this back. The calls to config_parse_msec_interval() and config_parse_interval() all check correctly the "ok" value so seems benign after all thus no backport.

Last edited 2 years ago by dgoulet (previous) (diff)

comment:5 Changed 2 years ago by nickm

patch above looks fine. Does anybody have a few minutes to write a changes file?

comment:6 Changed 2 years ago by catalyst

Based on IRC discussion this might not even be a bug. In that case, it would make more sense to just delete the offending lines. It will take me a little more time for me to grok enough config parsing stuff to make a test case. (It might be faster to do a manual test.)

comment:7 Changed 2 years ago by catalyst

Status: assignedaccepted

I am almost certain this is not a bug for reasons dgoulet and I discussed in IRC. With this config line

HeartbeatPeriod 21 snarks

I get

May 01 13:45:21.694 [notice] Tor 0.3.0.5-rc (git-5eb2786600014d02) running on Darwin with Libevent 2.0.22-stable, OpenSSL 1.0.2k and Zlib 1.2.5.
May 01 13:45:44.865 [warn] Unknown unit 'snarks'.
May 01 13:45:44.865 [warn] Failed to parse/validate config: Interval 'HeartbeatPeriod 21 snarks' is malformed or out of bounds.

Deleting the offending 4 lines of code makes more sense so it won't be doing useless checks that "look wrong", so I'll do that.

comment:8 Changed 2 years ago by catalyst

Status: acceptedneeds_review

Patch at https://gitlab.com/argonblue/tor/merge_requests/7. Does anyone think I should make a unit test for the failure mode that was suspected to occur (but didn't)?

comment:9 Changed 2 years ago by nickm

it might be nice to do that -- it's generally good to err on the side of more tests.

comment:10 Changed 2 years ago by nickm

Status: needs_reviewmerge_ready

(otherwise, this looks fine. let me know if you decide to add a test or not?)

comment:11 Changed 2 years ago by catalyst

Looks like I'll have to write some new test support code in test_options.c, but I'm going to go ahead and try adding some tests.

comment:12 Changed 2 years ago by nickm

Status: merge_readyneeds_revision

Okay; please put this back in merge_ready when you've got the tests.

comment:13 Changed 2 years ago by catalyst

Status: needs_revisionmerge_ready

Thanks. Updated with tests.

comment:14 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

lgtm; tests pass; merging!

comment:15 Changed 2 years ago by catalyst

Resolution: fixed
Status: closedreopened

Code movement for the test uncovered a latent memory management bug. Let's not reuse variables (especially pointers to allocated memory!) for multiple unrelated purposes. Proposed fix in https://gitlab.com/argonblue/tor/merge_requests/8

comment:16 Changed 2 years ago by catalyst

Status: reopenedneeds_review

comment:17 Changed 2 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Good find; merging!

We do this kind of variable reuse all over the unit tests now, I fear -- we should try to fix this pattern whenever we run into it, though, because it is indeed error-prone.

Note: See TracTickets for help on using tickets.