Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4650 closed defect (fixed)

DisableDebuggerAttachment pretends to be disabled if you change it and hup

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: ioerror Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The feature from #3313 does a prctl or ptrace call if it's on during options_act(), and does a log_notice if it's off during options_act.

So I can change it to off, hup, and get the log_notice saying attaching should work. But it doesn't actually do anything to make it work again.

rransom points out quite rightly that our process isn't *supposed* to be able to do anything to make it work again. So the correct fix is to stop deceiving the user. Perhaps we should make it an invalid transition in options_transition_allowed()?

Also, should we really be calling tor_disable_debugger_attach() on every hup? Seems like the sort of thing that we should have a static variable to remember.

And finally (and related), if the option is off, you get your log_notice on every hup whether anything changed or no.

Child Tickets

Change History (15)

comment:1 Changed 8 years ago by ioerror

It seems to make sense that you should not be allowed to toggle this option.

comment:2 Changed 8 years ago by Sebastian

branch bug4650 in my repo

comment:3 Changed 8 years ago by arma

Status: newneeds_review

comment:4 Changed 8 years ago by arma

a) still logs a notice every time you hup or setconf if it's off

b) still calls the function every time you hup or setconf if it's on

c) i think we should permit a transition to having it on; just not permit a transition back?

comment:5 Changed 8 years ago by Sebastian

I think you're wrong about a) and b). Did you *test* it and that indicated otherwise? Because it doesn't happen in my testing. I chose c) deliberately so I wouldn't have to explain a bunch more in the manpage and also because I didn't know what would happen if we already have a debugger attached and then the option changes, on the various platforms. Going with completely disallowing it seemed wise. If you still think c) should be otherwise, I'm happy to revise this.

comment:6 Changed 8 years ago by nickm

Re c) I too think we should allow it to transition from "off" to "on".

comment:7 Changed 8 years ago by Sebastian

Hrm. Maybe we should never log a notice, because it is a lie on current Ubuntu. We never do anything to actually *allow* you to attach a debugger, and doing so is disabled by Ubuntu by default. Unless we should change the implementation to set the right prctl (and someone can confirm this works on Ubuntu)

comment:8 in reply to:  7 Changed 8 years ago by nickm

Replying to Sebastian:

Hrm. Maybe we should never log a notice, because it is a lie on current Ubuntu. We never do anything to actually *allow* you to attach a debugger, and doing so is disabled by Ubuntu by default. Unless we should change the implementation to set the right prctl (and someone can confirm this works on Ubuntu)

Maybe we can come up with a better warning, then? Generally having a safety feature that's turned off is something that we want to tell people about.

comment:9 Changed 8 years ago by Sebastian

Updated the branch with a fixup commit. How does it look now?

comment:10 Changed 8 years ago by nickm

We want to look at the return value for disable_debugger_attachment, I think: It's possible for us to call that function and fail, or call it but never actually try to disable debugger attachment because the system doesn't support it.

comment:11 Changed 8 years ago by Sebastian

Yeah, we need to consider windows here, too. We probably warn on it by default about "Unable to disable ptrace attach" currently, if I'm reading the code right. grngl

comment:12 Changed 8 years ago by nickm

Have a look at branch bug4650_nm in my public repository?

comment:13 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Still looks fine to me. Merging branch bug4650_nm_squashed.

comment:14 Changed 7 years ago by nickm

Keywords: tor-client added

comment:15 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.