Comment 8 for bug 1234983

I agree with Seth,
PAM implementation and integration for it clear many issues.
It resolves itineraries when needing to hold a session type communication.
SSH Session Management Module like
(pam_sm_open_session()) and terminate (pam_sm_close_session()) sessions.
The pam_sm_open_session() : to start authentication phase.

On Wed, Jun 25, 2014 at 3:47 AM, Seth Arnold <email address hidden> wrote:
> I don't like these patches much; the PAM interfaces should have been
> used instead and there are enough oddities and uncertainties in these
> patches that I believe switching to PAM would be easier than trying to
> remedy the issues I see here.
>
> 1. A string is used for Qprocess::start(). Using a string instead of an
> array of arguments invokes the shell and that is almost never safe or
> reliable; for example, if PATH is set to an unexpected value an
> unexpected mkpasswd program may be executed; if IFS is set to unexpected
> values, all sanity is lost; if the salt can be set to have an ;, &, $,
> `, or other shell meta-characters, password authentication steps can
> execute arbitrary code.
>
> 2. The password appears to be forced into latin1() charset in multiple
> places. If the password is Chinese or Japanese what will be the result?
> An exception? A silently shortened string of length 0? Or something
> else?
>
> 3. The settingsFile mode 0600 operation looks susceptible to a race
> condition where it may not be locked down tightly before another process
> can open it; even if the permissions are changed, once the file is
> opened by another process, it can continue using the file descriptor in
> any way. The file needs to be created with the proper permissions in the
> first place. (The open(2) interface with O_CREAT|O_EXCL and mode 0600
> can do this.) The path to the settings file appears to be calculated
> based on the HOME environment variable ('homePath()'), allowing whoever
> is executing this code to specify the file with password contents
> essentially at will. (See the point about salt being unsafe earlier.)
>
> 4. securityValueMatches() has inconsistent immediate return vs setting a
> variable in preparation for a return. It furthermore fails open rather
> than failing closed in the event the default: branch is selected.
>
> 5. I'm concerned that setSecurityValue() sets properties named
> "passwordValue" (with a hashed value) and "passwordEncryptedValue" (with
> an encrypted version). Why are both necessary? When would one or another
> be used? Why do we even want an "encrypted" version?
>
> 6. makeEncryptedPinValue() appears to have multiple fail-open exits and
> a confused sense of purpose. In what way could the pbkdf2 process fail?
> Why would it be acceptable to return a null array in that case? In what
> way could the gcry_cipher_open() or gcry_cipher_encrypt() methods fail?
> Why would it be acceptable to return a null array in those cases? The
> systemd/dbus machineid is not guaranteed to be stable between boots.
> Using an IV of zeros with a fixed number of keys encrypting their own
> contents means lookup tables will be easy to construct. The padding
> operation in this function is not uniquely reversible -- padding should
> always be added, even if that means padding with an entire block, so the
> contents can be recovered afterwards. (Assuming there is even a point to
> having an encrypted version around rather than just the hashed
> versions.)
>
> I'd prefer for the PAM integration to be written instead of trying to
> repair the issues in these patches -- PAM exists to mitigate many of
> these and similar issues in authentication and authorization and its
> flexibility allows it to be easily used for differing security
> requirements.
>
> Thanks
>
> --
> You received this bug notification because you are subscribed to Unity
> 8.
> Matching subscriptions: Unity 8 Bug Reports
> https://bugs.launchpad.net/bugs/1234983
>
> Title:
> greeter pin stored in plain text with hidden demo greeter code
>
> Status in The Unity 8 shell:
> In Progress
> Status in “ubuntu-system-settings” package in Ubuntu:
> New
> Status in “unity8” package in Ubuntu:
> New
>
> Bug description:
> In previous images, there was a setting to setup a PIN or password for
> unlocking the greeter. This feature is no longer exposed in the user
> interface, so this is not a particularly important bug to fix and can
> likely just be closed when proper PAM support is used.
>
> Nevertheless:
>
> # cat /home/phablet/.unity8-greeter-demo
> [General]
> password=pin
> passwordValue=1234
>
> # ls -l /home/phablet/.unity8-greeter-demo
> -rw-r--r-- 1 phablet phablet 42 Sep 20 21:36 /home/phablet/.unity8-greeter-demo
>
> If the demo code is going to be reintroduced into the user interface,
> it should not store the PIN/password in plain text because people may
> not realize it and store an important credential there. It could
> probably remain if both of these were done:
>
> 1. the file is 'chmod 600'
> 2. you used a proper hashing algorithm (see 'man crypt'-- ie, use SHA-512 with a randomly generated salt when the password is set)
>
> If implementing the above, please contact the security team since we
> would want to review the implementation details.
>
> $ adb shell system-image-cli -i
> current build number: 78
> device name: mako
> channel: stable
> last update: 2013-10-03 13:05:32
> version version: 78
> version ubuntu: 20131003
> version device: 20131002.1
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/unity8/+bug/1234983/+subscriptions