[MIR] ubuntu-terminal-app

Bug #1625074 reported by Alan Pope 🍺🐧🐱 πŸ¦„
22
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Ubuntu Terminal App
Invalid
High
Michael Sheldon
ubuntu-terminal-app (Ubuntu)
Won't Fix
High
Unassigned

Bug Description

Please consider ubuntu-terminal-app for inclusion in main.

We'd like to include the Ubuntu Terminal App in the 16.10 Ubuntu desktop images, and to do that it needs to be in main. This bug is a companion to bug 1623845 which was filed to get it into the archive.

Availability: http://launchpad.net/ubuntu-terminal-app - currently in progress to get in the archive as per bug 1623845

Rationale:

Need a terminal by default on U8 session in Ubuntu desktop images.

Security:

No CVE entries.
No Secunia history.
Binary runs as root, but does not have user controlled input.
Written in QML and C++
Does not open any port.

Quality assurance:

Package works out of the box without configuration.
Has had a year or more of extensive usage on devices as the only terminal app available
Does not deal with exotic hardware which we cannot support.

Standards compliance:

Meets the FHS, Debian Policy
Standard packaging

Dependencies:

libqt5systeminfo5 & qml-module-qtsysteminfo provided by qtsystems-opensource-src which was previously in main, so should be possible to re-add them in again.

Will Cooke (willcooke)
description: updated
description: updated
Revision history for this message
Michael Terry (mterry) wrote :

I reviewed version 0.7ubuntu2~yakketytest in ppa:popey/touch.

This will need a team bug subscriber once it's in Ubuntu.

Need-more-info:

- What's the story with the konsole plugin? Is that bundled code from another package?

Non-blocking notes:

- I would expect to see dh-python3 used (with ${python3:Depends} in debian/control) for the autopilot package. No big deal in this specific case, but that's the usual way to handle python modules.

- It would be nicer to use the proper qml module names of qml-module-* before we land in Ubuntu (instead of qtdeclarative5-*).

- Is qtdeclarative5-pamauthentication0.1 supposed to be installed into a com/ubuntu subdirectory of the normal qml install path? Is it considered a private library? If so, why package it separately from ubuntu-terminal-path?

