Comment 16 for bug 1625074

I reviewed ubuntu-terminal-app version 0.7.216 as checked into yakkety.
This should not be considered a full security review.

The pieces of this program that I've read so far give me a mixed
impression. It appears that sections of Qt and KDE frameworks are pulled
into this source package rather than using the packages in the archive. I
suspect much of this code aren't actually used in the terminal emulator.
Its presence here is definitely distracting, and if there is a way to
reach the code then those bugs become our bugs to fix twice.

The mixing is especially complicated because the terminal and process
handling routines are some of the most complicated code to make work
cross-platform -- why, for example, does this package need to know how
to start terminals on IRIX or read OpenBSD MIB tables via SNMP?

Whatever modifications are needed to Qt and KDE frameworks should be
handled as patches to upstream, so that we can more easily reduce our
technical debt by upstreaming the changes. I don't see documentation
here for patches (for example, there's no debian/patches/ directory).
Perhaps this is handled in source control, but the lack of package-level
visibility will greatly complicate other teams (such as the security
team) from interacting with this package when bugs are discovered in
the embedded libraries.

cppcheck is clean.

Compilation logs are mixed; most warnings look harmless but the code
could be re-formatted so that the warnings are no longer emitted.

Here's some of the things I've found so far; I realize that there's a
chance very little of what I found here is actually in Canonical-authored
code.

./src/plugin/qmltermwidget/src/ksession.cpp:
- appears to set _program to getenv("SHELL") || "/bin/bash"
- setenv("TERM", "xterm")

./src/plugin/qmltermwidget/lib/qtermwidget.cpp:
- appears to set _program to getenv("SHELL")

./src/plugin/qmltermwidget/lib/Session.cpp:
- if _program exists and starts with / see if it is executable
  - otherwise use getenv("SHELL"), if that is executable
  - otherwise use /bin/sh
- if _program doesn't exist or doesn't start with / use it directly

- Pty::start() expects the command name to be representable in latin-1.

Normally a shell like /bin/bash is executed with:
execve("/bin/bash", ["-bash", NULL], env);
I don't see any logic to prefix the shell command name with a hyphen.

I'm surprised that the logic to select the program to execute is spread
over three different files. (That I've found so far.)

I'm surprised the shell-picking logic doesn't use getpwent(3)'s pw_shell
field. I'd expect getenv("SHELL"), then getpwent(), then /bin/sh to be
used, in that order, with no environment variable expansion and no
second-guessing if the specified program is executable or not.

- There are a surprising number of mechanisms in KProcess:: for starting
  processes. Some take lists, some take strings, some are "detached"? What
  does it mean to start a process attached? Detached? When would the
  terminal emulator use one vs the other?

- KPty::open() openpty() discards errno in error message
- KPty::open() is very nearly unreadable; I think the ancient Linux nested
  for loops are executed even when openpty() succeeds. Should they be?

- KSession::changeDir() is gross.
- QTermWidget::changeDir is gross.

- PamAuthentication::requireAuthentication() checks an environment
  variable to determine if authentication is required. This feels wrong;
  why an environment variable? What is the purpose behind the PAM
  authentication integration?

- PamAuthentication::requireAuthentication() appears to see if the Unity
  session is locked before deciding if authentication should be required?
  If the session is locked why is this code being executed?

- PamAuthentication::ConversationFunction() appears to return a password
  regardless of what a PAM module may ask for; appears to not show
  messages to users. PAM modules that log the responses to
  PAM_PROMPT_ECHO_ON questions will wind up logging passwords. Users won't
  be able to use 2fa tokens or software, or respond to other PAM prompts
  as they are needed.

- HistoryScrollBuffer::getCells() I'm confused by the memcpy() call:
  - Do callers always pass startColumn 0? I'd expect to see the starting
    point line.constData() + startColumn * sizeof(Character)
  - Do callers have to be aware that 'count' must satisfy constraints to
    avoid reading off the end of _historyBuffer? There's so many
    implementations and uses of getCells() in so many classes it's hard to
    check them for consistency.

I think this package needs cleanup and hardening work before we can
commit to supporting it in an LTS release.

Thanks