greeter pin stored in plain text with hidden demo greeter code

[Jamie Strandboge]

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

[Michael Terry (mterry)]

Note that in current images, the user cannot graphically use that feature. It's not a documented feature at all.

The real fix is to just use PAM for post-v1

[Jamie Strandboge (jdstrand)]

Hmm, I had set this in the past via 'System Settings', but that looks to be gone now. As such, I'll adjust the description.

[Michael Terry (mterry)]

I'll note that the current plan is to ship RTM with this bug.

[Michael Terry (mterry)]

(Although we can probably implement your suggested fixes there.)

[Michael Terry (mterry)]

Jamie, there are two branches linked to this bug. I've requested a review from you for each. Thanks!

[Jamie Strandboge (jdstrand)]

I've asked Seth to take a look at these.

[Seth Arnold (seth-arnold)]

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 se...

[xreuze (xreuze)]

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

[Pat McGowan (pat-mcgowan)]

assume this is not relevant to uss at this time?

[Launchpad Janitor (janitor)]

This bug was fixed in the package unity8 - 8.00+14.10.20140729.1-0ubuntu1

---------------
unity8 (8.00+14.10.20140729.1-0ubuntu1) utopic; urgency=low

  [ Michael Terry ]
  * Check user's pin/password using PAM, instead of a plaintext keyfile.
    New build dependency: libpam0g-dev for phone unlock with PAM (LP:
    #1234983)
 -- Ubuntu daily release <email address hidden> Tue, 29 Jul 2014 23:36:30 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package ubuntu-system-settings - 0.3+14.10.20140729.2-0ubuntu1

---------------
ubuntu-system-settings (0.3+14.10.20140729.2-0ubuntu1) utopic; urgency=low

  [ Michael Terry ]
  * Stop storing user's password in plaintext. Instead, store it in PAM
    the same way the Desktop images do. Also, expose the UI to set it.
    (LP: #1234983)
 -- Ubuntu daily release <email address hidden> Tue, 29 Jul 2014 23:31:40 +0000