- I'm sad there's no unit tests. :(

Bill Filler (bfiller)
Changed in ubuntu-terminal-app:
assignee: nobody → Michael Sheldon (michael-sheldon)
importance: Undecided → High
status: New → In Progress
Revision history for this message
David Planella (dpm) wrote :

Thanks Mike for the review. Addressing some of the questions:

> - What's the story with the konsole plugin? Is that bundled code from another package?

Initially the Terminal app evolved from Konsole, but later (IIRC) the UI was decoupled from the terminal backend into what became the QML Terminal widget. If I'm not mistaken, the Konsole code is no longer used. I asked popey to look into it, and I see the MP that removed the Konsole code has now been merged:

http://bazaar.launchpad.net/~ubuntu-terminal-dev/ubuntu-terminal-app/reboot/revision/210

> - I would expect to see dh-python3 used (with ${python3:Depends} in debian/control) for the autopilot package. No big deal in this specific case, but that's the usual way to handle python modules.

> - It would be nicer to use the proper qml module names of qml-module-* before we land in Ubuntu (instead of qtdeclarative5-*).

Luke Yelavich has helped us packaging terminal, so I'd probably forward these questions to him.

> - Is qtdeclarative5-pamauthentication0.1 supposed to be installed into a com/ubuntu subdirectory of the normal qml install path? Is it considered a private library? If so, why package it separately from ubuntu-terminal-path?

I can offer some context hoping that it helps. That qml plugin was created as a requirement before Terminal could be made available in the store and executed in customer phones: it provides an additional level of security by asking you for a password before Terminal can be started. IIRC it has the same requirements in terms of path of installation as any other QML plugin, so if it happens to diverge from that it might be worth correcting it.

Note though, that on Unity 8 running on the desktop you won't be prompted for the password.

Finally, one of the reasons it's a separate package is because the File Manager app reused the same package for password prompt. The next question would probably be: why is qtdeclarative5-pamauthentication0.1 not its own separate project? Mainly for historical reasons: at the time the CI infrastructure was put together for core apps Jenkins could not build .debs for MPs from a mixture of repositories, thus the authentication code was included in the Terminal tree to make it a self-contained project.

> - I'm sad there's no unit tests. :(

No excuses, but just some context: at the time the terminal app development was most active, the guidance was to create Autopilot tests instead.

Revision history for this message
Michael Terry (mterry) wrote :

This has a launchpad page now, so you can sub a team to bugs:
https://launchpad.net/ubuntu/+source/ubuntu-terminal-app

That's the last must-fix issue I think.

Revision history for this message
Alan Pope 🍺🐧🐱 πŸ¦„ (popey) wrote :

I added ~ubuntu-terminal-dev (I think). Which should resolve that?

Revision history for this message
Michael Sheldon (michael-sheldon) wrote :
Revision history for this message
Michael Terry (mterry) wrote :

We keep a list of teams we watch for to make sure we have maintainer coverage:
http://bazaar.launchpad.net/~ubuntu-archive/ubuntu-archive-tools/trunk/view/head:/package-subscribers#L107

Maybe ~system-apps-team would be a better fit for that purpose than ~ubuntu-terminal-dev?

Revision history for this message
Alan Pope 🍺🐧🐱 πŸ¦„ (popey) wrote :

Agreed @mterry. Unfortunately I don't have rights to add ~system-apps-team to bug contact for https://launchpad.net/ubuntu/+source/ubuntu-terminal-app, @bfiller, do you?

Revision history for this message
Pat McGowan (pat-mcgowan) wrote :

subscribed

Michael Terry (mterry)
Changed in ubuntu-terminal-app:
status: In Progress → Fix Committed
Revision history for this message
Will Cooke (willcooke) wrote :

Emily mentioned that the Security team would like to take a look at this so assigning over to them and setting back to new.

Changed in ubuntu-terminal-app:
status: Fix Committed → New
Michael Terry (mterry)
Changed in ubuntu-terminal-app (Ubuntu):
importance: Undecided → High
assignee: nobody → Michael Sheldon (michael-sheldon)
assignee: Michael Sheldon (michael-sheldon) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in ubuntu-terminal-app (Ubuntu):
status: New → Confirmed
Revision history for this message
Seth Arnold (seth-arnold) wrote :

There's no source package in Yakkety?

Revision history for this message
Matthias Klose (doko) wrote :

no package in the distro, only in PPAs

Changed in ubuntu-terminal-app (Ubuntu):
status: Confirmed → Incomplete
Revision history for this message
Will Cooke (willcooke) wrote :

Package has been uploaded to Y and is in the queue.

Will Cooke (willcooke)
Changed in ubuntu-terminal-app (Ubuntu):
status: Incomplete → New
Revision history for this message
Steve Langasek (vorlon) wrote :
Download full text (4.1 KiB)

Package is now in the archive. Pre-promoting to main in advance of security review, per agreement with management.

Override component to main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qml-module-pamauthentication0.1 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qml-module-qmltermwidget1.0 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qtdeclarative5-pamauthentication0.1 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety i386: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety powerpc: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety ppc64el: universe/misc/extra/100% -> main
qtdeclarative5-qmltermwidget1.0 0.7.218ubuntu2 in yakkety s390x: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakkety amd64: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakkety arm64: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakkety armhf: universe/misc/extra/100% -> main
ubuntu-terminal-app 0.7.218ubuntu2 in yakk...

Read more...

Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in ubuntu-terminal-app (Ubuntu):
status: New → Confirmed
Revision history for this message
Seth Arnold (seth-arnold) wrote :
Download full text (4.6 KiB)

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()...

Read more...

Emily Ratliff (emilyr)
Changed in ubuntu-terminal-app (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Jeremy Bicha (jbicha) wrote :

Since Unity8 has been removed from main, this MIR is obsolete so I'm going to go ahead and close this bug.

Changed in ubuntu-terminal-app (Ubuntu):
status: Confirmed → Won't Fix
Jeremy Bicha (jbicha)
Changed in ubuntu-terminal-app:
status: New → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.