koush / Superuser

Other
1.48k stars 999 forks source link

Key input in su shell #177

Open simeonkr opened 11 years ago

simeonkr commented 11 years ago

Starting from the new 4.3 compatible version su priveliged shells are blocking certain inputs such as the Ctrl+W combination in Terminal Emulator (and the corresponding up key in adb shell). Instead of jumping to the previous command such input outputs this: ^[[A

koush commented 11 years ago

it's probably an issue with how i set up the pseudoterminal. there's also echoing happening on commands. haven't been able to figure it out.

alieskills commented 11 years ago

Thanx

alieskills commented 11 years ago

Thanx

tan-ce commented 11 years ago

I'm currently working to fix this on my fork of Superuser. Will update once I get it working.

Meanwhile, I also have another project on github (pts-multi) which I'm currently using for my root terminals.

Koush, If I understand your code correctly, you're passing std{in|out|err} between the su client and daemon via FIFOs opened at $REQUESTOR_DAEMON_PATH/$PID.std{in|out|err}, is that right? What do you think about passing this input over the unix socket instead? Then we wouldn't have to open additional FIFOs, which I think would be more secure. I could probably code this change as well, if it seems good to you.

koush commented 11 years ago

Using a single unix socket will require framing and (de)multiplexing. Not to mention the locking/coopting that will need to happen between multiple fds (unless a switch is made to a reactor thread/epoll model). With my solution, you can essentially just dup2 the fd and exec in the daemon.

There's no security benefit with your solution, as you imply, as the fifos are protected by the filesystem permissions. Correct me if I'm mistaken.

I'd considered using your suggested approach, but the benefits of using a single pipe were, IMO, not worth the the cost in performance (multiplexing) and increased complexity of the code.

tan-ce commented 11 years ago

Security-wise I was thinking about a time-of-creation to time-of-use loophole, but you're right. If an attacker already has filesystem permissions to modify /dev, that person already has root.

By the way, I've also just finished modifications to daemon.c which attempts to correct some of the problems with the terminal handling. I've tested it and the stdin issues seems fixed. There is some extra complexity because I wanted su to choose whether to use FIFOs or the pseudo-terminal based on whether su was being used interactively or in something akin to a shell pipe. The commit is in my fork of the project: https://github.com/tan-ce/Superuser/commit/63485abe62002a80370e2b329fa1f3cff744eba1

Do give me your comments on it. One thing that isn't working yet is trapping SIGWINCH and executing the corresponding ioctl on the pseudo-terminal so that terminal-resizing works as expected.

koush commented 11 years ago

@tan-ce Will take a look, thanks

koush commented 11 years ago

@tan-ce Looks good at initial glance, mind submitting a pull request so I can do a proper code review w/ comments?